-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
rustPlatform.importCargoLock: add support for git dependencies that use workspace inheritance #217232
Conversation
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. |
77aeef1
to
7309269
Compare
@@ -0,0 +1,7 @@ | |||
{ replaceWorkspaceValues, runCommand }: |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.)
7309269
to
33488ed
Compare
33488ed
to
ed8fa82
Compare
6b99cde
to
413ca39
Compare
413ca39
to
0cc4830
Compare
021f556
to
ed26384
Compare
There was a problem hiding this 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
Result of 1 package failed to build:
5 packages built:
Result of 3 packages failed to build:
3 packages built:
|
All of those failures are unrelated. Darwin: dependency being missed after the libiconv changes (I presume, haven't looked at history to confirm). |
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 |
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.
ed26384
to
798e26a
Compare
Could you make an estimation when a consumer would be able to make use of this fix inside any nix (pre) release? |
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? |
Unfortunately, the error still persists
This is the relevant feature where we encounter this: centrifuge/centrifuge-chain#1241 |
This fix only applies to - cargoSha256 = "sha256-BkSVd08QksOvyIRO1bVpiOnWf440r+dLR1A3UiND7IM=";
+ cargoLock = {
+ lockFile = ./Cargo.lock;
+ allowBuiltinFetchGit = true;
+ }; |
Thanks for sharing this information with us. Looking forward to the release which supports workspace inheritance in |
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 assimple 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
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)