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

buildGoModule: do not strip modSha256 from derivation #82027

Merged
merged 1 commit into from Mar 11, 2020

Conversation

@bhipple
Copy link
Contributor

bhipple commented Mar 7, 2020

Normally the only reason to strip an attr from a derivation is to prevent a
rebuild when the attr changes or hide a private implementation detail, but this
attribute is a meaningful part of the package, so causing a rebuild on it
changing is not an issue. Keeping it as part of the derivation allows us to
inspect the modSha256 in tools like nixpkgs-update.

Specifically, this allows us to run a cmd like nix eval -f . pkgs.tflint.drvAttrs.modSha256
to get the current value, which is how the bot finds it to replace with the new
version in the Rust ecosystem.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 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.
@bhipple
Copy link
Contributor Author

bhipple commented Mar 7, 2020

@bhipple
Copy link
Contributor Author

bhipple commented Mar 7, 2020

@GrahamcOfBorg build tflint

@ryantm
Copy link
Member

ryantm commented Mar 9, 2020

Requesting review from @basvandijk who is the last person to touch this line according to git blame.

@ryantm
Copy link
Member

ryantm commented Mar 9, 2020

Requesting review from @kalbasit who introduced this change in a0d835e

Edit: Oops, sorry, this isn't correct. I was rushing.

Edit2: here is the correct commit 28435e4

@bhipple bhipple force-pushed the bhipple:fix/go-modsha256 branch from af2e066 to d3f0826 Mar 10, 2020
The builder does not technically need the modSha256 of the vendor dir, and even
though we pass it the entire vendor dir it makes sense not to risk having an
accidental dependency on that variable.

However, tools like [nixpkgs-update](https://github.com/ryantm/nixpkgs-update)
need to inspect the `modSha256` of a package in order to be able to update them,
and since this is a real part of the package (describes info about its
dependencies) let's add it to `passthru`.

Specifically, this allows us to run a cmd like `nix eval -f . tflint.modSha256`
to get the current value, which is how the bot finds it to replace with the new
version in the Rust ecosystem.
@ryantm
ryantm approved these changes Mar 11, 2020
@kalbasit kalbasit merged commit 0723df3 into NixOS:staging Mar 11, 2020
12 checks passed
12 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@kalbasit
Copy link
Member

kalbasit commented Mar 11, 2020

Oops the base was set to staging. Feel free to cherry-pick it to master if you need it sooner there.

Mic92 added a commit that referenced this pull request Mar 11, 2020
The builder does not technically need the modSha256 of the vendor dir, and even
though we pass it the entire vendor dir it makes sense not to risk having an
accidental dependency on that variable.

However, tools like [nixpkgs-update](https://github.com/ryantm/nixpkgs-update)
need to inspect the `modSha256` of a package in order to be able to update them,
and since this is a real part of the package (describes info about its
dependencies) let's add it to `passthru`.

Specifically, this allows us to run a cmd like `nix eval -f . tflint.modSha256`
to get the current value, which is how the bot finds it to replace with the new
version in the Rust ecosystem.
@Mic92
Copy link
Contributor

Mic92 commented Mar 11, 2020

Cherry picked: 5f77ff6

dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request Mar 11, 2020
The builder does not technically need the modSha256 of the vendor dir, and even
though we pass it the entire vendor dir it makes sense not to risk having an
accidental dependency on that variable.

However, tools like [nixpkgs-update](https://github.com/ryantm/nixpkgs-update)
need to inspect the `modSha256` of a package in order to be able to update them,
and since this is a real part of the package (describes info about its
dependencies) let's add it to `passthru`.

Specifically, this allows us to run a cmd like `nix eval -f . tflint.modSha256`
to get the current value, which is how the bot finds it to replace with the new
version in the Rust ecosystem.

(cherry picked from commit 5f77ff6)
@bhipple
Copy link
Contributor Author

bhipple commented Mar 11, 2020

Originally I set it to staging since this rebuilt every Go package, but now that we're just adding it to passthru it has no recompilations; thanks for the review + feedback + cherry-picking to master!

bhipple added a commit to bhipple/nixpkgs-update that referenced this pull request Mar 13, 2020
Having merged an update to make go packages passthru the `modSha256` [1], we can
now have `nixpkgs-update` inspect it and make updates, just as it now does for
Rust packages as of [2].

As a slight detail, because the attribute is in `passthru` rather than actually
being available in the derivation, we just do the equivalent of
`nix eval -f . <attrPath>.<attr>` rather than going through the `drvAttrs` as before.

[1] NixOS/nixpkgs#82027
[2] ryantm#156
bhipple added a commit to bhipple/nixpkgs-update that referenced this pull request Mar 13, 2020
Having merged an update to make go packages passthru the `modSha256` [1], we can
now have `nixpkgs-update` inspect it and make updates, just as it now does for
Rust packages as of [2].

As a slight detail, because the attribute is in `passthru` rather than actually
being available in the derivation, we just do the equivalent of
`nix eval -f . <attrPath>.<attr>` rather than going through the `drvAttrs` as before.

This version has been tested successfully with at least one go update [3], but
not thoroughly vetted. That said, it's essentially identical to the Rust
implementation that has been working well for some time now.

[1] NixOS/nixpkgs#82027
[2] ryantm#156
[3] NixOS/nixpkgs#82465
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.