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

Bin bash cleanup #19125

Merged
merged 1 commit into from
Oct 5, 2016
Merged

Bin bash cleanup #19125

merged 1 commit into from
Oct 5, 2016

Conversation

markus1189
Copy link
Contributor

Motivation for this change

Found #183 and did some cleanup on hard coded /bin/bash paths. The remaining /bin/bash occurrences found via grep '\b/bin/bash/\b' are calls to sed or substitutions.

Some things I like to have reviewed:

  1. Would it be even better to use stdenv.shell instead of bash?
  2. I assume that the shell scripts from this changeset that use #!/bin/bash are patched automatically somewhere? I still adapted them to use @bash@ to be explicit.
  3. Do I have to do some testing for each package changed or does the CI cover this?
Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • OS X
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@@ -113,6 +113,6 @@ stdenv.mkDerivation rec {
};

passthru = {
shellPath = "/bin/bash";
shellPath = "${bash}/bin/bash";
Copy link
Member

Choose a reason for hiding this comment

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

do you know what shellPath does in this context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, this might not make sense for bash itself ;) I only know passthru not exactly what it is used for, though.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I read the part about passthru but I don't know the relevancy for bash.shellPath ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

@markus1189 all packages that can be used as a login shell have to specify a stub shell path, I believe. So this should almost certainly be left alone.

@joachifm
Copy link
Contributor

joachifm commented Oct 2, 2016

Suggestion: use stdenv.shell instead of passing in bash everywhere. For files that are to be processed by substituteAll,just use @shell@.

Also consider that stuff that ends up in $out will most likely be subject to shebang patching, though I suppose it doesn't hurt to be explicit.

@@ -1,4 +1,4 @@
#!/bin/bash
#!@shell@
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume @shell@ points to the shell executable like stdenv.shell? Btw how can I find out how that @ interpolation works?

Copy link
Contributor

Choose a reason for hiding this comment

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

@shell@ here would be stdenv.shell, yes. There's a builder/bash procedure substituteAll infile outfile that substitutes all @pattern@ with the corresponding shell parameter, so if you have $shell = "${bash}/bin/bash" in the environment when substitution occurs, @shell@ is substituted for that value. From Nix, you'd do something like substituteAll { inherit (stdenv) shell; src = ./foo.sh; }.

@Mic92 Mic92 merged commit 1bd8d77 into NixOS:master Oct 5, 2016
@Mic92
Copy link
Member

Mic92 commented Oct 5, 2016

Thanks!

@markus1189 markus1189 deleted the bin-bash-cleanup branch October 7, 2016 07:12
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.

4 participants