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

Improve MSRV CI check to print out problems to log #11789

Merged
merged 5 commits into from
Aug 5, 2024

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Aug 2, 2024

Which issue does this PR close?

Closes #11788

Rationale for this change

When the MSRV CI check fails it does not say what is wrong or offer specific advice on how to fix it. For example:

CI failure: https://github.com/apache/datafusion/actions/runs/10204347289/job/28280296775?pr=11627

Output error:

Run # If you encounter an error with any of the commands below
Fetching index
Verifying the Minimum Supported Rust Version (MSRV) for toolchain x86_64-unknown-linux-gnu
Using check command cargo check
   Failed check command cargo check didn't succeed
Crate source was found to be incompatible with its MSRV '1.76.0', as defined in '/__w/datafusion/datafusion/datafusion/core/Cargo.toml'
Error: Process completed with exit code 1.

What changes are included in this PR?

  1. Add --logfile stdout so errors are printed to the log
  2. Update comments in CI check explaining how to fix.

Here is an example failure with this change: https://github.com/apache/datafusion/actions/runs/10220955214/job/28282400260?pr=11789

While the log is not pretty 🤮 it at least provides enough information to debug:

(/__w/datafusion/datafusion/datafusion/sql)\n Checking datafusion-functions v40.0.0 (/__w/datafusion/datafusion/datafusion/functions)\nerror[E0716]: temporary value dropped while borrowed\n --> datafusion/physical-expr-common/src/aggregate/groups_accumulator/prim_op.rs:167:18\n |\n151 | let values = match opt_filter {\n | ------ borrow later stored here\n...\n167 | &PrimitiveArray::::new(values_buf, nulls_buf).with_data_type(dt)\n | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use\n168 | }\n | - temporary value is freed at the end of this statement\n |\n = note: consider using a let binding to create a longer lived value\n\nFor more information about this error, try rustc --explain E0716.\nerror: could not compile datafusion-physical-expr-common (lib) due to 1 previous error\nwarning: build failed, waiting for other jobs to finish...\n" cmd="1.76.0-x86_64-unknown-linux-gnu cargo check"

Are these changes tested?

Yes (in progress)

Are there any user-facing changes?

@github-actions github-actions bot added documentation Improvements or additions to documentation development-process Related to development process of DataFusion logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) labels Aug 2, 2024
@alamb alamb changed the title Improve MSRV CI check to print out problems to log @alamb Improve MSRV CI check to print out problems to log Aug 2, 2024
@alamb alamb marked this pull request as ready for review August 2, 2024 19:29
@github-actions github-actions bot removed documentation Improvements or additions to documentation logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) labels Aug 2, 2024
Copy link
Contributor

@jayzhan211 jayzhan211 left a comment

Choose a reason for hiding this comment

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

Great, I usually just binary search the failed version and run against it to get the compile error message, print out the log should be more helpful

@alamb
Copy link
Contributor Author

alamb commented Aug 3, 2024

Great, I usually just binary search the failed version and run against it to get the compile error message, print out the log should be more helpful

😆 that is a good idea. I should have thought of that.

I'll also add that as a comment to help others (and my future self)

- name: Check datafusion-substrait
working-directory: datafusion/substrait
run: cargo msrv verify
run: cargo msrv --verify --log-target stdout
Copy link
Contributor

Choose a reason for hiding this comment

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

--verify flag has deprecation note in cargo msrv --help, so maybe it's better to continue using verify subcommand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you -- this is a good tip. I added some more logging / output based on the docs in https://gribnau.dev/cargo-msrv/migration-guide/v0.15_v0.16_json.html

This command seems to add even more output

cargo msrv --output-format json --log-target stdout verify

.github/workflows/rust.yml Outdated Show resolved Hide resolved
Co-authored-by: Eduard Karacharov <[email protected]>
@alamb alamb merged commit 336c15e into apache:main Aug 5, 2024
24 checks passed
@alamb
Copy link
Contributor Author

alamb commented Aug 5, 2024

Thanks again @korowa and @jayzhan211

@alamb alamb deleted the alamb/useful_msrv_check branch August 5, 2024 10:51
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development-process Related to development process of DataFusion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI cargo msrv check fails silently
3 participants