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): update clap and mdbook version #141

Closed
wants to merge 1 commit into from

Conversation

joshka
Copy link
Contributor

@joshka joshka commented Oct 15, 2023

I'd like to be able to install mdbook, mdbook-admonish from a cargo.toml dependencies section (in order to easily write an xtask that can build a site for netlify), but the pin breaks this while not seeming at all necessary. This updates the versions to the latest for both mdbook and clap.

See also the same PR for mdbook-catppuccin catppuccin/mdBook#78

@tommilligan
Copy link
Owner

Hey, thanks for the report. The reason the version was pinned back is to support the existing MSRV (which I try and keep about ~6 months old).

Please could you provide a minimal Cargo.toml file that reproduces the issue? I am not able to reproduce this locally with the latest mdbook and mdbook-admonish versions:

[package]
name = "tmp-mdbook"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
mdbook = "0.4.35"
mdbook-admonish = "1.13.0"
$ cargo build
    Finished dev [unoptimized + debuginfo] target(s) in 0.04s

I think, whatever the cause is, I should be able to fix it by unpinning in Cargo.toml but not changing the version locked in Cargo.lock. This will allow the MSRV to be supported with cargo install mdbook-admonish --locked. But I'll need a repro to confirm I've fixed your issue.

@tommilligan
Copy link
Owner

I've pulled the changes that I think work with the existing MSRV out into this PR, which is now merge in: #142

Could you try installing mdbook-admonish from a git dependency, and see if it resolves your issue?

mdbook-admonish = { git = "https://github.com/tommilligan/mdbook-admonish", branch = "main" }

@joshka
Copy link
Contributor Author

joshka commented Oct 17, 2023

Hey there - I ended up abandoning the idea of installing the various utils via cargo as it's much slower than using binstall (30s instead of 7 minutes), which is fine as a onetime thing locally, but annoying on CI. The minimal repro for this is mdbook-admonish and mdbook-catppuccin in a cargo.toml file. E.g.:

[package]
name = "mdbook-install"
version = "0.1.0"
edition = "2021"

[dependencies]
# mdbook = "0.4.35"
mdbook-admonish = "1.13.0"
mdbook-catppuccin = "2.0.1"

Output:

❯ cargo build
    Updating crates.io index
error: failed to select a version for `clap`.
    ... required by package `mdbook-admonish v1.13.0`
    ... which satisfies dependency `mdbook-admonish = "^1.13.0"` of package `mdbook-install v0.1.0 (/Users/joshka/local/mdbook-install)`
versions that meet the requirements `~4.3` are: 4.3.24, 4.3.23, 4.3.22, 4.3.21, 4.3.20, 4.3.19, 4.3.18, 4.3.17, 4.3.16, 4.3.15, 4.3.14, 4.3.13, 4.3.12, 4.3.11, 4.3.10, 4.3.9, 4.3.8, 4.3.7, 4.3.6, 4.3.5, 4.3.4, 4.3.3, 4.3.2, 4.3.1, 4.3.0

all possible versions conflict with previously selected packages.

  previously selected package `clap v4.4.3`
    ... which satisfies dependency `clap = "=4.4.3"` (locked to 4.4.3) of package `mdbook-catppuccin v2.0.1`
    ... which satisfies dependency `mdbook-catppuccin = "^2.0.1"` (locked to 2.0.1) of package `mdbook-install v0.1.0 (/Users/joshka/local/mdbook-install)`

failed to select a version for `clap` which could resolve this conflict

Could you try installing mdbook-admonish from a git dependency, and see if it resolves your issue?
Yep - the fix resolves that problem (build succeeds).

Happy to close this out.

@tommilligan
Copy link
Owner

Happy to close this out.

Excellent, great to hear. I'll release this as a patch version now.

I dug into this a little further as I'm still surprised this doesn't resolve correctly, given there is a correct set of dependencies that could be installed to satisfy the requirements. But I found this interesting comment by Alex Crichton:

Cargo allows multiple major versions of a crate, but all selected versions of a crate must be semver-incompatible (e.g. can't have 1.3.0 and 1.4.0 as they're semver compatible).

which I think is exactly what's happening here. TIL!

@tommilligan
Copy link
Owner

For my own reference, my notes on trying to understand this:

image

@joshka joshka deleted the update-clap branch October 17, 2023 08:48
@joshka
Copy link
Contributor Author

joshka commented Oct 17, 2023

Yeah, cargo dependency resolution has some interesting behavior that only really makes sense if you spend enough time looking at the edge cases. Thanks again.

@sgoudham
Copy link

Hey there, have been keeping an 👁️ on this thread - the dependency resolution is quite interesting - thanks for the visualisation! @tommilligan

I originally pinned clap in mdbook-catppuccin while trying to troubleshoot and debug some CI/CD failures and I unfortunately forgot to unpin it (Sorry!)

As for mdbook itself, they have unintentionally published breaking changes a few times in their patch versions so I opted to be on the safer side and pin it instead.

I assume unpinning clap makes it easier to avoid situations like these in the future?

@tommilligan
Copy link
Owner

I assume unpinning clap makes it easier to avoid situations like these in the future?

Yes agreed, as the version range is more permissive I think it is less likely to happen. I think to make this bug even less likely to happen you could specify only a major version like clap = "4", assuming of course you're not using any newer features.

As I said above, my initial reason for pinning was to support MSRV installs, but I'm happy with the outcome of just using Cargo.lock/--locked installs for that instead (have documented this in the readme).

Thanks for your work supporting mdbook-admonish in mdbook-catppuccin by the way, I was totally unaware of it until I looked for this issue! Looks really nice visually, will definitely be using it in future. Let me know if you need any support from here to make your life easier with integration work

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