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

dockerTools: mark store-path-to-layer.sh as executable #56151

Merged

Conversation

thoughtpolice
Copy link
Member

bcf54ce introduced a treewide change to use ${stdenv.shell} where-ever possible. However, this broke a script used by dockerTools, store-path-to-layer.sh, as it did not preserve the +x mode bit. This meant the file got put into the store as mode 0444, resulting in a build-time error later on that looked like:

xargs: /nix/store/jixivxhh3c8sncp9xlkc4ls3y5f2mmxh-store-path-to-layer.sh: Permission denied

However, in a twist of fate, bcf54ce not only introduced this regression but, in this particular instance, didn't even fix the original bug: the store-path-to-layer.sh script still uses /bin/sh as its shebang line, rather than an absolute path to stdenv. (Fixing this can be done in a separate commit.)

/cc @rnhmjoj

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

bcf54ce introduced a treewide change to
use ${stdenv.shell} where-ever possible. However, this broke a script
used by dockerTools, store-path-to-layer.sh, as it did not preserve the
+x mode bit. This meant the file got put into the store as mode 0444,
resulting in a build-time error later on that looked like:

    xargs: /nix/store/jixivxhh3c8sncp9xlkc4ls3y5f2mmxh-store-path-to-layer.sh: Permission denied

However, in a twist of fate, bcf54ce
not only introduced this regression but, in this particular instance,
didn't even fix the original bug: the store-path-to-layer.sh script
*still* uses /bin/sh as its shebang line, rather than an absolute path
to stdenv. (Fixing this can be done in a separate commit.)

Signed-off-by: Austin Seipp <aseipp@pobox.com>
@thoughtpolice thoughtpolice added the 0.kind: regression Something that worked before working no longer label Feb 21, 2019
@thoughtpolice thoughtpolice self-assigned this Feb 21, 2019
@thoughtpolice thoughtpolice merged commit c36c048 into NixOS:master Feb 21, 2019
@thoughtpolice thoughtpolice deleted the nixpkgs/fix-docker-image-build branch February 21, 2019 14:30
@rnhmjoj
Copy link
Contributor

rnhmjoj commented Feb 21, 2019

I'm sorry I haven't checked more extensively. I'll have to check all other instances of substitueAll.
Thank you for letting me know.

@thoughtpolice
Copy link
Member Author

@rnhmjoj No worries, it happens. I didn't put together a small reproducer or anything, but I've got a million things on my plate; do CC me if you want anyone to test or review something.

@rnhmjoj rnhmjoj mentioned this pull request Feb 23, 2019
@rnhmjoj rnhmjoj mentioned this pull request Mar 5, 2019
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: regression Something that worked before working no longer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants