-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: pin to working protos version, add lockfile #124
Conversation
A patch release of the protos contained a breaking change, and because we didn't pin to a specific version or have a Cargo.lock file, this caused the build to break for this project and consuming projects. This commit adds the Cargo.lock file, and (for now) pins to a version of the client protos that this library will compile successfully against.
@@ -18,7 +18,7 @@ members = [ | |||
exclude = [ "example" ] | |||
|
|||
[dependencies] | |||
momento-protos = { version = "0.42" } | |||
momento-protos = { version = "=0.42.4" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really think it makes sense to pin to explicit versions like this. Semantically the SDK depends on momento protos 0.42, and the lockfile is the better tool for explicit versions.
I'm not going to block you on this and I will approve once the approvals are green but I don't agree with this style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right now if i don't do this, it literally won't compile because 0.42.5 is not compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could put the version lock in the new cargo.lock file like this
[[package]]
name = "momento-protos"
version = "0.42.4"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was not aware of / under the impression that it was fair game to make manual edits to that file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One can edit any file one wishes to edit. It's certainly atypical, but it's also quite atypical to publish a library without a cargo.lock file :-(
As I said, I'm not going to block you but I disagree with this plan.
Prior to this commit we had a dep on a time library called chrono. In this PR we removed the Cargo.lock file from .gitignore, because without it we were picking up patch releases of dependencies that could cause the build of HEAD to break without any changes to our code. When Cargo.lock was re-added, github found a security vulnerability in the `time` library that `chrono` depended on. There is no new version of chrono that isn't pinned to a vulnerable version of `time`. We also don't really need to force a dependency on chrono on our users. To all of those ends, this commit removes the chrono dep in favor of the stdlib SystemTime.
@kvcache i pushed a commit that attempts to remove the chrono dep. |
idk how to make the checks re-run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for removing that chrono dependency! We'll figure out why the approval isn't running or I'll pull this down and independently verify tomorrow morning.
@@ -27,7 +27,6 @@ jsonwebtoken = "8.0.1" | |||
rand = "0.8.5" | |||
serde = {version = "1.0", features = ["derive"] } | |||
serde_json = "1.0.79" | |||
chrono = {version = "0.4.19", features = ["serde"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hooray!
src/simple_cache_client.rs
Outdated
.expect("couldn't parse from timestamp"), | ||
Utc, | ||
), | ||
expires_at: SystemTime::from(UNIX_EPOCH + Duration::from_secs(res.expires_at)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice.
I'm under the impression the cargo lock file in a library is ignored. So
your users will still have an issue if you don't explicitly pin in the
manifest
…On Tue, Feb 14, 2023, 6:55 PM Kenny ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Cargo.toml
<#124 (comment)>
:
> @@ -18,7 +18,7 @@ members = [
exclude = [ "example" ]
[dependencies]
-momento-protos = { version = "0.42" }
+momento-protos = { version = "=0.42.4" }
One can edit any file one wishes to edit. It's certainly atypical, but
it's also quite atypical to publish a library without a cargo.lock file :-(
As I said, I'm not going to block you but I disagree with this plan.
—
Reply to this email directly, view it on GitHub
<#124 (comment)>,
or unsubscribe
<https:/notifications/unsubscribe-auth/ACP6HF7R6G5EJYDBFL6L46LWXRARDANCNFSM6AAAAAAU4CU6UY>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
That would be news to me, but I don't have a ton of cargo distribution experience. Thanks for your insight! |
Specifically, I think it'll work in the library when you build the library.
But it is not honored by consumers of your library. It seems to still be
advised to not commit a lock file in a library only project. Was just
chatting with Sean about this the other day for another project
…On Tue, Feb 14, 2023, 6:59 PM Kenny ***@***.***> wrote:
I'm under the impression the cargo lock file in a library is ignored. So
your users will still have an issue if you don't explicitly pin in the
manifest
That would be news to me, but I don't have a ton of cargo distribution
experience. Thanks for your insight!
—
Reply to this email directly, view it on GitHub
<#124 (comment)>,
or unsubscribe
<https:/notifications/unsubscribe-auth/ACP6HF47ZNGZXXW47WUMAGDWXRBCPANCNFSM6AAAAAAU4CU6UY>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
src/simple_cache_client.rs
Outdated
.expect("couldn't parse from timestamp"), | ||
Utc, | ||
), | ||
expires_at: SystemTime::from(UNIX_EPOCH + Duration::from_secs(res.expires_at)), |
Check failure
Code scanning / clippy
useless conversion to the same type: `std::time::SystemTime`
src/simple_cache_client.rs
Outdated
.expect("couldn't parse timestamp from signing key"), | ||
Utc, | ||
), | ||
expires_at: SystemTime::from(UNIX_EPOCH + Duration::from_secs(signing_key.expires_at)), |
Check failure
Code scanning / clippy
useless conversion to the same type: `std::time::SystemTime`
Okay, so re-reading documentation on this yet again it appears I was exactly backwards on what I remembered and confidently gave the opposite guidance of the correct best practice. 🤦 Thank you. I remembered this page clearly and opposite of what it is in reality: https://doc.rust-lang.org/cargo/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries We should not check in a lockfile to this repo. |
I've put the lock file back into .gitignore. |
A patch release of the protos contained a breaking change, and
because we didn't pin to a specific version or have a Cargo.lock
file, this caused the build to break for this project and
consuming projects. This commit adds the Cargo.lock file,
and (for now) pins to a version of the client protos that this
library will compile successfully against.