rustPlatform.importCargoLock: allow duplicated packages with git. #282798

junjihashimoto · 2024-01-22T05:54:17Z

Description of changes

Fix these issues(#183344 and #30742).
This patch does the same thing of cargo-vendor --no-merge-sources.
It puts git sources to 'git-<sha>' directory.

Things done

Add a 👍 reaction to pull requests you find important.

kirillrdy · 2024-01-22T07:21:44Z

greetings @junjihashimoto this should target staging branch due to mass rebuilds

https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#changes-causing-mass-rebuilds

beware of commit pitfall when changing target branch https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#rebasing-between-branches-ie-from-master-to-staging

junjihashimoto · 2024-01-22T07:51:25Z

@kirillrdy Thx! Rebased to staging branch.

pkgs/build-support/rust/import-cargo-lock.nix

siddhantk232 · 2024-02-27T11:43:42Z

LGTM. I am able to use this branch to build a rust binary which I couldn't build with nixpkgs-unstable.

risicle · 2024-04-14T18:57:04Z

Interestingly this doesn't solve my issue trying to built qdrant 1.8.4, which contains two "wal 0.1.2"s with different git revs https://github.com/qdrant/qdrant/blob/v1.8.4/Cargo.lock

poelzi

tested with a sui flake that would not compile otherwise.
LGTM

poelzi · 2024-09-14T01:20:32Z

@junjihashimoto can you rebase ?

junjihashimoto · 2024-09-14T07:24:52Z

Sure!

poelzi · 2024-10-10T23:07:10Z

I had success with the last version but now it breaks while building postgresql.

❯ nix develop .
warning: Git tree '/data/home/poelzi/Projects/sui/sui' is dirty
error: builder for '/nix/store/qq3g9i45ngsmc56a0d9dvq3bvinxv1v7-postgresql-16.4.drv' failed with exit code 2;
       last 10 log lines:
       > ok 214       + oidjoins                                 1020 ms
       > ok 215       - fast_default                              686 ms
       > ok 216       - tablespace                               1526 ms
       > 1..216
       > # 3 of 216 tests failed.
       > # The differences that caused some tests to fail can be viewed in the file "/build/postgresql-16.4/src/test/regress/regression.diffs".
       > # A copy of the test summary that you see above is saved in the file "/build/postgresql-16.4/src/test/regress/regression.out".
       > make[1]: *** [GNUmakefile:118: check] Error 1
       > make[1]: Leaving directory '/build/postgresql-16.4/src/test/regress'
       > make: *** [GNUmakefile:69: check] Error 2
       For full logs, run 'nix log /nix/store/qq3g9i45ngsmc56a0d9dvq3bvinxv1v7-postgresql-16.4.drv'.

Daholli · 2024-10-18T19:09:49Z

Is there something we can help with here? Testing or anything

TomaSajt

There are packages that use two different crates from the same git repo with the same gitSha. I don't remember which packages, but I've seen them.
So, in those cases there would be key duplications in the source definitions. (Which, I think, would cause toml parsing errors)

Edit: I was wrong, sorry!
It looks like I don't even remember how the code worked... even though I wrote it :/

junjihashimoto · 2025-01-27T12:07:54Z

Fixed the merge conflict.

junjihashimoto · 2025-01-27T12:09:47Z

@TomaSajt Please remember the code as I don't think anyone else can review this PR but you.

TomaSajt · 2025-01-27T13:29:04Z

Note: I haven't thoughly reviewed the importCargoLock part yet.

IMO the git sources should be in git/gitShaRev instead of git-gitShaRev.

And maybe when other registries become supported they should also get a registry/registryName subdir

Or is this just how cargo vendor --no-merge-sources works? Haven't checked it.

Though, we have to be careful with moving things, since some packages assume the vendor directory's structure when patching some dependencies.

So maybe, to keep breakages to a minimum, only the git stuff should get their own subdir, and the registry deps should stay in the original base directory, instead of in ./registry.

junjihashimoto · 2025-01-27T16:10:21Z

Or is this just how cargo vendor --no-merge-sources works? Haven't checked it.

Yes.

So maybe, to keep breakages to a minimum, only the git stuff should get their own subdir, and the registry deps should stay in the original base directory, instead of in ./registry.

The original registry structure cannot stay as it does not allow multiple packages with different git-hashes for the same version of the same package.

TomaSajt · 2025-01-27T16:20:35Z

The original registry structure cannot stay as it does not allow multiple packages with different git-hashes for the same version of the same package.

Sorry if I wasn't being clear: I meant the registry-based stuff only. The git based stuff should go to a different place, since that's what's clashing.

(For the record: I looked at the implementation of cargo-vendor --no-merged-sources and it uses git-{hash} and registry-{hash} names for its directories, where {hash} is some random hash calculated from the name of the source. Just mentioning this here, not recommending to do the same.)

junjihashimoto · 2025-01-27T16:34:50Z

The registry-based stuff is registry-{hash} with cargo-vendor --no-merged-sources.
The following URL is a working example:
https://gist.github.com/junjihashimoto/b9fa01ba5983e98a1e21322eca541752#file-cargo-config-toml-L103

In this PR, the path is registry not registry-{hash}, but it doesn't seem to matter whether the base registry has a hash or not.

TomaSajt · 2025-01-27T16:51:00Z

Yes, the name doesn't matter currently, since we only support crates-io.
I think in the future we should have subdirectories, like registry/ or registry-{hash}.

The problem is, that there are packages that edit files inside cargoDepsCopy (search for them and test them out), and they will fail to build, since the crate they're trying to edit has been moved from $cargoDepsCopy/foo-1.0.0 to $cargoDepsCopy/registry/foo-1.0.0

If we don't want to also fix all of these packages in this PR we may not move files inside registry.

I don't know if there are any derivations that try to edit a git-based dependency, but those will 100% be broken by this PR no matter what we do.

TomaSajt · 2025-01-27T17:17:27Z

But, since there are so few of these packages, it shouldn't be too hard to fix them.

If you want to do the fixing yourself, I recommend not hardcoding $cargoDepsCopy/registry/some-package-*, but just using $cargoDepsCopy/*/some-package-*, so they won't depend on the exact naming.

TomaSajt · 2025-01-27T17:31:16Z

pkgs/build-support/rust/import-cargo-lock.nix

+            ln -s "$crate" "$out/$gitdir/$package_name"
+          fi
+        else
+          ln -s "$crate" $out/$(basename "$crate" | cut -c 34-)


When is this branch reached?

As long as the above mkCrate is running, it will never be reached, so it can be deleted.

TomaSajt · 2025-01-27T17:34:56Z

pkgs/build-support/rust/import-cargo-lock.nix

+          # This is to handle the case of referencing a package with a relative path.
+          if [ ! -e "$out/$gitdir/$package_name" ] ; then
+            ln -s "$crate" "$out/$gitdir/$package_name"
+          fi


Could you provide an example when this happens?

Thank you for pointing that out. This might not be necessary.

pkgs/build-support/rust/test/import-cargo-lock/git-dependency-rev-with-hash/default.nix

TomaSajt · 2025-01-27T17:42:21Z

pkgs/build-support/rust/replace-workspace-values.py

@@ -31,34 +31,36 @@ def replace_key(
        local_dep = table[key]
        del local_dep["workspace"]

-        workspace_dep = workspace_manifest[section][key]
-
        if section == "dependencies":


Could you summarize what was fixed in this file?

There may be cases where there is no value for a key such as "dependencies".
I encountered this when building the sui package.

TomaSajt · 2025-01-27T19:08:25Z

By the way: merging into a PR branch is not recommended. You should have rebased on top of the revision you want, instead of merging.

Since this PR doesn't depend on anything that's currently in staging, I think you should rebase onto the common merge base between master and staging.
(FYI the common merge base is currently rev fa9a43a8d9c6bc8672910d7d73da0e512b0df118)

This way it's easier to test the changes, because you don't have to rebuild everything that's in staging.

Here's a way you could do it:

undo merge with git reset HEAD~ --hard
run git rebase fa9a43a8d9c6bc8672910d7d73da0e512b0df118
fix conflict, just like the merge conflict
git add the fixed conflicting files and run git rebase --continue
when you're done and checked that the commits are correct, you can do git push --force

…s' feature

…es' feature

junjihashimoto · 2025-01-27T20:30:46Z

...upport/rust/test/import-cargo-lock/git-dependency-rev-non-workspace-nested-crate/default.nix

@@ -29,6 +30,7 @@ rustPlatform.buildRustPackage {
  buildInputs =
    [
      openssl
+      zlib


It wasn't necessary on Linux, but it seems to be necessary on Darwin.

junjihashimoto · 2025-01-27T20:40:05Z

@TomaSajt
Thank you! The code fixes and the rebase are done.

junjihashimoto · 2025-02-01T02:00:41Z

@TomaSajt Please let me know if there's anything else blocking the approval or if you have any other concerns.

TomaSajt · 2025-02-01T05:30:11Z

We should wait for the mass migration PRs to reach master, first.

After that, the packages that still use $cargoDepsCopy should be fixed manually (maybe in this PR).

junjihashimoto requested review from zowoq, winterqt and figsoda as code owners January 22, 2024 05:54

github-actions bot added the 6.topic: rust label Jan 22, 2024

ofborg bot added 10.rebuild-darwin: 501+ 10.rebuild-darwin: 2501-5000 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ labels Jan 22, 2024

junjihashimoto changed the base branch from master to staging January 22, 2024 07:49

siddhantk232 added a commit to fastn-stack/fastn that referenced this pull request Jan 24, 2024

x86_64-unknow-linux-musl working with NixOS/nixpkgs#282798

d3e0aaf

junjihashimoto commented Jan 30, 2024

View reviewed changes

pkgs/build-support/rust/import-cargo-lock.nix Outdated Show resolved Hide resolved

siddhantk232 mentioned this pull request Feb 3, 2024

use nix to make release builds for windows and linux (musl) fastn-stack/fastn#1745

Merged

junjihashimoto force-pushed the feature/rust-dup branch from b17a692 to 23aa765 Compare February 20, 2024 10:13

TomaSajt mentioned this pull request Apr 22, 2024

Package request: Sui #303467

Open

wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label May 22, 2024

poelzi approved these changes Sep 14, 2024

View reviewed changes

ofborg bot added 10.rebuild-darwin: 5001+ and removed 2.status: merge conflict This PR has merge conflicts with the target branch 10.rebuild-darwin: 2501-5000 labels Sep 14, 2024

poelzi requested a review from kirillrdy October 10, 2024 22:23

getchoo mentioned this pull request Oct 11, 2024

gitbutler: 0.12.16 -> 0.13.2 #347819

Closed

13 tasks

Daholli mentioned this pull request Oct 19, 2024

Duplicate Entry in Cargo file leads to issues building on nix gitbutlerapp/gitbutler#5221

Closed

junjihashimoto force-pushed the feature/rust-dup branch from 0808078 to b3d5702 Compare January 6, 2025 02:42

schradert mentioned this pull request Jan 6, 2025

gauntlet: init at 12 #371558

Draft

13 tasks

emilazy mentioned this pull request Jan 23, 2025

libgit2: 1.8.4 -> 1.9.0 (also pygit2 update) #373096

Merged

14 tasks

gepbird added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 25, 2025

TomaSajt reviewed Jan 27, 2025

View reviewed changes

ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 27, 2025

github-actions bot added 10.rebuild-darwin: 501+ 10.rebuild-linux: 501+ labels Jan 27, 2025

TomaSajt reviewed Jan 27, 2025

View reviewed changes

pkgs/build-support/rust/test/import-cargo-lock/git-dependency-rev-with-hash/default.nix Show resolved Hide resolved

TomaSajt reviewed Jan 27, 2025

View reviewed changes

junjihashimoto added 2 commits January 28, 2025 05:27

rustPlatform.importCargoLock: Support 'cargo-vendor --no-merge-source…

356c02d

…s' feature

rustPlatform.fetchCargoVendor: Support 'cargo-vendor --no-merge-sourc…

60fe186

…es' feature

junjihashimoto force-pushed the feature/rust-dup branch from 2871ba9 to 60fe186 Compare January 27, 2025 20:28

junjihashimoto commented Jan 27, 2025

View reviewed changes

lilyinstarlight mentioned this pull request Jan 31, 2025

Add the system-monitor applet lilyinstarlight/nixos-cosmic#626

Draft

2 tasks

gleachkr mentioned this pull request Feb 4, 2025

Bump Kani Version gleachkr/nix-tools#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustPlatform.importCargoLock: allow duplicated packages with git. #282798

rustPlatform.importCargoLock: allow duplicated packages with git. #282798

junjihashimoto commented Jan 22, 2024 •

edited

Loading

kirillrdy commented Jan 22, 2024

junjihashimoto commented Jan 22, 2024

siddhantk232 commented Feb 27, 2024 •

edited

Loading

risicle commented Apr 14, 2024

poelzi left a comment

poelzi commented Sep 14, 2024

junjihashimoto commented Sep 14, 2024

poelzi commented Oct 10, 2024

Daholli commented Oct 18, 2024

TomaSajt left a comment •

edited

Loading

junjihashimoto commented Jan 27, 2025

junjihashimoto commented Jan 27, 2025

TomaSajt commented Jan 27, 2025 •

edited

Loading

junjihashimoto commented Jan 27, 2025

TomaSajt commented Jan 27, 2025 •

edited

Loading

junjihashimoto commented Jan 27, 2025

TomaSajt commented Jan 27, 2025 •

edited

Loading

TomaSajt commented Jan 27, 2025

TomaSajt Jan 27, 2025

junjihashimoto Jan 27, 2025 •

edited

Loading

TomaSajt Jan 27, 2025

junjihashimoto Jan 27, 2025

TomaSajt Jan 27, 2025

junjihashimoto Jan 27, 2025

TomaSajt commented Jan 27, 2025

junjihashimoto Jan 27, 2025

junjihashimoto commented Jan 27, 2025

junjihashimoto commented Feb 1, 2025

TomaSajt commented Feb 1, 2025

rustPlatform.importCargoLock: allow duplicated packages with git. #282798

Are you sure you want to change the base?

rustPlatform.importCargoLock: allow duplicated packages with git. #282798

Conversation

junjihashimoto commented Jan 22, 2024 • edited Loading

Description of changes

Things done

kirillrdy commented Jan 22, 2024

junjihashimoto commented Jan 22, 2024

siddhantk232 commented Feb 27, 2024 • edited Loading

risicle commented Apr 14, 2024

poelzi left a comment

Choose a reason for hiding this comment

poelzi commented Sep 14, 2024

junjihashimoto commented Sep 14, 2024

poelzi commented Oct 10, 2024

Daholli commented Oct 18, 2024

TomaSajt left a comment • edited Loading

Choose a reason for hiding this comment

junjihashimoto commented Jan 27, 2025

junjihashimoto commented Jan 27, 2025

TomaSajt commented Jan 27, 2025 • edited Loading

junjihashimoto commented Jan 27, 2025

TomaSajt commented Jan 27, 2025 • edited Loading

junjihashimoto commented Jan 27, 2025

TomaSajt commented Jan 27, 2025 • edited Loading

TomaSajt commented Jan 27, 2025

TomaSajt Jan 27, 2025

Choose a reason for hiding this comment

junjihashimoto Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

TomaSajt Jan 27, 2025

Choose a reason for hiding this comment

junjihashimoto Jan 27, 2025

Choose a reason for hiding this comment

TomaSajt Jan 27, 2025

Choose a reason for hiding this comment

junjihashimoto Jan 27, 2025

Choose a reason for hiding this comment

TomaSajt commented Jan 27, 2025

junjihashimoto Jan 27, 2025

Choose a reason for hiding this comment

junjihashimoto commented Jan 27, 2025

junjihashimoto commented Feb 1, 2025

TomaSajt commented Feb 1, 2025

junjihashimoto commented Jan 22, 2024 •

edited

Loading

siddhantk232 commented Feb 27, 2024 •

edited

Loading

TomaSajt left a comment •

edited

Loading

TomaSajt commented Jan 27, 2025 •

edited

Loading

TomaSajt commented Jan 27, 2025 •

edited

Loading

TomaSajt commented Jan 27, 2025 •

edited

Loading

junjihashimoto Jan 27, 2025 •

edited

Loading