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

coreboot-toolchain: remove hardcoded hash of 21.11's x86_64-linux.stdenv #171223

Merged
merged 2 commits into from Jul 27, 2022
Merged

coreboot-toolchain: remove hardcoded hash of 21.11's x86_64-linux.stdenv #171223

merged 2 commits into from Jul 27, 2022

Conversation

ghost
Copy link

@ghost ghost commented May 2, 2022

(@felixsinger, thank you so much for writing the coreboot-toolchain expression!)

The problem

The following line is found in tools/misc/coreboot-toolchain/default.nix:

  sha256 = "073n8yid3v0l9wgwnrdqrlgzaj9mnhs33a007dgr7xq3z0iw3i52"

This hash matches the output of the fetchgit invocation in the same file; however that fetchgit invocation uses a postFetch block with patchShebangs, so the nix store-path of stdenv.shell is now hardwired into the hash above.

This means that this derivation will fail to build for any user whose stdenv store-path does not exactly match the one used at the time the derivation was first built by hydra. For example:

  • Anybody building on a platform other than x86_64-linux, since they have a different boostrap-files.

  • Anybody with an overlay which adds -march=...

  • Any future nixpkgs users, if nixpkgs commits a change which influences the store-path of stdenv.

See for yourself

To reproduce the problem, check out this PR, revert the last commit, and:

nix-build \
  --option substituters "" \
  --option extra-substituters "" \
  --option trusted-substituters "" \
  --option extra-trusted-substituters "" \
  --option trusted-public-keys "" \
  -A coreboot-toolchain.aarch64

Some of those --options are probably redundant, but I can never seem to get nix's rules for substituters straight.

Assuming you didn't already have the outpath in your store (if you did, delete it, start over), you'll see:

error: output '/nix/store/nvswbzyl39ifpwswfvx132j2mys80ifr-coreboot' is not allowed to refer to the following paths:
         /nix/store/8ngciqnw8jzvyvbx00arkp05gvn5q6sq-libunistring-1.0
         /nix/store/p2r9ynirymj47x5m6y9pnq0lpssn4ahm-bash-5.1-p16
         /nix/store/rflgyvwcnmrql5wf8kchynmmq7raggvj-libidn2-2.3.2
         /nix/store/rszg7d581z3v3fwrak68ba2wv5lrckx7-glibc-2.34-115

The fix

A three-patch series:

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ghost ghost requested a review from felixsinger May 2, 2022 12:03
Copy link
Member

@felixsinger felixsinger left a comment

Choose a reason for hiding this comment

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

Commit messages missing and problem statement needs to be improved.

@ghost
Copy link
Author

ghost commented May 2, 2022

Commit messages missing and problem statement needs to be improved.

Done.

Mainly the middle commit; the first commit is a straightforward one-line code change. The last commit fixes the issue described in the middle commit, so the last commit's message refers the reader to the previous commit's message.

@ghost ghost requested a review from felixsinger May 2, 2022 21:32
@ghost
Copy link
Author

ghost commented May 28, 2022

Ping...

@ghost
Copy link
Author

ghost commented May 28, 2022

Force-push resolves merge conflict with 7aae279, no other changes.

@ghost
Copy link
Author

ghost commented Jun 9, 2022

Ping...

@ghost ghost changed the title fetchgit: allowedRequisites parameter, use in coreboot-toolchain fix coreboot-toolchain: remove hardcoded hash of 21.11's x86_64-linux.stdenv Jun 11, 2022
@ghost
Copy link
Author

ghost commented Jun 22, 2022

Ping...

@ghost ghost marked this pull request as draft June 24, 2022 05:50
@ghost ghost marked this pull request as ready for review June 24, 2022 05:51
@ghost
Copy link
Author

ghost commented Jun 24, 2022

Rebased since #177326 has merged.

Adam Joseph added 2 commits June 23, 2022 22:53
This commit exposes a bug in the coreboot-toolchain expression; if you
nix-build that expression at this commit and do not use subsituters,
you will get a failure like this:

error: output '/nix/store/nvswbzyl39ifpwswfvx132j2mys80ifr-coreboot' is not allowed to refer to the following paths:
         /nix/store/8ngciqnw8jzvyvbx00arkp05gvn5q6sq-libunistring-1.0
         /nix/store/p2r9ynirymj47x5m6y9pnq0lpssn4ahm-bash-5.1-p16
         /nix/store/rflgyvwcnmrql5wf8kchynmmq7raggvj-libidn2-2.3.2
         /nix/store/rszg7d581z3v3fwrak68ba2wv5lrckx7-glibc-2.34-115

The root cause of the bug is this line in
tools/misc/coreboot-toolchain/default.nix:

  sha256 = "073n8yid3v0l9wgwnrdqrlgzaj9mnhs33a007dgr7xq3z0iw3i52"

This hash covers the result of the fetchgit operation, including the
postFetch block.  The postFetch block runs patchShebangs, which writes
the store-path of ${stdenv.shell} into $out/util/crossgcc/buildgcc
*before* computing its hash.

The next commit after this one fixes the bug by moving patchShebangs
out of the postFetch block, so it happens *after* the hash is
computed.

Note that even without allowedRequisites=[], the state of the code
prior to this commit is problematic.  Because it hardcodes the
store-path of stdenv, the expression will break in these situations:

* Building on a platform other than x86_64-linux, since those
  platforms will have a different boostrap-files store-path and
  therefore a different stdenv store-path.

* Building with an overlay which adds -march= or other compiler flag
  customizations to stdenv.

* Future nixpkgs users if nixpkgs commits any change which influences
  the store-path of stdenv.

So we should fix the problem in any event.  The `allowedRequisites=[]`
added by this commit ensures that the problem will be noticed
immediately if it recurs in the future.
This commit fixes the issue described in detail in the commit message
of the previous commit.
@ghost
Copy link
Author

ghost commented Jul 16, 2022

Ping...

@vcunat
Copy link
Member

vcunat commented Jul 27, 2022

To reproduce the issue I recommend simply

nix-build -QA coreboot-toolchain.x64.src --check

@vcunat vcunat merged commit 580d083 into NixOS:master Jul 27, 2022
@ghost ghost deleted the fetchgit-allowedRequisites branch July 27, 2022 19:55
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.

2 participants