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

cargo: move cert info to fetch-cargo-tarball #210366

Merged
merged 2 commits into from
Jan 31, 2023
Merged

Conversation

linsui
Copy link
Contributor

@linsui linsui commented Jan 12, 2023

Description of changes

Closes #82496
Closes #89526

As proposed in #82496 (review), we should only set the related env vars for the fetcher instead of breaking the function of cargo itself. We add env vers in nix-prefetch-git instead of breaking git. We add env vars in fetchurl instead of breaking curl. We should do the same for cargo.

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/)
  • 23.05 Release Notes (or backporting 22.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.

@zowoq
Copy link
Contributor

zowoq commented Jan 31, 2023

Thanks for the PR, I've been meaning to follow up on rust/cacert. #206195 (comment)

I think we might be able to remove cacert from buildRustPackage as well?

diff --git a/pkgs/build-support/rust/build-rust-package/default.nix b/pkgs/build-support/rust/build-rust-package/default.nix
index 69ee4f56b98..b057d1681b0 100644
--- a/pkgs/build-support/rust/build-rust-package/default.nix
+++ b/pkgs/build-support/rust/build-rust-package/default.nix
@@ -4,7 +4,6 @@
 , rust
 , stdenv
 , callPackage
-, cacert
 , cargoBuildHook
 , cargoCheckHook
 , cargoInstallHook
@@ -124,7 +123,6 @@ stdenv.mkDerivation ((removeAttrs args [ "depsExtraArgs" "cargoUpdateHook" "carg
       inherit cargo cargo-auditable;
     })
   ] ++ [
-    cacert
     cargoBuildHook
     (if useNextest then cargoNextestHook else cargoCheckHook)
     cargoInstallHook

@@ -73,6 +73,10 @@ in stdenv.mkDerivation ({

${cargoUpdateHook}

# Override the `http.cainfo` option usually specified in `.cargo/config`.
export CARGO_HTTP_CAINFO=${cacert}/etc/ssl/certs/ca-bundle.crt
export SSL_CERT_FILE=${cacert}/etc/ssl/certs/ca-bundle.crt
Copy link
Contributor

Choose a reason for hiding this comment

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

SSL_CERT_FILE is set by the cacert setupHook so probably don't need this.

Suggested change
export SSL_CERT_FILE=${cacert}/etc/ssl/certs/ca-bundle.crt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks!

@zowoq zowoq requested a review from figsoda January 31, 2023 01:16
@figsoda
Copy link
Member

figsoda commented Jan 31, 2023

not really familiar with certificates, changes lgtm aside from the things zowoq pointed out

@zowoq
Copy link
Contributor

zowoq commented Jan 31, 2023

May as well make use of the new team. cc @nixos/rust


I reverted the formatting changes and added the buildRustPackage diff in a separate commit.

@linsui The email you're using for your commits doesn't seem to match with your github account so the authorship of the commits on github doesn't look correct.

@linsui
Copy link
Contributor Author

linsui commented Jan 31, 2023

I didn't update my local git config. 🤷

@winterqt
Copy link
Member

@linsui Check your GitHub email settings to make sure linsui555@gmail.com is (still) there?

@linsui
Copy link
Contributor Author

linsui commented Jan 31, 2023

It's not there. I updated it on github but not locally. 🤷

@winterqt
Copy link
Member

If I'm understanding your comment correctly: can you fix the commits to point to the right email, then? Thanks.

@linsui
Copy link
Contributor Author

linsui commented Jan 31, 2023

Ah, if that's required I can fix it.

@winterqt
Copy link
Member

Can you maybe include an overview/rationale in the first commit's description? A link to previous discussions would probably also be good to add :)

LGTM otherwise, thanks for this!

linsui and others added 2 commits January 31, 2023 22:28
As proposed in NixOS#82496, we should only set the related env vars for the fetcher instead of breaking the function of cargo itself.
not needed here, set by fetchCargoTarball
Copy link
Member

@figsoda figsoda left a comment

Choose a reason for hiding this comment

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

lgtm

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