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

tree-wide: convert rust with git deps to importCargoLock #221716

Merged
merged 9 commits into from
Mar 26, 2023
Merged

tree-wide: convert rust with git deps to importCargoLock #221716

merged 9 commits into from
Mar 26, 2023

Conversation

yu-re-ka
Copy link
Contributor

@yu-re-ka yu-re-ka commented Mar 17, 2023

Description of changes

Less invasive alternative to #217084

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
  • Fits CONTRIBUTING.md.

@winterqt
Copy link
Member

0319eccab39512bf55afa1536515c21134a1dbd0 should explain why, at least in the commit message, as it does technically work. I assume you just want to discourage its use?

@yu-re-ka
Copy link
Contributor Author

Well, with cargo 1.68 and above we can not rely on it being reproducible with git dependencies, as there is extended logic involved now which might change in future versions, so we would have to regenerate the FOD hashes on every update.

The assumption is that cargo vendor is still reproducible when no git dependencies are involved.

@winterqt
Copy link
Member

Please re-read my comment -- that's exactly what I'm trying to say; it technically works, but we don't want anyone to actually use it. This should be made more clear, at least in the commit message, IMO.

@zowoq
Copy link
Contributor

zowoq commented Mar 18, 2023

There are several lockfiles committed without a corresponding nix package change?

e.g:

squeekboard/Cargo.lock
gnome-podcats/Cargo.lock
emblem/Cargo.lock
rnote/Cargo.lock
citations/Cargo.lock
newsflash/Cargo.lock
warp/Cargo.lock

@oxalica
Copy link
Contributor

oxalica commented Mar 18, 2023

Nixpkgs size increase of this PR is ~5% before compression, ~6% after compression.

$ fd '^Cargo.lock$' -X cat | wc -c
8.8Mi

$ git archive --format=tar HEAD~3 | wc -c
162600960
$ git archive --format=tar HEAD | wc -c
170997760

$ git archive --format=tar.gz HEAD~3 | wc -c
34881234
$ git archive --format=tar.gz HEAD | wc -c
37023424

@yu-re-ka
Copy link
Contributor Author

There are several lockfiles committed without a corresponding nix package change?

good find, these should be fixed now

@yu-re-ka yu-re-ka changed the base branch from staging to master March 18, 2023 15:37
@ofborg ofborg bot added 10.rebuild-darwin: 101-500 2.status: merge conflict This PR has merge conflicts with the target branch and removed 10.rebuild-darwin: 11-100 labels Mar 18, 2023
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 18, 2023
@yu-re-ka
Copy link
Contributor Author

yu-re-ka commented Mar 18, 2023

I have run nix-review to ensure that everything still builds.

These packages are already broken on master: fedigroups, habitat

@figsoda
Copy link
Member

figsoda commented Mar 19, 2023

Should I move the jumpy changes to #221921? Didn't see it when I was updating jumpy, sorry

@figsoda
Copy link
Member

figsoda commented Mar 19, 2023

oh cool that will work too

@@ -76,6 +76,17 @@ in stdenv.mkDerivation ({
# Override the `http.cainfo` option usually specified in `.cargo/config`.
export CARGO_HTTP_CAINFO=${cacert}/etc/ssl/certs/ca-bundle.crt

if grep --pcre2 '^source = "(?!registry)' Cargo.lock; then
Copy link
Contributor

Choose a reason for hiding this comment

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

grep: unrecognized option '--pcre2'

Copy link
Member

@figsoda figsoda Mar 26, 2023

Choose a reason for hiding this comment

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

perhaps we can just do ^source = "git\+ since we don't really know how to deal with non-git deps anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my alias grep=rg in my environment 😅

The reason is that we can not expect the extended logic run on git
dependencies starting from Cargo 1.68 to be reproducible in future
 versions, and thus the output hash would not be sufficiently stable.
rust-lang/cargo#11414
@zowoq zowoq merged commit d94076a into NixOS:master Mar 26, 2023
@yu-re-ka yu-re-ka deleted the git-deps-import-cargo-lock branch March 26, 2023 06:33
@superherointj
Copy link
Contributor

superherointj commented Mar 27, 2023

Noticed a spike averaging ~6.5 seconds in eval times at nixpkgs. On bisecting found:

Preceding commit to this PR:

  • 2f0ff93 Merge pull request #222585 from fabaff/yara-bump
    • real 1m8,129s
      user 1m5,506s
      sys 0m2,620s

At this PR:

  • f8cbc3c tree-wide: convert rust with git deps to importCargoLock
    • real 1m14,646s
      user 1m12,097s
      sys 0m2,546s

Command used for testing:

  • time nix-env --query --available --out-path --file ./. > /dev/null

@Mic92
Copy link
Member

Mic92 commented Mar 29, 2023

Also around 10GB of RAM on x86_64-linux. Haven't compared what we had before.

@tomhoule tomhoule mentioned this pull request Mar 29, 2023
12 tasks
@alyssais
Copy link
Member

alyssais commented Mar 29, 2023

That's really weird, considering that in #217084 we found that converting all Rust packages rather than just ones with git deps had almost no impact on CPU time, and a similarly low impact on memory. cc @winterqt

@alyssais
Copy link
Member

OfBorg also found a much bigger evaluation performance difference than we were expecting from Winter's research: https://github.com/NixOS/nixpkgs/runs/12279178729

@kirillrdy
Copy link
Member

@yu-re-ka apologies if this has been answered somewhere, but is there a reason for not using Cargo.lock from ${src}/Cargo.lock ( where possible of course )

@kirillrdy
Copy link
Member

@yu-re-ka apologies if this has been answered somewhere, but is there a reason for not using Cargo.lock from ${src}/Cargo.lock ( where possible of course )

I think I've found answer,

during evaluation because the option 'allow-import-from-derivation' is disabled

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.