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

rustPlatform.importCargoLock: add support for v1 lock files #215629

Merged
merged 1 commit into from
Feb 26, 2023

Conversation

winterqt
Copy link
Member

Description of changes

v1 lock files (generated by default by Cargo versions 1.40 and below) use a single table, metadata, to store the checksums of packages.

The primary motivation for doing this now is that we're considering vendoring all Cargo lock files in Nixpkgs, some packages still use it (e.g. cargo-asm), and adding support for it doesn't increase the complexity of the function. No matter the outcome of the vendoring discussion, this is a nice thing to have because Cargo still supports v1 lock files.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin, nix-build -A tests.importCargoLock
  • 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.

v1 lock files (generated by default by Cargo versions 1.40 and below)
use a single table, `metadata`, to store the checksums of packages.

The primary motivation for doing this now is that we're considering
vendoring all Cargo lock files in Nixpkgs, some packages still use it
(e.g. cargo-asm), and adding support for it doesn't increase the
complexity of the function. No matter the outcome of the vendoring
discussion, this is a nice thing to have because Cargo still supports v1
lock files.
@winterqt
Copy link
Member Author

winterqt commented Feb 10, 2023

@ofborg build tests.importCargoLock

The primary motivation for doing this now is that we're considering vendoring all Cargo lock files in Nixpkgs, some packages still use it (e.g. cargo-asm), and adding support for it doesn't increase the complexity of the function.

Yes, this is foreshadowing (sorry); I'll be posting the details relatively soon, so we can actually have this discussion. I want to land this either way, though.

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, that was easier than I expected

pkgs/build-support/rust/import-cargo-lock.nix Show resolved Hide resolved
Comment on lines +87 to 89
assert lib.assertMsg (checksum != null) ''
Package ${pkg.name} does not have a checksum.
Please note that the Cargo.lock format where checksums used to be listed
under [metadata] is not supported.
If that is the case, running `cargo update` with a recent toolchain will
automatically update the format along with the crate's depenendencies.
'';
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe I should adjust the message to say something like "the lock file may be corrupted, you can generate one with ...", instead of now just leaving it vague?

Copy link
Member Author

Choose a reason for hiding this comment

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

This also does beg the question of why v1 support was intentionally omitted from the initial implementation -- is there some issue we may run into? I haven't run into anything in my limited testing, at least, so I hope that's not it.

Copy link
Member

Choose a reason for hiding this comment

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

Leaving it as it is lgtm. I think suggesting running cargo update could be helpful, but I don't think we should say it might be corrupted, since the issue is probably more likely to be impoCargoLock instead of Cargo.lock

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Feb 10, 2023
@winterqt winterqt merged commit 115e341 into NixOS:master Feb 26, 2023
@winterqt winterqt deleted the import-cargo-lock-v1-support branch February 26, 2023 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: rust 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants