Skip to content

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

Merged
torkleyy merged 1 commit into
ron-rs:masterfrom
torkleyy:deps
Nov 26, 2021
Merged

chore(deps): use exact version for bitflags#340
torkleyy merged 1 commit into
ron-rs:masterfrom
torkleyy:deps

Conversation

@torkleyy

Copy link
Copy Markdown
Member

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
Copy Markdown
Member Author

r? @MomoLangenstein

@juntyr

juntyr commented Nov 18, 2021

Copy link
Copy Markdown
Member

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
Copy Markdown
Member 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

juntyr commented Nov 18, 2021

Copy link
Copy Markdown
Member

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
Copy Markdown
Member 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
Copy Markdown

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

torkleyy commented Dec 6, 2021

Copy link
Copy Markdown
Member Author

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

@Imberflur

Copy link
Copy Markdown

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
Copy Markdown

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

@torkleyy

torkleyy commented Dec 7, 2021

Copy link
Copy Markdown
Member Author

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