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

[xtask] Convert ci_download bash scripts to Rust #5481

Merged
merged 12 commits into from
May 31, 2024
Merged

Conversation

smklein
Copy link
Collaborator

@smklein smklein commented Apr 9, 2024

Implements cargo xtask download, which provides options to replace the existing scripts:

  • ci_download_clickhouse
  • ci_download_cockroachdb
  • ci_download_console
  • ci_download_dendrite_openapi
  • ci_download_dendrite_stub
  • ci_download_maghemite_mgd
  • ci_download_maghemite_openapi
  • ci_download_softnpu_machinery
  • ci_download_thundermuffin
  • ci_download_transceiver_control

This PR additionally introduces the cargo xtask download all option, which attempts to download all artifacts concurrently.

Somewhat related to #3939

@smklein
Copy link
Collaborator Author

smklein commented Apr 9, 2024

Example usage:

 $ cargo xtask download --help
    Finished dev [unoptimized + debuginfo] target(s) in 0.80s
     Running `target/debug/xtask download --help`
Download binaries, OpenAPI specs, and other out-of-repo utilities

Usage: xtask download [OPTIONS] <TARGETS>...

Arguments:
  <TARGETS>...
          The targets to be downloaded. This list is additive

          Possible values:
          - all:                 Download all targets
          - clickhouse:          Clickhouse binary
          - cockroach:           CockroachDB binary
          - console:             Web console assets
          - dendrite-openapi:    Dendrite OpenAPI spec
          - dendrite-stub:       Stub Dendrite binary tarball
          - maghemite-mgd:       Maghemite mgd binary
          - maghemite-openapi:   Maghemite OpenAPI spec
          - softnpu:             SoftNPU, an admin program (scadm) and a pre-compiled P4 program
          - thundermuffin:       Thundermuffin binary, for sending non-stop buffers between devices
          - transceiver-control: Transceiver Control binary

Options:
      --output-dir <OUTPUT_DIR>
          The path to the "out" directory of omicron
          
          [default: out]

      --versions-dir <VERSIONS_DIR>
          The path to the versions and checksums directory
          
          [default: tools]

  -h, --help
          Print help (see a summary with '-h')

@david-crespo
Copy link
Contributor

@rcgoodfellow did you say maghemite openapi might not be necessary anymore?

@rcgoodfellow
Copy link
Contributor

@rcgoodfellow did you say maghemite openapi might not be necessary anymore?

It's still necessary to download ddmd and mgd from maghemite CI. But I do not believe downloading the openapi spec should be needed anymore.

@smklein
Copy link
Collaborator Author

smklein commented Apr 10, 2024

CI failing looks like test flake, filed #5498

@smklein
Copy link
Collaborator Author

smklein commented Apr 10, 2024

@rcgoodfellow did you say maghemite openapi might not be necessary anymore?

It's still necessary to download ddmd and mgd from maghemite CI. But I do not believe downloading the openapi spec should be needed anymore.

Removed this in the rust implementation in 0cd628c

@smklein
Copy link
Collaborator Author

smklein commented Apr 10, 2024

The follow-up to remove the bash scripts is in: #5482

)
.await?;
let commit_path =
self.versions_dir.join("maghemite_mg_openapi_version");
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Member

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. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

Copy link
Contributor

@iliana iliana left a 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.

@smklein smklein merged commit b0dfd53 into main May 31, 2024
20 checks passed
@smklein smklein deleted the xtask_ci_download branch May 31, 2024 15:48
iliana added a commit that referenced this pull request Jun 4, 2024
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants