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

fix: loosen version constraints #62

Closed
wants to merge 3 commits into from
Closed

fix: loosen version constraints #62

wants to merge 3 commits into from

Conversation

chris13524
Copy link
Member

@chris13524 chris13524 commented Feb 14, 2024

Description

Removes specific version req's. We should be as permissive as possible, letting the downstream app customize the versions. If this library requires a specific minor version or patch, then we can use ^, ~ or * as appropriate

Resolves #44

How Has This Been Tested?

Not tested

Due Diligence

  • Breaking change
  • Requires a documentation update
  • Requires a e2e/integration test update

@chris13524 chris13524 self-assigned this Feb 14, 2024
@chris13524 chris13524 linked an issue Feb 14, 2024 that may be closed by this pull request
regex = "1.7"
once_cell = "1.16"
jsonwebtoken = "8.1"
regex = "1"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what happens if relay_rpc is using features added in 1.7 release, while the consumer code forces e.g. 1.0 version?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we needed features/fixes from the 1.7 release, then we would set the minimum to 1.7 not 1 as I did

@xDarksome
Copy link
Contributor

@heilhead
Copy link
Collaborator

Agreed with Nodar and that PSA. I don't think we should make this change, because of how cargo actually resolves dependency versions.

@chris13524
Copy link
Member Author

chris13524 commented Feb 15, 2024

The PSA linked actually doesn't discourage loosening version constraints like I did, but requests that when doing so that cargo +nightly -Z minimal-versions update is used in CI. Lose constraints is still good.

I added this to CI, and looks like indeed these are not appropriate version constraints. Furthermore, I tested minimal-versions against the main branch and it also fails there. Looks like (many) other crates are also lying about what versions they support.

As discussed in the thread (and the stabilization issue for the flag), fixing this requires all downstream crates getting their dependencies right. Hopefully one day we'll get there and we'll be able to install this CI and loosen our own constraints. But closing this until then.

I suppose it's slightly better that, for now, when installing a new package that you use the current version, rather than just the major version. So then you are at least not lying about what version you support. But this could still get outdated as cargo will still install the latest version of the package, which is the one you test against.

@chris13524 chris13524 closed this Feb 15, 2024
@chris13524 chris13524 deleted the 44-tokio-version branch February 15, 2024 16:36
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.

Tokio Version
3 participants