Skip to content
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

{cc,bintools}-wrapper: Some fixes #92089

Merged

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Jul 2, 2020

Motivation for this change
  1. This means we can freely keep the comments up to date without the penalty of a mass rebuild.

  2. This will unbreak firefox and a few other packages which try to grab some of the libcxx flags.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built stdenv on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

This means we can freely keep the comments up to date without the
penalty of a mass rebuild.
This will unbreak firefox and a few other packages which try to grab
some of the libcxx flags.
@@ -228,58 +231,56 @@ stdenv.mkDerivation {
*) echo "Multiple dynamic linkers found for platform '${targetPlatform.config}'." >&2;;
esac

if [ -n "''${dynamicLinker:-}" ]; then
if [ -n "''${dynamicLinker-}" ]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mainly curious... this dash is some kind of special operator? I can't find this one in man bash.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vcunat See https://wiki.bash-hackers.org/syntax/pe#use_a_default_value

If you omit the : (colon), like shown in the second form, the default value is only used when the parameter was unset, not when it was empty.

Copy link
Member

@vcunat vcunat Jul 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Now I also read that man bash part again – more properly – and I see it there as well 🤦

Copy link
Member

@vcunat vcunat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's have this in the current staging-next iteration. The risks seem low enough and otherwise we'd have to work around those problems in some other way.

@vcunat vcunat merged commit 9dcb508 into NixOS:staging-next Jul 2, 2020
@Ericson2314 Ericson2314 deleted the wrapper-ensure-file-exists branch July 2, 2020 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants