-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
gst_all_1.gstreamer: use a better libunwind check #339006
Conversation
This will allow packages like gstreamer that need to find libunwind via pkg-config to check whether the libunwind they've been given has the modules they expect.
Checking for Darwin didn't take into account that we also use a libunwind implementation without pkg-config on riscv32-linux. Fix by checking for the presence of the pkg-config file required directly, rather than by using platform as a proxy.
b4ea269
to
52b0b4e
Compare
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.
Shouldn’t result in a behavioural change on the major platforms, so this seems fine to optimistically merge.
, withLibunwind ? !stdenv.isDarwin && lib.meta.availableOn stdenv.hostPlatform libunwind | ||
, withLibunwind ? | ||
lib.meta.availableOn stdenv.hostPlatform libunwind && | ||
lib.elem "libunwind" libunwind.meta.pkgConfigModules or [] |
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.
Nit: I find f x.a or b
to be a really confusing parse, and would prefer parentheses around the relevant subexpression here. (Non‐blocking, will merge soon regardless of any response if you don’t beat me to it.)
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 also find it confusing, but unless we can enforce parentheses treewide, I think it's better to teach it through exposure, rather than making it rare and therefore even more surprising.
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.
Opened NixOS/nixfmt#251 and merging :)
Description of changes
Checking for Darwin didn't take into account that we also use a
libunwind implementation without pkg-config on riscv32-linux. Fix by
checking for the presence of the pkg-config file required directly,
rather than by using platform as a proxy.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.