Stop fabricating PackageId and look it up instead

[pacak]

• lock.rs: get rid of confusing Metadata type alias
• IndexedMetadata::new_from_merged doesn't consume MergedMetadata
• Lookup PackageId in resolved metadata

Fixes #340

[kolloch]

Hey @pacak,

thanks for trying to address this! I believe the general direction is the right one, even though I forgot much about the context in the code since it is quite a long time that I wrote (parts of) it. Let me comment on the easy to agree with parts:

• lock.rs: get rid of confusing Metadata type alias

Note that this is code copied from elsewhere, so potentially it would be worth it to keep it closer to the source, but hey, I merged it.

• IndexedMetadata::new_from_merged doesn't consume MergedMetadata

Makes sense, since it doesn't reuse anything anyways

[kolloch]

I noticed that you removed quite some tests, can you comment on that?

[kolloch]

Please don't delete them, rather add some for the new cases :)

[pacak]

Note that this is code copied from elsewhere, so potentially it would be worth it to keep it closer to the source

I don't have strong opinions about it, but knowing that we are working with cargo-metadata- I assumed that it was Metadata from that crate and tried to use it accordingly, wasting some time. Just wanted to save this trouble for other people.

I noticed that you removed quite some tests, can you comment on that?

New code relies on info from cargo-metadata, tests don't have it included. Faking might work but it isn't the best idea. For as long as this crate uses itself to generate its own Cargo.nix this serves as a test for current lock file format. Legacy might or might not work, but using the most recent crate2nix with very old rust tool chain...

[kolloch]

I would like the code supporting for the old legacy formats tested.

Not sure how to decide that it is not needed anymore, then the code could be removed and the tests as well. But I'd not like to do that without a deprecation period.

[pacak]

Not sure how to decide that it is not needed anymore, then the code could be removed and the tests as well. But I'd not like to do that without a deprecation period.

Comment calling it legacy was added in January 2020, so at most 1.42 - 35 rust releases ago. crate2nix requires at least 1.56 to compile - it uses 2021 edition, actual MSRV might be higher due to dependencies. I'd say it is safe to drop the support right now but it goes outside of the scope for this pull request.

[kolloch]

Still, can we add tests for the new stuff?

Sure, it will probably fail elsewhere if there is a problem.

But it is usually easier to understand with more specific tests.

[pacak]

Still, can we add tests for the new stuff?

Will look into that when I have free time.

[flokli]

@kolloch can you publish a release with this?

[kolloch]

Done, hopefully not too rushed.

[flokli]

Thanks! I opened NixOS/nixpkgs#303309 so nixpkgs has the fix too.

[Ten0]

Not sure if that's relevant but IFD code seems to still generate such IDs with old format to store them in crate-hashes.json, while Rust code now generates those with the new format.
Looking at #340 (comment), it seems that it may be what's causing #348 (not sure though because even when giving to IFD extra hashes from manually generated crate-hashes.json I still observe #348).

[pacak]

I'm seeing stuff like this which suggests new format. Old format comes from old version of cargo.

  "git+https://github.com/nix-rust/nix?rev=fbdac70c78975fe7c75b882337bf9f40a639f51a#0.25.0": "1jvls4d90ns5qwqab1a2hcyff2dlls1q4i5xl6cs09a371g5h7b3",

[pacak]

And in case of #340 it was working, but wasn't able to use cache so I don't think it's related.

[Ten0]

Even with rust 1.77.1 being used everywhere, I see IFD like this:

++ crate2nix generate -f ./.cargo/workspace/Cargo.toml -o Cargo-generated.nix -h /nix/store/f443x0imd2b8r7gb5jkyai9j32y6xhmv-proj-crate2nix/crate-hashes.json
Error: while retrieving metadata about ./.cargo/workspace/Cargo.toml:

with /nix/store/f443x0imd2b8r7gb5jkyai9j32y6xhmv-proj-crate2nix/crate-hashes.json containing stuff like this:

{
  "lightgbm-sys 0.3.0 (git+https://github.com/Ten0/lightgbm-rs.git?branch=for_proj_main#ebc5262299d75cc770e21416f9e3ef8204427fba)": "0nr42asw4sg7q7xrkg565ivl1s9b6g75kflzy9hvl3srqrnxf10z"
}

which seems to be old format.

Isn't it generated on the nix side in the case of IFD?

crate2nix/tools.nix

Line 64 in cf03486

echo -n '${builtins.toJSON vendor.extendedHashes}' | jq > "$crate_hashes"

Indeed it does not seem related to #348.

[pacak]

Isn't it generated on the nix side in the case of IFD?

crate2nix/tools.nix

Lines 246 to 249 in cf03486

	rec {
	toPackageId = { name, version, source, ... }:
	"${name} ${version} (${source})";

Looks like it indeed. That's not going to work...

[flokli]

I just ran crate2nix generate --all-features with the more recent rust version (not inside IFD), and crate2nix started to manually add all crate hashes into crate-hashes.json, not just for the ones for which there are no hashes in Cargo.lock - so definitely something still seems to be off.

Note it however still builds for me…

[pacak]

I just ran crate2nix generate --all-features with the more recent rust version (not inside IFD)

Is this using crate2nix after the change, 0.14.0? Anything public or any reproduction? Works for me...

[flokli]

Hmmh, no, indeed I'm still on 0.13.0 for some reason. I'll get this bumped and will report back.

[flokli]

Indeed, this fixed it. Sorry for the noise!