Skip to content

Does not compile with MSRV #1808

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

Open
bryceberger opened this issue Jan 25, 2025 · 8 comments
Open

Does not compile with MSRV #1808

bryceberger opened this issue Jan 25, 2025 · 8 comments
Labels
help wanted Extra attention is needed

Comments

@bryceberger
Copy link
Contributor

Currently, the MSRV is:

  • 1.70 according to gix/Cargo.toml
  • 1.74 (nightly!) according to .github/workflows/msrv.yml
  • 1.76 according to a comment in gix-commitgraph/CHANGELOG.md stating:

    Our MSRV follows the one of helix, which in turn follows Firefox

  • 1.84 for all crates that are not dependents of gix (list?), according to STABILITY.md

However, gix can only build on 1.77+, due to c-string literals, used in rusqlite/rusqlite#1483. The CI passes because it only runs cargo check, which does work on 1.74.

Additionally, tests only build on 1.79+, due to rust-lang/rust#121346.

gix-macros/tests/momo/ux/error_if_ineffective.rs fails on nightly versions. This appears to be due to differences in macro source location tracking on nightly versions.

@Byron
Copy link
Member

Byron commented Jan 26, 2025

Thanks for bringing this to my attention!

Maybe we can start to untangle this by making the MSRV CI step perform a build instead, and use stable?

Then it seems that STABILITY.md could be changed to be less ambiguous and clarify possible differences between build and test.

Regarding gix/Cargo.toml - that can probably just be put to 1.74 like the rest of the crates.

@EliahKagan
Copy link
Member

EliahKagan commented Feb 22, 2025

Regarding gix/Cargo.toml - that can probably just be put to 1.74 like the rest of the crates.

Is this to say that all crates whose Cargo.toml files list lower Rust versions than 1.74 can and should already have them updated to 1.74? Currently, not all crates have a rust-version key in their Cargo.toml file, but of the 76 crates that do, its value is "1.70" for 75 of them.

Specifically, at the current tip of main (8776a3e), I ran:

diff -U1000 <(git grep -Fn 'rust-version = "1.70"' -- '**/Cargo.toml') <(git grep -Fn 'rust-version = "' -- '**/Cargo.toml')

The output was:

