tommilligan / mdbook-admonish

A preprocessor for mdbook to add Material Design admonishments.
MIT License
176 stars 20 forks source link

chore(deps): update clap and mdbook version #141

Closed joshka closed 1 year ago

joshka commented 1 year ago

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 https://github.com/catppuccin/mdBook/pull/78

tommilligan commented 1 year ago

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 commented 1 year ago

I've pulled the changes that I think work with the existing MSRV out into this PR, which is now merge in: https://github.com/tommilligan/mdbook-admonish/pull/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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

image

joshka commented 1 year ago

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 commented 1 year ago

Hey there, have been keeping an :eye: 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 commented 1 year ago

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