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 reqwest pinning #757

Merged
merged 2 commits into from
Jan 27, 2024
Merged

Fix reqwest pinning #757

merged 2 commits into from
Jan 27, 2024

Conversation

ok300
Copy link
Contributor

@ok300 ok300 commented Jan 27, 2024

Unfortunately the cargo versioning rules are not very intuitive.

From the docs:

[dependencies]
time = "0.1.12"

The string "0.1.12" is a version requirement. Although it looks like a specific version of the time crate, it actually specifies a range of versions and allows SemVer compatible updates. An update is allowed if the new version number does not modify the left-most non-zero number in the major, minor, patch grouping. In this case, if we ran cargo update time, cargo should update us to version 0.1.13 if it is the latest 0.1.z release, but would not update us to 0.2.0.

This means that

reqwest = { version = "0.11.20", features = ["json"] }

would allow, for example, version 0.11.22.

However, we want to limit it to 0.11.20 at the most, which can be done with

reqwest = { version = "=0.11.20", features = ["json"] }

@ok300 ok300 requested a review from hydra-yse January 27, 2024 00:11
@ok300 ok300 force-pushed the ok300-fix-reqwest-version-pinning branch from e42ea73 to 0482703 Compare January 27, 2024 00:17
@ok300
Copy link
Contributor Author

ok300 commented Jan 27, 2024

Not sure why the CI doesn't like <=, as it worked for me locally. I switched it to =.

@@ -1296,7 +1296,7 @@ dependencies = [
"async-trait",
"base64 0.21.5",
"bech32",
"bitcoin 0.29.2",
"bitcoin 0.30.2",
Copy link
Member

Choose a reason for hiding this comment

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

🤔 I wonder why this updates the bitcoin and lightning-invoice crate as well (as the only crate changed was reqwest) though there don't seem to be any problems with building, so should be alright.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's possible this selected-in or selected-out other dependencies too, based on how they set their own dependency ranges for reqwest.

Copy link
Member

@hydra-yse hydra-yse left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the catch, @ok300

@ok300 ok300 merged commit c60b122 into main Jan 27, 2024
7 checks passed
@ok300 ok300 deleted the ok300-fix-reqwest-version-pinning branch January 27, 2024 00:57
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.

2 participants