Add --all-features, cargo fmt --check, cargo check, cargo test, cargo doc, and RUSTFLAGS=-Dwarnings to CI

[kkysen]

Starts to f ix #477.

This adds --all-features/--features dynamic-instrumentation, cargo fmt --check, cargo check, cargo test, cargo doc, and RUSTFLAGS=-Dwarnings to CI. Only cargo clippy is left.

This adds about 5 minutes to CI, going from 10-15 minutes to 15-25 minutes. This is mainly from:

• cargo check --all-features: ~5 minutes
• cargo test: ~3 minutes (mostly pdg snapshot and static analysis tests)

On the other hand, these take practically no time:

• cargo fmt --check: 1 second
• cargo doc --no-deps: 6 seconds

Previous we only had:

• cargo build
• ./scripts/test_translator.py tests/

Now we have added:

• cargo fmt --check
• RUSTFLAGS='-D warnings' cargo check --all-features
• RUSTFLAGS='-D warnings' cargo build
• RUSTFLAGS='-D warnings' cargo test
• RUSTDOCFLAGS='-D warnings' cargo doc --all-features --document-private-items

We still have these to go:

• RUSTFLAGS='-D warnings' cargo clippy --tests --all --all-features (includes cargo check): Fix all clippy warnings #474

For commands that check only and do not run (cargo check, cargo doc), we specify --all-features, which is equivalent to --features llvm-static. But for commands that do build/run (cargo build, cargo test), we don't specify --all-features as we don't want to test --features llvm-static by default, and it doesn't work on the Arch and Fedora docker images (see #500).

Currently, cargo test -p c2rust-analyze is excluded as it fails on some OSes in CI (#593). It will be re-enabled once that's fixed, but I want to get cargo test into CI as soon as possible.

[kkysen]

Fails on Arch and Fedora during cargo test for two similar reasons:

Arch:

     Running unittests (target/release/deps/c2rust_ast_exporter-08a638edb21ed379)
: CommandLine Error: Option 'openmp-ir-builder-optimistic-attributes' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options
error: test failed, to rerun pass '-p c2rust-ast-exporter --lib'

Fedora:

     Running unittests (target/release/deps/c2rust_ast_exporter-08a638edb21ed379)
: CommandLine Error: Option 'help-list' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options
error: test failed, to rerun pass '-p c2rust-ast-exporter --lib'

It seems we're passing multiple of the same options to clang in c2rust-ast-exporter. I'm not sure where or why, though, or why it's only failing on Arch and Fedora. They must use different clang versions, right?

[kkysen]

Neither of the errors are reproducible in local docker builds; that's weird.

[kkysen]

Doesn't appear to be flaky CI either; very odd.

[kkysen]

It appears the same CI errors are happening in Arch in Fedora, but in ./test_translator.py instead of cargo test (which I commented out). I have no idea what's happening. Maybe the CI steps aren't isolated from each other as I thought?

[kkysen]

The errors went away when I removed --all-features from cargo build, so I'm guessing c2rust-instrument links against the wrong/multiple version(s) of LLVM on Arch and Fedora maybe, and that causes the issue. ./test_translator.py doesn't need c2rust-instrument and --all-features, so it should be okay if we can do cargo check --all-features and then only cargo build (with no --all-features) for now, but this is something we should fix.

[kkysen]

Currently, anything that builds with --all-features (either cargo build and subsequent tests or cargo test) and runs those binaries on Arch or Fedora crash. This is due to them linking twice to/linking to the wrong version of LLVM, and I'm not sure what causes this in --feature dynamic-instrumentation/c2rust-instrument. Clearly we should fix that, but in the meantime, I could also split Arch and Fedora to have a different pipeline, as Darwin currently does (and thus can enable --all-features on cargo build and cargo test). Or I could just leave it as is for now until we solve the issue, as we still run cargo check --all-features, so we should catch any compile-time errors; we're just missing cargo test for c2rust-instrument really.

See #500 for more on this.

[kkysen]

This should be good to go now. Only clippy is skipped for now, though that's almost fixed as well (just have a couple tweaks left).

[kkysen]

Once these PRs land, finally closing #474, we can add clippy:

• Fix the clippy::duplicate_underscore_argument warning #497
• Fix the clippy::large_enum_variant warnings that are simple by Boxing them #508
• Allow the last clippy::large_enum_variant warning #509

[kkysen]

The PDG snapshot tests are under cargo test (#505), so we'd need to run that in CI like implemented here to test that now.

[kkysen]

clippy is ready now (#474), so I can add that here (just uncommenting) or put it in a separate PR.

[thedataking]

1. How do the check.sh and pdg.sh scripts relate to the CI changes? I don't see them being referenced in the azure configuration file. If they are not needed for this PR, please remove them.
2. Now that we have substantially more test steps, it would be good to capture the motivation for some of the steps. For instance, it isn't immediately clear to me why it is beneficial to first run cargo check and then cargo build. I couldn't tell what problems the first step would catch that the second wouldn't. (This may be obvious to others but the point still stands.)

[nitpick & digression] pdg.sh is too terse and check.sh too generic IMHO. Can you use names that help new users understand the purpose of these scripts and match the names of other scripts better? Please fix in a separate PR if these were not committed by accident.

[kkysen]

@spernsteiner, CI for cargo test is failing on not finding FileCheck in Ubuntu 18 (but not 20) and Debian 10 (but not 11). It must not be included in LLVM installations by default in older distros (I'm guessing). We'd really like to have cargo test running in CI.

It appears the flaky CI bug may (🤞) be gone now after the major #554 PR that improved instrumentation/test times.

[kkysen]

@spernsteiner, I tried #536 (comment), but apt install llvm-tools doesn't seem to be a thing.

[spernsteiner]

I tried #536 (comment), but apt install llvm-tools doesn't seem to be a thing.

Apparently the package name is e.g. llvm-11-tools. There is no version-independent llvm-tools alias. However, it looks like llvm-dev depends on llvm-$version-dev, which depends on llvm-$version-tools, so maybe you can pull it in that way.

[kkysen]

It seems that for LLVM 6 and earlier, it's llvm-6.0-tools and it's not installed by default with llvm like it is in later versions. I found a fix for that now, but your FileCheck search code isn't finding it since it doesn't look for FileCheck-6.0. I think a better solution is to look for llvm-config in the same way c2rust-ast-exporter does and then use $(llvm-config --bindir)/FileCheck.