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

Minimum Supported Rust Version = 1.77.0 #323

Merged
merged 3 commits into from
Oct 13, 2024
Merged

Conversation

fox0
Copy link
Contributor

@fox0 fox0 commented Oct 13, 2024

@jgarzik
Copy link
Contributor

jgarzik commented Oct 13, 2024

LGTM. @andrewliebenow, any comments?

@andrewliebenow
Copy link
Contributor

Does not actually build on 1.77.0:

❯ git log -1
commit 1faed3d083e229fc520240b82cfdf3e47a07e631 (HEAD, fox0/msrv)
Author: fox0 <15684995+fox0@users.noreply.github.com>
Date:   Sun Oct 13 10:55:36 2024 +0700

    MSRV = 1.77.0

❯ cargo msrv find --include-all-patch-releases --log-target stdout --no-check-feedback --no-user-output | rg --fixed-strings -- 'found minimal-compatible toolchain'
2024-10-13T04:16:26.875219Z  INFO cargo_msrv::sub_command::find: found minimal-compatible toolchain toolchain=1.80.0-x86_64-unknown-linux-gnu

We either need to add the changes that were in my PR, or bump the version to 1.80.0. Your reasoning made sense to me: it probably isn't worth making too many changes just for the purpose of backwards compatibility. I just think it would be good to start checking/tracking MSRV, so that developers don't accidentally increase the minimum Rust version needed without knowing that they're doing that. I'd also suggest that a CI run is added to ensure that the project actually builds with whatever MSRV is advertised in the Cargo.toml files. I'm not sure how to do that.

@fox0 my PR also added a clippy.toml configuration file. This will help developers catch some MSRV violations sooner (ideally before a PR is submitted), because Clippy knows when some APIs were added, etc.

andrewliebenow@680fe57#diff-f3dbfb2563c3490394f44c26b51837018e79a79e75fd31e89b19e943a38317ea

@andrewliebenow
Copy link
Contributor

It looks like using a specific toolchain version might be as simple as adding this step to the GitHub workflow:

- uses: dtolnay/rust-toolchain@1.80.0

See https://github.com/tauri-apps/plugins-workspace/blob/495cbf721f820e8e2b17cad7d0b38503c83572cd/.github/workflows/msrv-check.yml

https://github.com/dtolnay/rust-toolchain

@fox0
Copy link
Contributor Author

fox0 commented Oct 13, 2024

$ cargo +1.77.0 b
   Compiling glob v0.3.1
error[E0716]: temporary value dropped while borrowed
   --> ftw/src/lib.rs:799:42
    |
797 |         let dir_fd = match dir {
    |             ------ borrow later stored here
798 |             HybridDir::Owned(dir) => dir.file_descriptor(),
799 |             HybridDir::Deferred(dir) => &dir.open_file_descriptor(),
    |                                          ^^^^^^^^^^^^^^^^^^^^^^^^^-
    |                                          |                        |
    |                                          |                        temporary value is freed at the end of this statement
    |                                          creates a temporary value which is freed while still in use
    |
    = note: consider using a `let` binding to create a longer lived value

error[E0716]: temporary value dropped while borrowed
   --> ftw/src/lib.rs:826:62
    |
823 |                         let prev_dir = match second_last_index {
    |                             -------- borrow later stored here
...
826 |                                 HybridDir::Deferred(dir) => &dir.open_file_descriptor(),
    |                                                              ^^^^^^^^^^^^^^^^^^^^^^^^^-
    |                                                              |                        |
    |                                                              |                        temporary value is freed at the end of this statement
    |                                                              creates a temporary value which is freed while still in use
    |
    = note: consider using a `let` binding to create a longer lived value

error[E0716]: temporary value dropped while borrowed
   --> ftw/src/lib.rs:964:46
    |
961 |         let prev_dir = match second_last_index {
    |             -------- borrow later stored here
...
964 |                 HybridDir::Deferred(dir) => &dir.open_file_descriptor(),
    |                                              ^^^^^^^^^^^^^^^^^^^^^^^^^-
    |                                              |                        |
    |                                              |                        temporary value is freed at the end of this statement
    |                                              creates a temporary value which is freed while still in use
    |
    = note: consider using a `let` binding to create a longer lived value

For more information about this error, try `rustc --explain E0716`.
error: could not compile `ftw` (lib) due to 3 previous errors
warning: build failed, waiting for other jobs to finish...

@andrewliebenow
Copy link
Contributor

@fox0, that's correct, please see #305 for all changes needed to build on 1.77.0. @jgarzik wasn't interested in losing the Default derives here:

https://github.com/rustcoreutils/posixutils-rs/pull/305/files#diff-d3ee6b1816f74129bed2e897b393939cd0b0c10a35a619e360abd92eea24106b

So I would suggest bumping the MSRV to 1.80.0, and also adding:

- uses: dtolnay/rust-toolchain@1.80.0

to the GitHub workflow.

@fox0
Copy link
Contributor Author

fox0 commented Oct 13, 2024

I am not allowed to make edits to ftw.

@andrewliebenow
Copy link
Contributor

Okay, so my suggestion would be to edit your PR and change the version to 1.80.0.

@jgarzik
Copy link
Contributor

jgarzik commented Oct 13, 2024

Okay, so my suggestion would be to edit your PR and change the version to 1.80.0.

Agree

@jgarzik jgarzik merged commit f9c35ea into rustcoreutils:main Oct 13, 2024
2 checks passed
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.

3 participants