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 git dependencies that use workspace inheritance #217232

Merged

Conversation

winterqt
Copy link
Member

@winterqt winterqt commented Feb 19, 2023

Description of changes

Rust 1.64.0 added support for workspace inheritance, which allows
for crates to inherit values such as dependency version constraints or
package metadata information from their workspaces 0.

This works by having workspace members specify a value as a table, with
workspace set to true. Thus, supporting this in importCargoLock is as
simple as walking the crate's Cargo.toml, replacing inherited values
with their workspace counterpart.

This is also what a forthcoming Cargo release will do for cargo vendor 1,
but we can get ahead of it ;)

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 Author

I wanted to include bumping Ruff here, but ran into astral-sh/ruff#3039. If it gets fixed before this is merged, I'll add it.

@winterqt winterqt force-pushed the import-cargo-lock-git-dep-workspace-inheritance branch 3 times, most recently from 77aeef1 to 7309269 Compare February 19, 2023 22:03
@ofborg ofborg bot requested a review from pstn February 19, 2023 22:28
@@ -0,0 +1,7 @@
{ replaceWorkspaceValues, runCommand }:
Copy link
Member

Choose a reason for hiding this comment

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

instead of testing the behavior of replaceWorskpaceValues, what if we just test cargoLock.lockFile directly?

[dependencies.rustpython-compiler-core]
package = "rustpython-compiler-core"
git = "https://github.com/RustPython/RustPython"

Copy link
Member Author

@winterqt winterqt Feb 19, 2023

Choose a reason for hiding this comment

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

We could do that, but I also kind of prefer the simplistic nature of the current test, as it's tiny + adequately tests what the tool does.

Either works -- let's wait for others to weigh in. (Though if we do decide to go with the current approach, I should add a case for target.*.*, as well as maybe the other dependency types, just to be complete.)

@winterqt winterqt force-pushed the import-cargo-lock-git-dep-workspace-inheritance branch from 7309269 to 33488ed Compare February 20, 2023 02:30
@winterqt winterqt force-pushed the import-cargo-lock-git-dep-workspace-inheritance branch from 33488ed to ed8fa82 Compare February 20, 2023 03:53
@winterqt winterqt force-pushed the import-cargo-lock-git-dep-workspace-inheritance branch 2 times, most recently from 6b99cde to 413ca39 Compare February 20, 2023 04:59
@ofborg ofborg bot requested a review from figsoda February 20, 2023 05:13
@winterqt winterqt force-pushed the import-cargo-lock-git-dep-workspace-inheritance branch from 413ca39 to 0cc4830 Compare February 20, 2023 22:25
@ofborg ofborg bot requested a review from figsoda February 20, 2023 23:12
@winterqt winterqt force-pushed the import-cargo-lock-git-dep-workspace-inheritance branch 2 times, most recently from 021f556 to ed26384 Compare February 20, 2023 23:35
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.

Can't speak much about the changes to matrix-conduit, would appreciate a review from one of the maintainers.

The importCargoLock and ruff changes lgtm, would prefer if tests are done using buildRustPackage instead but this is up for discussion

@figsoda
Copy link
Member

figsoda commented Feb 21, 2023

Result of nixpkgs-review pr 217232 run on x86_64-linux 1

1 package failed to build:
  • flare-floss
5 packages built:
  • matrix-conduit
  • python310Packages.python-flirt
  • python311Packages.python-flirt
  • ruff
  • sapling

Result of nixpkgs-review pr 217232 run on aarch64-darwin 1

3 packages failed to build:
  • flare-floss
  • python310Packages.python-flirt
  • python311Packages.python-flirt
3 packages built:
  • matrix-conduit
  • ruff
  • sapling

@winterqt
Copy link
Member Author

All of those failures are unrelated.

Darwin: dependency being missed after the libiconv changes (I presume, haven't looked at history to confirm).
Linux (and I presume Darwin, after fixing the above): dependency version mismatch?

@figsoda
Copy link
Member

figsoda commented Feb 21, 2023

Yep I was just posting this so other people don't have to rebuild these, and I think you were right about the dependency version mismatch

@winterqt
Copy link
Member Author

Fixing these now...

…se workspace inheritance

Rust 1.64.0 added support for workspace inheritance, which allows
for crates to inherit values such as dependency version constraints or
package metadata information from their workspaces [0].

This works by having workspace members specify a value as a table, with
`workspace` set to true. Thus, supporting this in importCargoLock is as
simple as walking the crate's Cargo.toml, replacing inherited values
with their workspace counterpart.

This is also what a forthcoming Cargo release will do for `cargo vendor` [1],
but we can get ahead of it ;)

[0]: https://blog.rust-lang.org/2022/09/22/Rust-1.64.0.html#cargo-improvements-workspace-inheritance-and-multi-target-builds
[1]: rust-lang/cargo#11414
As `cargo vendor` currently doesn't support workspace inherited manifest
values, a patch that replaced the values manually was used as a workaround.

Now that importCargoLock has support for this, and we're probably going
to move towards it anyways, let's switch to it now, as there's a clear
benefit either way.

This also switches to using our copy of SQLite instead of the vendored
one included in `libsqlite3-sys` -- because it's preferred, but also
because it causes the build to fail, now that dependencies are read-only.
@winterqt winterqt force-pushed the import-cargo-lock-git-dep-workspace-inheritance branch from ed26384 to 798e26a Compare March 5, 2023 02:49
@ofborg ofborg bot requested a review from figsoda March 5, 2023 03:04
@yu-re-ka yu-re-ka merged commit ffd48ee into NixOS:master Mar 18, 2023
@wischli
Copy link

wischli commented Mar 29, 2023

Could you make an estimation when a consumer would be able to make use of this fix inside any nix (pre) release?

@figsoda
Copy link
Member

figsoda commented Mar 29, 2023

This is already in master and the unstable branches, if you are using stable, then 23.05

@wischli
Copy link

wischli commented Mar 29, 2023

This is already in master and the unstable branches, if you are using stable, then 23.05

Thanks for the quick reply. Any chance it will be backported to 22.11?

@figsoda
Copy link
Member

figsoda commented Mar 29, 2023

@wischli can you test #223804?

@wischli
Copy link

wischli commented Mar 31, 2023

@wischli can you test #223804?

Unfortunately, the error still persists

       > Caused by:
       >   error inheriting `version` from workspace root manifest's `workspace.package.version`

This is the relevant feature where we encounter this: centrifuge/centrifuge-chain#1241
One of our dependencies (Polkadot) switched to workspace inheritance.

@figsoda
Copy link
Member

figsoda commented Mar 31, 2023

This fix only applies to importCargoLock, the fix for fetchCargoTarball will come with the rust 1.68 update, which will not be backported to 22.11

-             cargoSha256 = "sha256-BkSVd08QksOvyIRO1bVpiOnWf440r+dLR1A3UiND7IM=";
+             cargoLock = {
+               lockFile = ./Cargo.lock;
+               allowBuiltinFetchGit = true;
+             };

@wischli
Copy link

wischli commented Apr 11, 2023

Thanks for sharing this information with us. Looking forward to the release which supports workspace inheritance in fetchCargoTarball.

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