-
Notifications
You must be signed in to change notification settings - Fork 304
Implement merging Cargo.toml file with workspace file
#3988
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
base: develop
Are you sure you want to change the base?
Conversation
Loosely parse everything that could be in a Cargo.toml file
Allow reusing the parsed result.
When there is a line > key = """ the value starts and ends with the delimitter wrongly terminating the value parser. Check that there is anything else first.
'#'-signs can be embedded in strings
Resolve backreferences like `authors.workspace = true`
We use the line number only outside the loop which triggers > Loop control variable 'num' not used within the loop body. If this is intended, start the name with an underscore.
c5ac95a to
50602d7
Compare
|
Test report by @Flamefire Overview of tested easyconfigs (in order)
Build succeeded for 108 out of 115 (total: 14 hours 23 mins 29 secs) (115 easyconfigs in total) |
|
polars-1.28.1-gfbf-2023b.eb all fail from unrelated reasons, like missing dependencies that fail or were just missing or something. SnapATAC2-2.9.0-dev0-20250630-foss-2024a.eb gets this and I'm not sure how; we specify the vendord cargo packags to be home version 0.5.11, so I'm not sure how it's finding 0.5.12. I don't believe this is, or even could be caused by this. I would suspect the python/maturin or something is overriding the CARGO_OFFLINE that we have to protect against this, so I think the easyblock in this PR is fine, at least not introducing any of the issues here. |
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
lots of code here, mostly the lack of a tomllib.
EB supporting really old pythons with no extra packages definitely is coming to bite us quite a bit.
Hence the question above if we want require at least some very basic ones. Even just
Hm, in the report I don't see a mention of unpacking crates. I'd better double check that they are actually used. |
|
Without the Cargo.Lock file and with Removing |
fb80d38 to
18f8c2a
Compare
Cargo.toml file with workspace file
|
I'm unclear on the SnapATAC issue; was that present all along or somehow related to this easyblock change? |
|
I'm done here and working on supporting SnapATAC. The failure is not introduced by this change: However this highlights a larger issue we should make a decision about: When We could: |
Resolve backreferences like
authors.workspace = truehttps://doc.rust-lang.org/cargo/reference/workspaces.html#the-package-table
For ruff-0.14.3 the salsa crate is using the
[workspace.package]section where one can define values that can be inherited by membersSo the members
Cargo.tomlfile contains e.g.For the offline builds we need to copy the members to standalone packages which has the side-effect that no "workspace" exists anymore causing
cargoto fail withChecking
cargo vendoroutput: It replacesversion.workspace = truebyversion = "1.2.3"Hence we need to parse both TOML files, "merge" them to replace those references and write the result to the member
Cargo.tomlFor that I enhanced the minimal TOML parser I implemented for the
"members"key to create adict[str, dict[str, str]]with sections as outer keys, keys as inner keys mapping the 1:1 copy of the value. I.e. I don't parse the value at all besides trying to find the end: TOML values support lists, strings (single & double quotes), multiline strings (triple quotes), bools, numbersThe above example results in Python:
{ 'package': { "name": '"bar"', "version.workspace": 'true', "authors.workspace": 'true', "description.workspace": 'true', "documentation.workspace": 'true', }, 'dependencies': { "leptos": '{ version = "0.6", features = ["csr"] }', }, }This works because we only need to copy the value 1:1 to the member file but doesn't support everything:
Member:
would require merging the mappings, i.e.
dict.update()in Python instead of a simple replacement.But for that we likely need a proper TOML parser:
tomllibtomlpackage on PYPItomlipackage on PYPItomliso we couldimport pip._vendor.tomliSo my idea would be to try importing
tomllib,tomli,import pip._vendor.tomli,tomlin this order if the previous ones failed and error out if none are availableOr we stick with the minimal parser and extend if only necessary: YAGNI