Skip to content

Conversation

@finalyards
Copy link
Contributor

I noticed, while doing #474 that not all corners of the code base got the same testing by CI. This PR is based on the assumptions that:

  • all features are equal; if a source file is built/tested in CI, it should be done so with all (necessary) feature combinations

Note: The above is ideal (not realistic). But it can be seen as something to aim for. In particular, the PR adds test coverage for the security feature.

I would also like to reorder the ci.sh commands, lifting some cargo commands from below cargo-batch section to the tip. The main reason for this is to fail fast if there are any formatting glitches (cargo fmt). I believe many PRs might have them; testing them doesn't require any downloads, so it would be a fast sanity check up-front. This, however, needs an edit in the .github/workflows/ci.yaml, adding (perhaps) the - uses: dtolnay/rust-toolchain@nightly to build. It's already used in binary-size. Without that, cargo is not available until it's been installed by the commands in ci.sh.

Furthermore, what is this cargo batch?

Looking at https://github.com/embassy-rs/cargo-batch, it seems a little home-grown. Would like to see a comment in ci.sh as to what gap it fills that standard cargo couldn't cater to.

I'd really like to run the ci.sh locally, but it turned out too difficult. Thanks to cargo-batch.

@lulf
Copy link
Member

lulf commented Oct 16, 2025

I noticed, while doing #474 that not all corners of the code base got the same testing by CI. This PR is based on the assumptions that:

  • all features are equal; if a source file is built/tested in CI, it should be done so with all (necessary) feature combinations

Note: The above is ideal (not realistic). But it can be seen as something to aim for. In particular, the PR adds test coverage for the security feature.

I would also like to reorder the ci.sh commands, lifting some cargo commands from below cargo-batch section to the tip. The main reason for this is to fail fast if there are any formatting glitches (cargo fmt). I believe many PRs might have them; testing them doesn't require any downloads, so it would be a fast sanity check up-front. This, however, needs an edit in the .github/workflows/ci.yaml, adding (perhaps) the - uses: dtolnay/rust-toolchain@nightly to build. It's already used in binary-size. Without that, cargo is not available until it's been installed by the commands in ci.sh.

Good idea on doing format and clippy up front.

Furthermore, what is this cargo batch?

It allows running parallell builds for multiple target and feature combinations, greatly saving time in CI.

Looking at https://github.com/embassy-rs/cargo-batch, it seems a little home-grown. Would like to see a comment in ci.sh as to what gap it fills that standard cargo couldn't cater to.

It is a bit home grown, but it's used by embassy as well, and the improvements it brings far outweighs the downsides.

I'd really like to run the ci.sh locally, but it turned out too difficult. Thanks to cargo-batch.

It should be automatically downloading it if it can't find it, what issue are you seeing?

@HaoboGu
Copy link
Collaborator

HaoboGu commented Oct 16, 2025

It should be automatically downloading it if it can't find it, what issue are you seeing?

It's linux only? I cannot run it too, cannot execute binary file was reported

@finalyards
Copy link
Contributor Author

finalyards commented Oct 17, 2025

I'll add changes on Monday, most likely:

Good idea on doing format and clippy up front.

Ok. Will change the .github/workflows/ci.yaml to make this possible (as discussed above).

It allows running parallell builds for multiple target and feature combinations, greatly saving time in CI.

Thanks. As long as it has benefits. Would this information have been available anywhere? Didn't find it - cargo batch README could state why it's different from mainline Cargo. But... that's out of focus, here!

It should be automatically downloading it if it can't find it, what issue are you seeing?

It was just taking way, way too long to build it. I work on a Linux VM with 2-3 cores. Waited until progress was "300/1604" or something and chose to quit.

Edit: I saw 300/1640 and thought it's only building cargo-batch. In fact, it's (likely) building all the pieces needed by the parameters.

   Compiling defmt-parser v1.0.0
   Compiling defmt-macros v1.0.1
   Compiling phf v0.11.3
   Compiling doxygen-rs v0.4.2
   Compiling zerocopy-derive v0.8.27
    Building [=>                      ] 149/1640: defmt-macros, zerocopy-derive, cortex-m-rt-macros                                                                                                     

Will let it run, now.

@lulf
Copy link
Member

lulf commented Oct 17, 2025

It should be automatically downloading it if it can't find it, what issue are you seeing?

It's linux only? I cannot run it too, cannot execute binary file was reported

Ah, right! In that case, you can compile and install it like this:

cargo install --git https://github.com/embassy-rs/cargo-batch cargo --bin cargo-batch --locked

@lulf
Copy link
Member

lulf commented Oct 17, 2025

I'll add changes on Monday, most likely:

Good idea on doing format and clippy up front.

Ok. Will change the .github/workflows/ci.yaml to make this possible (as discussed above).

It allows running parallell builds for multiple target and feature combinations, greatly saving time in CI.

Thanks. As long as it has benefits. Would this information have been available anywhere? Didn't find it - cargo batch README could state why it's different from mainline Cargo. But... that's out of focus, here!

It should be automatically downloading it if it can't find it, what issue are you seeing?

It was just taking way, way too long to build it. I work on a Linux VM with 2-3 cores. Waited until progress was "300/1604" or something and chose to quit.

You should be able to use the pre-compiled binary from the cargo-batch release then I think?

@finalyards finalyards force-pushed the ci-changes-1510 branch 2 times, most recently from 130cf45 to 659a184 Compare October 20, 2025 11:32
@finalyards
Copy link
Contributor Author

finalyards commented Oct 20, 2025

My latest addition seems to have blown the bank exceeded the limits of the CI VM:

 = note: /usr/bin/ld: final link failed: No space left on device
          collect2: error: ld returned 1 exit status

Please check the changes and comment them. Moving the fmt and clippy to front was the cause for this; perhaps I did something wrong with .github.


In the mean time, the current CI setup e.g. grows ~/.cargo/git without limits, I think.

@HaoboGu
Copy link
Collaborator

HaoboGu commented Oct 22, 2025

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.

3 participants