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

build_and_test.yml: ensure tests & ./run_clippy.sh pass for build in the Dockerfile #4073

Closed
wants to merge 6 commits into from

Conversation

problame
Copy link
Contributor

This patch adds a third build type to the build-neon build matrix. This build type corresponds to what we do in Dockerfile, i.e., a --release build without --features testing.

The advantage of doing it this way is that we can re-use the steps from the fine-grained & richer build-neon workflow job.

The disadvantage is that we duplicate the build arguments from the Dockerfile.

The alternative is to do it the other way around, i.e., from the Dockerfile, invoke cargo test and ./run_clippy.sh.

However, that would duplicate all the CARGO_FLAGS and CARGO_FEATURES logic that we have in the build-neon workflow job. Which seems worse to me.

…the Dockerfile

This patch adds a third build type to the build-neon build matrix.
This build type corresponds to what we do in Dockerfile, i.e.,
a `--release` build without `--features testing`.

The advantage of doing it this way is that we can re-use the
steps from the fine-grained & richer build-neon workflow job.

The disadvantage is that we duplicate the build arguments from the
Dockerfile.

The alternative is to do it the other way around, i.e.,
from the Dockerfile, invoke `cargo test` and `./run_clippy.sh`.

However, that would duplicate all the CARGO_FLAGS and CARGO_FEATURES
logic that we have in the build-neon workflow job.
Which seems worse to me.
@problame problame requested review from koivunej and zoete April 25, 2023 14:43
@zoete
Copy link
Contributor

zoete commented Apr 25, 2023

Let's do this after #4067

@problame
Copy link
Contributor Author

This build should fail because it doesn't have #4070 .
4070 fixes blunders from #4055

build-neon uses `make postgres-...`.

The Makefile expects BUILD_TYPE to be debug or release.

Before this PR, the build matrix's BUILD_TYPE would be just that.
Now the build matrix's build type is something different than the
Makefile's BUILD_TYPE.

Make the difference explicit.
@problame
Copy link
Contributor Author

@zoete had to push a fixup because the Makefile expects BUILD_TYPE to be either debug or release, but not image.

fix: distinguish build matrix build type from Makefile's BUILD_TYPE
    
    build-neon uses `make postgres-...`.
    
    The Makefile expects BUILD_TYPE to be debug or release.
    
    Before this PR, the build matrix's BUILD_TYPE would be just that.
    Now the build matrix's build type is something different than the
    Makefile's BUILD_TYPE.
    
    Make the difference explicit.

@problame
Copy link
Contributor Author

Sigh, the current state of this PR doesn't solve the clippy situation yet.

There are two problems:

  1. We run ./run_clippy.sh in a separate workflow job (check-codestyle-rust) that doesn't have the CARGO_FLAGS / CARGO_FEATURES variables from the build-neon job.
  2. The ./run_clippy.sh itself doesn't respect any of the CARGO_FLAGS / CARGO_FEATURES variables. It uses --all-features instead.

I was going to suggest using cargo clippy $CARGO_FLAGS $CARGO_FEATURES ... directly instead of ./run_clippy.sh. But then we risk the CI going out of sync with ./run_clippy.sh.

It's probably the right approach, though.

Thoughts?

@github-actions
Copy link

Test results for f42aede:


debug build: 219 tests run: 209 passed, 0 failed, 10 (full report)


release build: 219 tests run: 208 passed, 1 failed, 10 (full report)

Failed tests:


- rename MATRIX_BUILD_TYPE to "variant"
- extract build variables into separate action
- move clippy to separate action, use build vars from the new action
- build-neon: use build vars from the new action
@problame
Copy link
Contributor Author

Yuck, turns out I'd need to propagate the new variant paradigm much further, because the artifact uploads and downloads use matrix.build_type and would need to switch over as well.

This will eat too much time for too little benefit.
Let's close this PR and figure out a more minimal approach :/

@problame problame closed this Apr 25, 2023
problame added a commit that referenced this pull request Apr 25, 2023
This will catch compiler & clippy warnings in all feature combinations.

We should probably use cargo hack for build and test as well, but,
that's quite expensive and would add to overall CI wait times.

obsoletes #4073
refs #4070
problame added a commit that referenced this pull request Apr 27, 2023
This will catch compiler & clippy warnings in all feature combinations.

We should probably use cargo hack for build and test as well, but,
that's quite expensive and would add to overall CI wait times.

obsoletes #4073
refs #4070
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.

None yet

2 participants