-
Notifications
You must be signed in to change notification settings - Fork 40
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
[xtask] Convert ci_download bash scripts to Rust #5481
Conversation
Example usage:
|
@rcgoodfellow did you say maghemite openapi might not be necessary anymore? |
It's still necessary to download |
CI failing looks like test flake, filed #5498 |
Removed this in the rust implementation in 0cd628c |
The follow-up to remove the bash scripts is in: #5482 |
) | ||
.await?; | ||
let commit_path = | ||
self.versions_dir.join("maghemite_mg_openapi_version"); |
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.
Using the rev value from Cargo.toml is fine here, and we can probably get rid of the maghemite_mg_openapi_version
file.
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.
I agree that we probably should do this, but it's slightly sticky because the flake.nix
file also depends on having this as a rev.
I agree that we should minimize version dups - but if it's okay with you, I'm going to punt this to a different PR, since my goal is "parity with the bash scripts" before changing this download process too much.
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.
The Nix flake could be rewritten to also parse the rev from Cargo.toml
, FWIW, but I would love to have advance warning so I can make sure to actually do that. :)
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.
SGTM
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 given the goal of "parity with the bash scripts" until we remove those.
The additional deps in xtask increase the build time from 30s to 40s on my machine, but I don't think that's enough to warrant this being an external xtask.
As of #5481 , we have a rust replacement with `cargo xtask download`. This replaces the bash scripts with versions that print a deprecation warning and call `cargo xtask download`. --------- Co-authored-by: David Crespo <[email protected]> Co-authored-by: iliana etaoin <[email protected]>
Implements
cargo xtask download
, which provides options to replace the existing scripts:This PR additionally introduces the
cargo xtask download all
option, which attempts to download all artifacts concurrently.Somewhat related to #3939