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

use [patch] sections to specify serde_json dependency #20

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tvsfx
Copy link

@tvsfx tvsfx commented Oct 22, 2024

The repo currently uses explicit git dependencies for serde_json (for example here)

This has to two disadvantages:

  • It has the undesired side-effect of making these dependencies non-overridable in crates that depend on (crates in) this repo. This is similar to a =-version constraint in a Cargo.toml and can cause version collisions if dependents use a different version of serde_json (no interaction between both versions is then possible).
  • It does not patch serde_json in transitive dependencies of this crate, and hence makes it so this crate uses multiple versions of serde_json (see the diffs in the Cargo.lock files for this PR)

The one small advantage is that it puts no burden on users of this crate to patch serde_json for this crate specifically, but this is easily outweighed by the disadvantages mentioned above.

Better is to use a [patch] section, so that dependencies are patched transitively (if parts of this repo are used and built stand-alone) but can still be re-patched/overridden by its clients. This PR implements this change.

@tvsfx
Copy link
Author

tvsfx commented Oct 22, 2024

CI fails because of serde-rs/json#444, which stipulates usage of serde_json > 1.0.25. Will try upating patches to the 1.0.51 branch until my patch for 1.0.128 lands and we can port to that.

@nshyrei
Copy link
Contributor

nshyrei commented Dec 17, 2024

@tvsfx Please resolve merge conflicts

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