-
Notifications
You must be signed in to change notification settings - Fork 312
Unify handling of vendored crates in cargo EasyBlock and add support for non-virtual workspaces #3665
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
Unify handling of vendored crates in cargo EasyBlock and add support for non-virtual workspaces #3665
Conversation
|
Test report by @Flamefire Overview of tested easyconfigs (in order)
Build succeeded for 2 out of 2 (2 easyconfigs in total) |
|
@Flamefire please target |
|
Ok I think I got the merge conflict properly resolved. Used the Browser though but looks fine |
|
@Flamefire I changed to target branch in this PR from |
ddb8ddd to
4eef8a4
Compare
9b60a7d to
5e67e77
Compare
5e67e77 to
9188602
Compare
|
Test report by @Flamefire Overview of tested easyconfigs (in order)
Build succeeded for 1 out of 2 (2 easyconfigs in total) |
The Cargo.toml file might contain: ``` [workspace] members = ["components/salsa-macro-rules", "components/salsa-macros"] ``` This means we need to move the 2 subfolders of components and NOT the components folder into the vendor root-directory so they are found. Hence use `basename` for determining the target path which is a no-op when no folder is present.
Seen e.g. for lsp-types in ruff: [[package]] name = "lsp-types" version = "0.95.1" source = "git+https://github.com/astral-sh/lsp-types.git?rev=3512a9f#3512a9f33eadc5402cfab1b8f7340824c8ca1439"
Micket
left a comment
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.
lgtm, lets get a bunch of testing done on this
|
@boegelbot please test @ jsc-zen3 |
|
@boegel: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... Details- notification for comment with ID 3391484785 processed Message to humans: this is just bookkeeping information for me, |
|
Test report by @boegelbot Overview of tested easyconfigs (in order)
Build succeeded for 100 out of 115 (115 easyconfigs in total) |
|
Similar problem across several failing easyconfigs: @Flamefire Can you take a look? |
|
I investigated this and the issue is caused by sources downloaded from crates.io. They keep their original The solution I implemented is to only attempt the "split-up" when any member folder exists assuming when one exist then all have to exist. This involved a bit of cleanup and whitespace change which makes the diff of that commit look larger However for me `modkit-0.4.1-GCCcore-13.3.0.eb fails in the test step due to a concurrency issue which exists already unrelated to this PR, see nanoporetech/modkit#520 Fixes for unrelated failures: |
|
@boegelbot please test @ jsc-zen3 |
|
@boegel: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... Details- notification for comment with ID 3404771062 processed Message to humans: this is just bookkeeping information for me, |
|
Test report by @boegelbot Overview of tested easyconfigs (in order)
Build succeeded for 113 out of 115 (115 easyconfigs in total) |
main Python module was missing/being rebuild
unrelated intermediate failure |
|
Test report by @Flamefire Overview of tested easyconfigs (in order)
Build succeeded for 1 out of 1 (1 easyconfigs in total) |
|
@boegelbot please test @ jsc-zen3 |
|
@boegel: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... Details- notification for comment with ID 3409885625 processed Message to humans: this is just bookkeeping information for me, |
|
Test report by @boegelbot Overview of tested easyconfigs (in order)
Build succeeded for 2 out of 2 (2 easyconfigs in total) |
boegel
left a comment
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.
lgtm
(created using
eb --new-pr)Followup to #3654 implementing the suggested approach from Flamefire#9 (comment)
It basically removes
self.git_vendor_dirand the additional template(s) required for "workspaces" in Git repos to avoid having to duplicate the same logic for "branch" in those too and possibly later for other entries like "tag".As a result all offline preparation code could be moved to a single function.
What needs to be done for vendored sources:
Cargo.tomlfile) we need a.checksums.jsonfileVirtualworkspaces need to be "resolved" to individual packages elsecargoerrors due to a "virtual manifest"A "Virtual manifest" is a
Cargo.tomlfile with a[workspace]section but without a[package]section. That hasmemberswhich are folders containing sub-packages. Each of that needs to be a folder inside what we set asdirectory =in the$CARGO_HOME/config.tomlfile.Previously I extracted git sources to a separate folder, check for workspaces and moved non-workspaces to our regular vendor folder. For workspaces the
directoryentry for the git repository was set to the folder of the workspace. I.e. the one containing the subfolders. This worked a bit by accident: For the folder set bydirectoryCargo will search (and require!) direct subfolders. Hence the "outer Cargo.toml" and everything else was irrelevant.cargo vendordoes that: Omit the outer folder and copy only the subfolders to the vendor folder containing all vendored crates.So that is what I did here:
Cargo.tomlfolder inside that. This catches our TODO case when a source could contain multiple folders with packagesCargo.tomlfolder for a virtual workspace. If found move the whole folder out of our vendor folder and copy only the members backOne downside is an extra copy of crates in virtual workspaces because they might contain symlinks to their parent folder. I've seen that for e.g. the
LICENSEfile. We need to copy the file, not the symlink ascargo vendordoes too.It can get harder: Packages inside a workspace might reference the workspace file.
cargo vendorresolves those too, i.e. replacefoo.workspace = Truebyfoo = <value of foo in workspace Cargo.toml>in the members. We might need to do that too. Doesn't seem to be necessary for what we have now though.Those virtual workspaces only apply to git repositories.
Another approach would be:
subfoo/foo-1.2.3)CONFIG_TOML_SOURCE_GIT_WORKSPACEtemplate$CARGO_HOMEentry, e.g.subfoo/foo-1.2.3otherwise use the parent folder, i.e.subfooCargo.tomlfile but include the 2nd level for the git-vendor folder, i.e.vendor_dir/*/Cargo.toml, git_vendor_dir/*/Cargo.toml, git_vendor_dir/*/*/Cargo.tomlto match the virtual workspacesThis is basically the inverse approach as implemented here: Don't put all (git) sources into the same folder but each in a separate folder.
That has the nice side-effect of avoiding name collisions. Not sure if they are possible at all though.
Edit: Turns out you can have non-virtual workspaces where the parent folder contains a
[[package]].For vendoring purposes all packages need to be subfolders of a (root) vendor folder and contain a
.cargo-checksum.jsonfile.--> Move all members of workspaces out and delete the parent folder if it was only a virtual workspace, but keep if it has a package at the root.