--- /dev/fd/63  2025-02-22 16:35:26.292251700 -0500
+++ /dev/fd/62  2025-02-22 16:35:26.292251700 -0500
@@ -1,75 +1,76 @@
 gix-actor/Cargo.toml:12:rust-version = "1.70"
 gix-archive/Cargo.toml:11:rust-version = "1.70"
 gix-attributes/Cargo.toml:12:rust-version = "1.70"
 gix-bitmap/Cargo.toml:11:rust-version = "1.70"
 gix-blame/Cargo.toml:11:rust-version = "1.70"
 gix-chunk/Cargo.toml:13:rust-version = "1.70"
 gix-command/Cargo.toml:11:rust-version = "1.70"
 gix-commitgraph/Cargo.toml:13:rust-version = "1.70"
 gix-config-value/Cargo.toml:11:rust-version = "1.70"
 gix-config/Cargo.toml:14:rust-version = "1.70"
 gix-config/tests/Cargo.toml:11:rust-version = "1.70"
 gix-credentials/Cargo.toml:11:rust-version = "1.70"
 gix-date/Cargo.toml:12:rust-version = "1.70"
 gix-diff/Cargo.toml:12:rust-version = "1.70"
 gix-diff/tests/Cargo.toml:12:rust-version = "1.70"
 gix-dir/Cargo.toml:11:rust-version = "1.70"
 gix-discover/Cargo.toml:12:rust-version = "1.70"
 gix-features/Cargo.toml:11:rust-version = "1.70"
 gix-fetchhead/Cargo.toml:11:rust-version = "1.70"
 gix-filter/Cargo.toml:11:rust-version = "1.70"
 gix-fs/Cargo.toml:11:rust-version = "1.70"
 gix-fsck/Cargo.toml:12:rust-version = "1.70"
 gix-glob/Cargo.toml:11:rust-version = "1.70"
 gix-hash/Cargo.toml:12:rust-version = "1.70"
 gix-hashtable/Cargo.toml:12:rust-version = "1.70"
 gix-ignore/Cargo.toml:12:rust-version = "1.70"
 gix-index/Cargo.toml:12:rust-version = "1.70"
 gix-index/tests/Cargo.toml:12:rust-version = "1.70"
 gix-lfs/Cargo.toml:11:rust-version = "1.70"
 gix-lock/Cargo.toml:12:rust-version = "1.70"
 gix-macros/Cargo.toml:16:rust-version = "1.70"
 gix-mailmap/Cargo.toml:11:rust-version = "1.70"
 gix-merge/Cargo.toml:9:rust-version = "1.70"
 gix-negotiate/Cargo.toml:11:rust-version = "1.70"
 gix-note/Cargo.toml:11:rust-version = "1.70"
 gix-object/Cargo.toml:12:rust-version = "1.70"
 gix-odb/Cargo.toml:12:rust-version = "1.70"
 gix-odb/tests/Cargo.toml:11:rust-version = "1.70"
 gix-pack/Cargo.toml:12:rust-version = "1.70"
 gix-pack/tests/Cargo.toml:11:rust-version = "1.70"
 gix-packetline-blocking/Cargo.toml:12:rust-version = "1.70"
 gix-packetline/Cargo.toml:12:rust-version = "1.70"
 gix-path/Cargo.toml:12:rust-version = "1.70"
 gix-pathspec/Cargo.toml:11:rust-version = "1.70"
 gix-prompt/Cargo.toml:12:rust-version = "1.70"
 gix-protocol/Cargo.toml:12:rust-version = "1.70"
 gix-quote/Cargo.toml:11:rust-version = "1.70"
 gix-rebase/Cargo.toml:11:rust-version = "1.70"
 gix-ref/Cargo.toml:12:rust-version = "1.70"
 gix-ref/tests/Cargo.toml:12:rust-version = "1.70"
 gix-refspec/Cargo.toml:12:rust-version = "1.70"
 gix-revision/Cargo.toml:12:rust-version = "1.70"
 gix-revwalk/Cargo.toml:12:rust-version = "1.70"
 gix-sec/Cargo.toml:12:rust-version = "1.70"
 gix-sequencer/Cargo.toml:11:rust-version = "1.70"
 gix-shallow/Cargo.toml:12:rust-version = "1.70"
 gix-status/Cargo.toml:12:rust-version = "1.70"
 gix-status/tests/Cargo.toml:12:rust-version = "1.70"
 gix-submodule/Cargo.toml:11:rust-version = "1.70"
 gix-tempfile/Cargo.toml:12:rust-version = "1.70"
 gix-tix/Cargo.toml:11:rust-version = "1.70"
 gix-trace/Cargo.toml:11:rust-version = "1.70"
 gix-transport/Cargo.toml:12:rust-version = "1.70"
 gix-traverse/Cargo.toml:12:rust-version = "1.70"
 gix-traverse/tests/Cargo.toml:11:rust-version = "1.70"
 gix-tui/Cargo.toml:11:rust-version = "1.70"
 gix-url/Cargo.toml:12:rust-version = "1.70"
 gix-utils/Cargo.toml:11:rust-version = "1.70"
 gix-validate/Cargo.toml:12:rust-version = "1.70"
 gix-worktree-state/Cargo.toml:12:rust-version = "1.70"
 gix-worktree-state/tests/Cargo.toml:12:rust-version = "1.70"
 gix-worktree-stream/Cargo.toml:11:rust-version = "1.70"
 gix-worktree/Cargo.toml:12:rust-version = "1.70"
 gix-worktree/tests/Cargo.toml:11:rust-version = "1.70"
 gix/Cargo.toml:12:rust-version = "1.70"
+tests/tools/Cargo.toml:11:rust-version = "1.76"

I would find it convenient to be able to use Rust 1.76 in more places, such as in tests on a feature branch that are more complex without OsString::into_encoded_bytes. However, that is only my motivation for inquiring about what the MSRV should be; I do not advocate using a higher MSRV to support those tests, which may or may not look this in their final form anyway.

@Byron
Copy link
Member

Byron commented Feb 23, 2025

That's a good point! I think usually when considering MSRV updates, I change all to the highest-possible MSRV and run clippy. If it doesn't find anything, I keep the previous MSRV so the version upgrade isn't happening without need. In theory, if there was a tool to test the MSRV (and I think there is), one could run it on all crates to find the lowest possible MSRV for each of the crates. That way, each of them, if used individually, might even support a lower MSRV than we test for, making them more compatible.

And since helix still uses 1.74, that's to current maximum I am afraid.

And just to state that explicitly, if it was just me, I'd use the latest stable, always, MSRV is something I don't want to think about, yet there is no way around it if one wants these crates to be as useful as possible.

@EliahKagan
Copy link
Member

Sorry--I accidentally wrote 1.76 here as the version I hoped to use, but what I meant to write here was 1.74:

I would find it convenient to be able to use Rust 1.76 in more places

1.74 is the version I hoped to use, since OsString::into_encoded_bytes is available from 1.74 and on.

@Byron
Copy link
Member

Byron commented Feb 23, 2025

Great! Then there should be nothing in the way of using it, upping the rust-version in the crate as needed.

@EliahKagan
Copy link
Member

EliahKagan commented Feb 23, 2025

Sounds good. If it ends up being useful to keep tests that (roughly speaking) look inside an OsString, then I'll undo that change to use into_encoded_bytes again and increase the rust-version on gix-command. Assuming that happens, I think it will also be necessary for me to increase the rust-version on all the crates that depend on gix-command.

EliahKagan added a commit to EliahKagan/gitoxide that referenced this issue May 9, 2025
This changes `cargo check` commands to `cargo build` in the
`ci-check-msrv` recipe in the `justfile`, which the `check-msrv` CI
jobs in `msrv.yml` use. (It updates a CI step name accordingly.)

The idea is to make sure at least `gix`, with the two combinations
of features tested, actually *builds* under the MSRV toolchain.

As in f10f18d, this also updates the `Makefile` rule corresponding
to that `justfile` recipe.

The idea of actually building was suggested in:
GitoxideLabs#1808 (comment)

However, this does not uncover any new breakages. And there has
been further improvement on GitoxideLabs#1808, including in the commits leading
up to this, as well as earlier, in 569c186 (GitoxideLabs#1909). Nonetheless, it
seems likely that some problems remain with some combinations of
crates and features that are not currently exercised in the MSRV
check. GitoxideLabs#1808 is most likely not yet fully fixed.
@EliahKagan
Copy link
Member

The MSRV was increased to 1.75 in 569c186 (#1909), and I've clarified the check and made it somewhat more robust in #2003, but this issue is most not yet fully fixed.

How the CI job uses nightly

Regarding this from the issue description:

1.74 (nightly!) according to .github/workflows/msrv.yml

And #1808 (comment) (@Byron):

Maybe we can start to untangle this by making the MSRV CI step perform a build instead, and use stable?

The MSRV was already a stable Rust version, and the cargo check commands used to validate it were not using a nightly toolchain.

The check-msrv job in msrv.yml installs two toolchains: the MSRV, and nightly.

rustup toolchain install ${{ env.rust_version }} nightly --profile minimal --no-self-update

This command is easily misread, because ${{ env.rust_version }} nightly, which installs two separate toolchains, looks like ${{ env.rust_version }}-nightly, which would install one toolchain (assuming there is an installable nightly toolchain versioned the same as the MSRV before the -nightly channel suffix).

To verify that this really installed two separate toolchains, a run log can be examined:

info: syncing channel updates for '1.75.0-x86_64-unknown-linux-gnu'
info: latest update on 2023-12-28, rust version 1.75.0 (82e1608df 2023-12-21)
info: downloading component 'cargo'
info: downloading component 'rust-std'
info: downloading component 'rustc'
info: installing component 'cargo'
info: installing component 'rust-std'
info: installing component 'rustc'

  1.75.0-x86_64-unknown-linux-gnu installed - rustc 1.75.0 (82e1608df 2023-12-21)
info: syncing channel updates for 'nightly-x86_64-unknown-linux-gnu'

info: latest update on 2025-05-08, rust version 1.88.0-nightly (e[9](https://github.com/GitoxideLabs/gitoxide/actions/runs/14916020230/job/41901690907#step:4:10)f8103f9 2025-05-07)
info: downloading component 'cargo'
info: downloading component 'rust-std'
info: downloading component 'rustc'
info: installing component 'cargo'
info: installing component 'rust-std'
info: installing component 'rustc'

  nightly-x86_64-unknown-linux-gnu installed - rustc 1.88.0-nightly (e9f8[10](https://github.com/GitoxideLabs/gitoxide/actions/runs/14916020230/job/41901690907#step:4:11)3f9 2025-05-07)

After installing both toolchains, it makes the MSRV the default:

rustup default ${{ env.rust_version }}

It uses the nightly toolchain to downgrade dependencies in Cargo.lock to the lowest versions they can be, while still being consistent with manifests and transitive dependency relationships. To do this, it runs cargo update with the -Zminimal-versions option, which requires +nightly because it not been stabilized.

cargo +nightly update -Zminimal-versions

This option is planned to eventually become available as --minimal-versions, but that has not happened yet. Even with the latest stable version:

ek in 🌐 catenary in gitoxide on  main is 📦 v0.44.0 via 🦀 v1.86.0
❯ rustc +stable --version
rustc 1.86.0 (05f9846f8 2025-03-31)

ek in 🌐 catenary in gitoxide on  main is 📦 v0.44.0 via 🦀 v1.86.0
❯ cargo +stable update --minimal-versions
error: unexpected argument '--minimal-versions' found

  tip: to pass '--minimal-versions' as a value, use '-- --minimal-versions'

Usage: cargo update [OPTIONS] [SPEC]...

For more information, try '--help'.

ek in 🌐 catenary in gitoxide on  main is 📦 v0.44.0 via 🦀 v1.86.0
❯ cargo +stable update -Zminimal-versions
error: the `-Z` flag is only accepted on the nightly channel of Cargo, but this is the `stable` channel
See https://doc.rust-lang.org/book/appendix-07-nightly-rust.html for more information about Rust release channels.

(Note that the "minimal" in -Zminimal-versions is not related to minimal supported Rust versions. My guess is that we are downgrading Cargo.lock in order to avoid failing if it has dependencies that work with the current stable toolchain but not with the older MSRV toolchain. But if the intent of -Zminimal-versions is to do something that is expected to vary across which toolchain runs the cargo update command to which it is passed, then that command should probably be removed or replaced with something different.)

Finally, it runs just ci-check-msrv:

- run: just ci-check-msrv

That ran:

gitoxide/justfile

Lines 243 to 245 in ac5f33d

rustc --version
cargo check -p gix
cargo check -p gix --no-default-features --features async-network-client,max-performance

Those commands do not specify which toolchain to use. Since we do not have a rust-toolchain.toml or rust-toolchain file in the repository, and the CI runner doesn't configure the default toolchain by any mechanism other than that which is overridden to use the MSRV in the rustup default command it ran, the effect is that those commands use the MSRV toolchain.

Changes in #2003

All the above code snippets, as well as the linked and excerpted CI log, are from the last commit strictly before the changes in #2003, which was just merged.

However, the effects described above remain as they were, except that the commands are changed so they:

  • Pass --locked so that, if the versions set with -Zminimal-versions were actually somehow incompatible and the cargo commands would change them, then those cargo commands fail instead.
  • Change the cargo commands in the recipe from cargo check to cargo build.

The ci-check-msrv recipe in the justfile now has these commands:

gitoxide/justfile

Lines 248 to 250 in 9d0c809

rustc --version
cargo build --locked -p gix
cargo build --locked -p gix --no-default-features --features async-network-client,max-performance

#2003 contained various other changes (including some related to the MSRV), but those were the only changes to the behavior I specifically described above.

Also, #2003 did not attempt to do anything related to unit tests, gix-macros, or updating STABILITY.md. Thus, this issue is not yet fixed, for those reasons and also because...

gix and 1.77+?

From the issue description:

However, gix can only build on 1.77+, due to c-string literals, used in rusqlite/rusqlite#1483. The CI passes because it only runs cargo check, which does work on 1.74.

If you mean the gix crate, changing cargo check to cargo build in the ci-check-msrv recipe in the justfile did not surface a failure.

  • If gix crate fails to build with other features besides the two feature sets checked in the recipe -- the default features, and async-network-client,max-performance -- then the existing checks would not surface that. In that case, the existing checks should be expanded.
  • If I've misunderstood and it is actually the gix binary, which is part of the gitoxide crate, that fails to build, then I guess there would need to be a separate decision about whether it's acceptable for that binary crate to have the higher MSRV, and either it should be modified or the rust-version field under [package] in the top-level Cargo.toml should be adjusted.

@Byron
Copy link
Member

Byron commented May 9, 2025

Thank you for the exhaustive analysis and summary!

  • If I've misunderstood and it is actually the gix binary, which is part of the gitoxide crate, that fails to build, then I guess there would need to be a separate decision about whether it's acceptable for that binary crate to have the higher MSRV, and either it should be modified or the rust-version field under [package] in the top-level Cargo.toml should be adjusted.

The binary isn't included in the MSRV right now so for consistency the rust-version should probably be adjusted.
If it's an inconvenience for the downstream it could also be included in the MSRV, but I'd hope that the interested parties could contribute the respective code change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants