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

Update cargo regress tool and move CI #905

Merged
merged 1 commit into from
Feb 7, 2025

Conversation

robamu
Copy link
Contributor

@robamu robamu commented Feb 3, 2025

  • CI now uses the svd2rust-regress tool instead of a bash script
  • Fixed a bug where the workspace TOML file was still updated and changed
    when running the regression tool
  • Upgrade portable-atomic version for CI checks to 1.
  • Introduce MSRV v1.82.0 for the regress tool.
  • Allow passing multiple passthrough options to the regress tool.
  • Allow passing options using the test YAML file using the opts key
  • Allow specifying a suffix for test cases inside the YAML file using the suffix key.
    This can be used to resolve name duplications.
  • Allow skipping the cargo check for test cases with a skip_check key
    inside the YAML file

@robamu robamu requested a review from a team as a code owner February 3, 2025 16:40
@robamu robamu marked this pull request as draft February 3, 2025 16:40
@robamu robamu force-pushed the move-ci-to-regress-tool branch from 9d2df55 to 14da61b Compare February 3, 2025 16:46
@robamu
Copy link
Contributor Author

robamu commented Feb 3, 2025

I don't know if I can enable CI for my own fork, that would be useful for testing..

@burrbull
Copy link
Member

burrbull commented Feb 3, 2025

cc @Emilgardis

@robamu robamu force-pushed the move-ci-to-regress-tool branch from 1ea7ef0 to 7922184 Compare February 3, 2025 23:06
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
ci/svd2rust-regress/src/github.rs Outdated Show resolved Hide resolved
src/util.rs Outdated Show resolved Hide resolved
matrix:
# Options are all, none, strict and const
include:
Copy link
Member

Choose a reason for hiding this comment

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

we could use a

outputs:
    matrix: ${{ steps.generate-matrix.outputs.matrix }}

job

which would convert the matrix in this file to

strategy:
  fail-fast: false
    include: ${{ fromJson(needs.generate-matrix.outputs.matrix) }}

however, that wouldn't test different command flags. So maybe we shouldn't do that and instead just have a single job to test all defined test targets in svd2rust-regress

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would the input specification via a different job help? Wouldn´t one command mean that there are no individual CI jobs for the architecture and flag options anymore? I think this is a good thing in the current setup.. I tried to preserve the old setup here.

Copy link
Member

@Emilgardis Emilgardis Feb 4, 2025

Choose a reason for hiding this comment

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

I do this in cross-rs, where I have a targets.toml that populate the CI automatically through a ci job to filter targets.

The input basically expands to a list of targets, not just a single target in a list

Copy link
Contributor Author

@robamu robamu Feb 5, 2025

Choose a reason for hiding this comment

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

This would probably be useful to make running the tests as they are done in the CI locally, even though it makes the CI chain a bit more complex.. But it might be better for a different PR?

ci/svd2rust-regress/src/svd_test.rs Outdated Show resolved Hide resolved
@robamu robamu force-pushed the move-ci-to-regress-tool branch from 799ac32 to 3564036 Compare February 4, 2025 08:56
- CI now uses the svd2rust-regress tool instead of a bash script
- Fixed a bug where the workspace TOML file was still updated and changed
  when running the regression tool
- Upgrade `portable-atomic` version for CI checks to 1.
- Introduce MSRV v1.82.0 for the regress tool.
- Allow passing multiple passthrough options to the regress tool.
- Allow passing options using the test YAML file using the `opts` key
- Allow specifying a suffix for test cases inside the YAML file using the `suffix` key.
  This can be used to resolve name duplications.
- Allow skipping the `cargo check` for test cases with a `skip_check` key
  inside the YAML file
@robamu robamu force-pushed the move-ci-to-regress-tool branch from 8fa38dd to 17c74f3 Compare February 4, 2025 10:42
@robamu robamu marked this pull request as ready for review February 4, 2025 10:42
@robamu
Copy link
Contributor Author

robamu commented Feb 4, 2025

I think this is getting to a good state now. The only problematic thing is that I had to disable the checks for MSP430. When I try to do a cargo check (locally, but same problem in CI) here, I get errors that some external C function specified in some dependency is not valid for the target.. I don´t know why this wasn´t an issue with the previous CI. It might work when using the correct target, but that required a few steps like building the core library for the MPS target and also installing a GCC cross-compiler for MSP430.

@burrbull I did not update the changelog, because this is only related to internal tooling. Should I still add a changelog entry?

@Emilgardis Emilgardis added the no changelog no-changelog label Feb 4, 2025
@Emilgardis
Copy link
Member

no changelog is needed for this

@robamu
Copy link
Contributor Author

robamu commented Feb 4, 2025

Fixes #689

@Emilgardis Emilgardis linked an issue Feb 4, 2025 that may be closed by this pull request
@burrbull burrbull dismissed Emilgardis’s stale review February 7, 2025 09:31

Let's merge this now. If there is more issues we can fix them in another PR

@burrbull burrbull added this pull request to the merge queue Feb 7, 2025
Merged via the queue into rust-embedded:master with commit 93918f0 Feb 7, 2025
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog no-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Why doesn't the CI use svd2rust-regress?
3 participants