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

Add cross-dev and cross-util commands. #767

Merged
merged 3 commits into from
Jun 8, 2022
Merged

Conversation

Alexhuszagh
Copy link
Contributor

The cross-dev command has the target-info subcommand, which target information (libc, compiler, C++, qemu version) for a given target.

cross-dev target-info
cross-dev target-info arm-unknown-linux-gnueabihf

The cross-util command has the list-images and remove-images subcommands. list-images lists all cross images, including local development files. remove-images removes cross images.

cross-util list-images
cross-util remove-images
cross-util remove-images arm-unknown-linux-gnueabihf

Closes #760.

@Alexhuszagh Alexhuszagh requested a review from a team as a code owner June 8, 2022 06:16
@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Jun 8, 2022

I've separated this into many different commits, since this is a large PR and refactors some logic. Each I think is a relatively independent commit, and some of them (such as the removal of the duplicate cpp tags) is crucial for subsequent steps (serde_yaml parsing the matrix).

This also changes the target-info the use Rust rather than bash on the host machine, so the logic is simpler as was mentioned in comment (although using a bash script inside the container still makes sense).

src/bin/cross-util.rs Show resolved Hide resolved
src/bin/cross-util.rs Outdated Show resolved Hide resolved
src/bin/cross-util.rs Outdated Show resolved Hide resolved
src/bin/cross-util.rs Outdated Show resolved Hide resolved
src/bin/cross-util.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
cross-dev/Cargo.lock Outdated Show resolved Hide resolved
cross-dev/Cargo.toml Outdated Show resolved Hide resolved
cross-dev/Cargo.toml Outdated Show resolved Hide resolved
cross-dev/src/main.rs Outdated Show resolved Hide resolved
@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Jun 8, 2022

Is this better now? I incorporated some of the changes, but thought it would be a bit better to provide a [[bin]] target for cross-dev, since then it can be run easily but disabled by default.

The changes I've made include:

  • Moving cross-dev to /cross-dev/main.rs, and using the main Cargo.toml for it. No more Cargo.lock, no more need to not publish it.
  • Make cross-dev a [[bin]] target.Since we need serde_yaml as a dependency for it, I've changed it so it's only compiled if the dev feature is enabled.
  • engine still uses the same logic. I considered using default_value_t and parse for the clap options, but it was going to be a lot of code and would have probably been less readable than the existing implementation. This is because we'd have to provide a wrapper for which::which, then use default_value_t = cross::get_container_engine().
  • Changed the image output to {{.Repository}}:{{.Tag}} {{.ID}}. This is then parsed to an Image (see below). This is because images may be tagged as ${repository}:<none>, which would cause on error on removing the image. Removing it by ID works in this case.
  • I've added the --execute flag, so it's a dry-run by default.
  • I've removed the unnecessary Matrix fields.
  • I've changed get_container_engine to take an Option<&str>.
  • rustembedded_target no longer returns an option.
  • No longer use a custom readme in Cargo.toml. lib.rs still warns about this for crates.io.
#[derive(Debug, PartialOrd, Ord, PartialEq, Eq)]
struct Image {
    repository: String,
    tag: String,
    // need to remove images by ID, not just tag
    id: String,
}

If all is good, I'll squash the commits to remove some unnecessary ones.

@@ -249,6 +249,10 @@ jobs:
if: matrix.deploy
run: cargo install --path . --force

- name: Install cross-dev
if: matrix.deploy
run: cargo install --path cross-dev --force
Copy link
Member

@Emilgardis Emilgardis Jun 8, 2022

Choose a reason for hiding this comment

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

Suggested change
run: cargo install --path cross-dev --force
run: cargo install --bin cross-dev --path . --force --features dev

Copy link
Member

Choose a reason for hiding this comment

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

actually, this is wrong, we should just make sure to always check if it builds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So remove matrix.deploy, and then just add cargo build --features=dev?

Copy link
Member

Choose a reason for hiding this comment

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

specifying --bin cross-dev should be done too, to avoid unsync between needed features and the ci stage

docs/crates_io.md Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/bin/cross-util.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Add `list-images` and `remove-images` subcommands. `list-images` lists
all cross images, including local development files. `remove-images`
removes cross images.

```bash
cross-util list-images
cross-util remove-images
cross-util remove-images arm-unknown-linux-gnueabihf
```

`remove-images` also accepts the following flags:
- `-f`, `--force`: force removal of images.
- `-l`, `--local`: also remove local development images.
- `-e`, `--execute`: remove the images, rather than a dry run.

Both commands accept the additional flags:
- `-v`, `--verbose`: print verbose diagnostic output.
- `--engine`: specify the container engine to use.
Add `target-info` subcommands. This provides the target info for the
provided targets, or all targets if none are provided.

```bash
cross-dev target-info
cross-dev target-info arm-unknown-linux-gnueabihf
```

`target-info` also accepts the following flags:
- `-v`, `--verbose`: print verbose diagnostic output.
- `--engine`: specify the container engine to use.
- `--registry`: the registry for the image.
- `--repository`: the repository for the image.
- `--tag`: the tag for the image.

Other changes:
- Updated the CI to test building `cross-dev`.
- Add target info support for Dragonfly.
@Alexhuszagh
Copy link
Contributor Author

I think that's everything. If anything else needs to be patched, let me know, then I'll squash the commits since we have a few... extras.

I've:

  • Removed the unnecessary badges.
  • Squashed the commits into 3 logical commits.
  • Added a build step for cross-dev which runs on every CI run.
  • Inlined the documentation and removed crates_io.md.

Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot added a commit that referenced this pull request Jun 8, 2022
767: Add cross-dev and cross-util commands. r=Emilgardis a=Alexhuszagh

The cross-dev command has the `target-info` subcommand, which target information (libc, compiler, C++, qemu version) for a given target.

```bash
cross-dev target-info
cross-dev target-info arm-unknown-linux-gnueabihf
```

The cross-util command has the `list-images` and `remove-images` subcommands. `list-images` lists all cross images, including local development files. `remove-images` removes cross images.

```bash
cross-util list-images
cross-util remove-images
cross-util remove-images arm-unknown-linux-gnueabihf
```

Closes #760.

Co-authored-by: Alex Huszagh <[email protected]>
@Emilgardis
Copy link
Member

bors r-

oh yeah you wanted to squash?

@bors
Copy link
Contributor

bors bot commented Jun 8, 2022

Canceled.

@Alexhuszagh
Copy link
Contributor Author

bors r-

oh yeah you wanted to squash?

Already did.

bors r=Emilgardis

@bors
Copy link
Contributor

bors bot commented Jun 8, 2022

Build succeeded:

@bors bors bot merged commit 0b3abed into cross-rs:main Jun 8, 2022
@Emilgardis Emilgardis added this to the v0.2.2 milestone Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add subcommand to cleanup images?
2 participants