-
Notifications
You must be signed in to change notification settings - Fork 549
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[alsa] Bump to v1.2.11 #8485
[alsa] Bump to v1.2.11 #8485
Conversation
Shoot I also just bumped [alsa_plugins] to v1.2.7, forgetting that would get added in here |
This reverts commit 3e5bee8.
A/alsa/build_tarballs.jl
Outdated
] | ||
|
||
# Bash recipe for building across all platforms | ||
script = raw""" | ||
cd $WORKSPACE/srcdir/alsa-lib*/ | ||
./configure --prefix=${prefix} --build=${MACHTYPE} --host=${target} | ||
./configure --prefix=${prefix} --with-configdir=/usr/share/alsa --with-plugindir=/usr/lib/alsa-lib --build=${MACHTYPE} --host=${target} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's slightly awkward to hardcode them like this, but I guess there isn't really a good way around that. Have you tested this locally to make sure the paths all work out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would that work on nix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not on nix, and this will also reference the user's existing Alsa stuff, not anything we provide in the JLL. There are apparently environment variables that could be used to work around this at runtime (they basically override this setting apparently).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was moderately sure we were doing this already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be better to having an init block like (it's a sketch, totally untested):
get!(ENV, "ALSA_PLUGIN_DIR", joinpath(artifact_dir, "lib", "alsa-lib"))
rather than hardcoding absolute paths that we can't know whether will exist. Other projects seem to be doing this already, e.g. JuliaMultimedia/SimpleDirectMediaLayer.jl#65
This reverts commit a7d1982.
Thanks for the discussion & link! |
I think you still want to add the init block? After testing it of course 🙂 I have no idea of what to test |
Oh sorry I got confused, I thought the move was just to add it^ to any downstream packages using alsa_jll. I can build locally (though that's the same as the checks here), but I don't have a linux system to test locally |
No, I was suggesting setting the keyword argument |
I don't think I have the expertise to do that, and that's just an experimental argument, right? I'm clueless how |
Now just bumping alsa