buildRustPackage: remove git from nativeBuildInputs#206195
Closed
Atemu wants to merge 2 commits intoNixOS:stagingfrom
Closed
buildRustPackage: remove git from nativeBuildInputs#206195Atemu wants to merge 2 commits intoNixOS:stagingfrom
Atemu wants to merge 2 commits intoNixOS:stagingfrom
Conversation
Having git in nativeBuildInputs causes thousands of rebuilds when git is touched because every derivation that somehow transiently depends on a rust package will change. See NixOS#205682 for instance. AFAICT git is unused. Only the fetcher needs git which it has already. Fixes NixOS#205804
Cacert doesn't have any binary parts; it does not make sense to have it in nativeBuildInputs. Its home sould be buildInputs. It probably shouldn't be in there either as I can't imagine it being a general requirement for rust packages and any package that depends on cacert should do so explicitly. I don't have the experience with the rust packages set to know whether that's the case, so let's err on the side of caution.
github-actions
bot
added
the
6.topic: rust
ofborg
bot
added
10.rebuild-darwin: 501+
Atemu
changed the title
Mic92
reviewed
Dec 15, 2022
| ]; | ||
|
|
||
| buildInputs = buildInputs | ||
| ++ [ cacert ] # FIXME Is this actually needed? |
Member
There was a problem hiding this comment.
Does this even have an effect when its in buildInputs?
Contributor
There was a problem hiding this comment.
Yes:
$ nix print-dev-env --impure --expr 'with import ./. {}; pkgsCross.riscv64.mkShell { buildInputs = [ cacert ]; }' | grep SSL
NIX_SSL_CERT_FILE='/nix/store/65wwvlzss9wsrsjwidrp2i0p0ps38nvg-nss-cacert-3.83/etc/ssl/certs/ca-bundle.crt'
export NIX_SSL_CERT_FILE
$ nix print-dev-env --impure --expr 'with import ./. {}; pkgsCross.riscv64.mkShell { }' | grep SSL
$
Member
Author
There was a problem hiding this comment.
@Mic92 asked the other way around, what should having it in nativeBuildInputs achieve?
AFAIK, the only distinction made is when cross-compiling where nativeBuildInputs are expected to run on the host while buildInputs are for the target.
Since you're here, do you know whether some rust packages need this env var to build a usable package? (Especially w.r.t. internet-facing ones.)
Contributor
|
Removing |
1 task
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of changes
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes