ci: drop really old trick and ensure MSRV for all feature combo #676
Conversation
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
There was a problem hiding this comment.
yak shaving from here .. even from rustc 1.60.0 we don't need this trick I think.
There was a problem hiding this comment.
Glad to see this get cleaned up! I think we were previously only testing these in our MSRV job. To get them working this way we may need to pull that macros module into the regular log crate so its tests are still picked up.
There was a problem hiding this comment.
Do you provide any change here?
Cargo would detect tests folder and these two files are already tested by the after setup.
See https://doc.rust-lang.org/book/ch11-03-test-organization.html#integration-tests
| - uses: actions/checkout@master | ||
| - name: Install Rust | ||
| run: | | ||
| rustup update 1.60.0 --no-self-update | ||
| rustup default 1.60.0 | ||
| - run: | | ||
| cargo test --verbose --manifest-path tests/Cargo.toml | ||
| cargo test --verbose --manifest-path tests/Cargo.toml --features kv |
There was a problem hiding this comment.
Previously we check MSRV only for a subset.
There was a problem hiding this comment.
If we agree that MSRV is only valid for no feature / with kv, I can revert the test targets and undo MSRV -> 1.61 and the sval PR is unnecessary.
There was a problem hiding this comment.
Previously we only applied it to a subset, but now that the key-value API is stable I’m happy to add one for it. What do you think @Thomasdezeeuw?
There was a problem hiding this comment.
Thanks for your information! I'd emphasize that ensuring MSRV for sval/serde would require bumping MSRV to 1.61, mainly for const fn. You may take it into consideration whether it's possible, since I see:
This version is explicitly tested in CI and may be bumped in any release as needed. Maintaining compatibility with older compilers is a priority though, so the bar for bumping the minimum supported version is set very high. Any changes to the supported minimum version will be called out in the release notes.
There was a problem hiding this comment.
I think we haven't done an MSRV bump in some time, so it should be safe to move up to 1.61.0 and keep a single policy 👍
There was a problem hiding this comment.
Technically a MSRV bump is a breaking change, but since 1.61 came out in May 2022, I think it's fine we increase it.
There was a problem hiding this comment.
I don't know why you say it is "technically" a breaking change. The log policy in the README even explicitly allows it, and that is the general practice of Rust libraries (although some do treat MSRV bumps as semver incompatible).
See also: https://doc.rust-lang.org/cargo/reference/rust-version.html#setting-and-updating-rust-version
There was a problem hiding this comment.
I'm not sure why you're questioning whether or not bumping MSRV is a breaking change or not. It is. If I run Rust 1.60 then I can run log 0.4.26, but not 0.4.27, that's a breaking change.
Whether or not it's allowed by our own policy is a different question.
There was a problem hiding this comment.
The topic has been discussed to death. Search the libs team repo or the api guidelines repo. I'm on mobile or I would link you. Increasing toolchain requirements is broadly not considered a breaking change.
|
Thanks for doing some housekeeping here @tisonkun! I just had a few comments, but I think the direction this is all moving in is good. The only one I feel particularly strongly about is the use of external actions in CI. The pinning of build workers is not that big a deal, so if you do feel strongly about it then I'm happy to keep it that way. |
Thomasdezeeuw
left a comment
Thomasdezeeuw
left a comment
There was a problem hiding this comment.
For the future it would be nice if these can be different prs. This does so many things it much harder to review.
| - uses: actions/checkout@master | ||
| - name: Install Rust | ||
| run: | | ||
| rustup update 1.60.0 --no-self-update | ||
| rustup default 1.60.0 | ||
| - run: | | ||
| cargo test --verbose --manifest-path tests/Cargo.toml | ||
| cargo test --verbose --manifest-path tests/Cargo.toml --features kv |
There was a problem hiding this comment.
Technically a MSRV bump is a breaking change, but since 1.61 came out in May 2022, I think it's fine we increase it.
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
|
@KodrAus @Thomasdezeeuw Thanks for your reviews. I pushed several self-described commits to address all your comments. |
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
tisonkun
changed the title
|
Thanks @tisonkun 👍 The only issue left I think is that we're no longer running our macro tests in CI. Otherwise this looks good to me. |
Signed-off-by: tison <[email protected]>
|
Thanks for your review @KodrAus @Thomasdezeeuw! Some out of thread comments: @KodrAus Yes I always find that once the code structure becomes more straightforward (self-described and well structured), it can foster more contributions. Otherwise, to work around an old workaround you add some new tricks and quickly the codebase becomes misty and even unmaintainable. I like "obviously no bugs" over "no obvious bugs". |
4 participants
@KodrAus I found that our MSRV is not quite 1.60.0 since we rely on value-bag 1.7+ who needs 1.61 for const fn. This is also the MSRV for many other dependencies newest version (serde_derive, memchr, syn, etc.)
Also I found sval-rs/sval#207 breaks MSRV 1.60 or 1.61.
Not sure if our MSRV for basic usage or any feature combo. I'd suggest to bump the MSRV to 1.61 and try to ensure that sval won't break the same baseline.