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.

Sign up for GitHub

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

Jump to bottom

Catch more compile-time errors in CI #477

Open
11 of 14 tasks
kkysen opened this issue Jun 30, 2022 · 3 comments · May be fixed by #594
Open
11 of 14 tasks

Catch more compile-time errors in CI #477

kkysen opened this issue Jun 30, 2022 · 3 comments · May be fixed by #594
Assignees
@kkysen

Comments

@kkysen
Copy link
Contributor

kkysen commented Jun 30, 2022 •
edited
Loading

Right now in CI, we just test

cargo build
./scripts/test_translator.py tests/

We should test more:

cargo fmt --check
RUSTFLAGS='-D warnings' cargo clippy --tests --all --all-features # includes cargo build
RUSTDOCFLAGS='-D warnings' cargo doc --all-features --document-private-items
cargo test --all-features
./scripts/test_translator.py tests/
# immunant/c2rust-testsuite CI

Note that we currently don't build with -all-features so, for example, the dynamic-instrumentation feature and that whole crate are not built in CI at all.

Furthermore, we should run the external tests, ./scripts/test_translator.py tests/ and immunant/c2rust-testsuite CI inside of cargo test. This not only simplifies testing workflows, but also hooks into external testing, such as crater runs.

I think we should definitely add these at least by the time we move CI to GitHub Actions instead of Azure Pipelines, but preferably earlier, as cargo clippy is almost passing (#474), and once it is, I'd like to keep it that way.

  • cargo fmt --check
  • cargo check --all-features
  • cargo build
  • RUSTFLAGS='-D warnings' cargo check --all-features
  • RUSTFLAGS='-D warnings' cargo build
  • RUSTFLAGS='-D warnings' cargo clippy --tests --all-features (includes cargo check)
  • RUSTDOCFLAGS='-D warnings' cargo doc --all-features --document-private-items --no-deps
  • cargo test
  • ./scripts/test_translator.py tests/
  • cargo test -p c2rust-analyze fails CI due to no FileCheck #593

PRs and Tracking Issues:
@thedataking
Copy link
Contributor

Furthermore, we should run the external tests, ./scripts/test_translator.py tests/ and immunant/c2rust-testsuite CI inside of cargo test.

I think cargo test should not run immunant/c2rust-testsuite since the latter has complex dependencies. Let's keep it separate.

@kkysen
Copy link
Contributor Author

kkysen commented Jul 1, 2022

Okay, I haven't really looked into the details of what immunant/c2rust-testsuite does, so that might make sense. I do think running the in-repo ./scripts/test_translator.py tests/ as part of cargo test makes sense though, as that doesn't have many dependencies, and for ones there are, like the cross tests, we can just skip testing them if they're not available.

@thedataking
Copy link
Contributor

That sounds reasonable. ./scripts/test_translator.py tests/ tests/ has far fewer dependencies, so that should be okay.

@kkysen kkysen mentioned this issue Jul 1, 2022
Merged
8 tasks
@kkysen kkysen self-assigned this Jul 1, 2022
@kkysen kkysen linked a pull request Aug 9, 2022 that will close this issue
Open
@kkysen kkysen mentioned this issue Jan 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kkysen kkysen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Turn on clippy in CI immunant/c2rust
2 participants
@thedataking @kkysen