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

chore(deps): use exact version for bitflags #340

Merged
merged 1 commit into from
Nov 26, 2021
Merged

Conversation

torkleyy
Copy link
Contributor

Motivation: allow building with MSRV without patching lock file - see #332

Usually this is discouraged, but bitflags only provides a macro that generates code for us, so if it gets included with multiple different versions in a dependency tree it won't cause any issues.

  • [x] I've included my change in CHANGELOG.md not necesary

Motivation: allow building with MSRV without patching lock file
@torkleyy
Copy link
Contributor Author

r? @MomoLangenstein

@juntyr
Copy link
Member

juntyr commented Nov 18, 2021

I think you should also pin indexmap since building with all features on 1.36.0 still fails. Afterwards, the workaround in the CI could probably also be removed.

@torkleyy
Copy link
Contributor Author

Good point, I wasn't aware indexmap is also an issue. However, forcing an exact version for it has more implications than it has for bitflags...

Afterwards, the workaround in the CI could probably also be removed.

Yes, I'll do that 👍

@juntyr
Copy link
Member

juntyr commented Nov 18, 2021

Good point, I wasn't aware indexmap is also an issue. However, forcing an exact version for it has more implications than it has for bitflags...

That is true ... it seems that hashbrown is the problem there as it uses the ptr_cast feature that wasn't stable yet in 1.36.0

@torkleyy
Copy link
Contributor Author

I'll merge this as-is, it's already a bit of an improvement.

@torkleyy torkleyy merged commit ed666ab into ron-rs:master Nov 26, 2021
@torkleyy torkleyy deleted the deps branch November 26, 2021 18:54
@Imberflur
Copy link

bitflags only provides a macro that generates code for us, so if it gets included with multiple different versions in a dependency tree it won't cause any issues.

IME cargo will fail to compile rather than including multiple compatible versions of a crate in the tree, so anyone depending on ron won't be able to use a newer version of bitflags (unless they release a new major version)

@torkleyy
Copy link
Contributor Author

torkleyy commented Dec 6, 2021

@Imberflur that's only when you have that dependency in your public API.

@Imberflur
Copy link

that's only when you have that dependency in your public API.

@torkleyy unless I'm missing something that is not the case.

I created a test setup to try this and it produces an error https://github.com/Imberflur/cargo-dep-resolution-test

FWIW I'm not currently running into this issue, but I expect I will likely encounter it in the future when another dependency or my code tries to take advantage of any newer features in the bitflags crate. So I wanted to explore the issue ahead of time.

This might be relevant: https://github.com/rust-lang/rfcs/blob/master/text/2495-min-rust-version.md#future-work-and-extensions

@Imberflur
Copy link

e.g. see nix-rust/nix#1548

@torkleyy
Copy link
Contributor Author

torkleyy commented Dec 7, 2021

Oh I see. My understanding was that Cargo can handle multiple versions of a dependency in the tree, which is the case, but with the limitation that all versions must be semver incompatible, which isn't true for bitflags...

@torkleyy torkleyy mentioned this pull request Dec 7, 2021
torkleyy added a commit to torkleyy/ron that referenced this pull request Dec 27, 2021
juntyr pushed a commit to juntyr/ron that referenced this pull request Jan 28, 2022
juntyr added a commit that referenced this pull request Jan 28, 2022
* revert "chore(deps): use exact version for bitflags"

This reverts commit 6bff207.
PR: #340
Fixes #347

* chore: bump msrv to 1.46.0

* chore: fix test warnings

* Switched to fxx::EPSILON and #[non_exhaustive]

Co-authored-by: Thomas Schaller <me@torkleyy.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants