codelabsab / rust-ocpp

Libraries for ocpp 1.6 and 2.0.1
https://docs.rs/rust-ocpp/latest/rust_ocpp/
Apache License 2.0
64 stars 15 forks source link

Remove lock file #50

Closed Erk- closed 2 months ago

Erk- commented 2 months ago

This commit removes the lock file as it does not really help much for libraries since it will be disregarded by dependants.

parberge commented 2 months ago

Interesting 🤔 I think we use it to be deterministic of our builds, but I might be wrong here. Is there any cons of having it though?

https://doc.rust-lang.org/cargo/faq.html#why-have-cargolock-in-version-control seems to suggest that it's not an bad idea, even though it doesn't affect dependants as you mentioned

Erk- commented 2 months ago

It seems that the guidelines was changed last year https://blog.rust-lang.org/2023/08/29/committing-lockfiles.html

Though it also seems that having a lock file makes dependabot think it is a application, which makes it update in a slightly different way: https://github.com/dependabot/dependabot-core/blob/1a0b57f851490074486c818bcdc92ce16eea88b5/cargo/lib/dependabot/cargo/update_checker.rb#L112-L116

I am not sure what is best, there is just a need to make sure that everything is updated, but I guess DependaBot does that well enough.

parberge commented 2 months ago

It seems that the guidelines was changed last year https://blog.rust-lang.org/2023/08/29/committing-lockfiles.html

Though it also seems that having a lock file makes dependabot think it is a application, which makes it update in a slightly different way: https://github.com/dependabot/dependabot-core/blob/1a0b57f851490074486c818bcdc92ce16eea88b5/cargo/lib/dependabot/cargo/update_checker.rb#L112-L116

I am not sure what is best, there is just a need to make sure that everything is updated, but I guess DependaBot does that well enough.

Oh interesting find about dependabot and identifying if library or app. I'm really not an expert in Rust and the tooling, I will leave this to you @tommymalmqvist 😅

tommymalmqvist commented 2 months ago

I'm going to set version-strategy: increase-if-necessary instead and keep Cargo.lock in the repo for now

tommymalmqvist commented 2 months ago

Only auto and lockfile-only is available for cargo... I might open this again

https://docs.github.com/en/code-security/dependabot/dependabot-version-updates/configuration-options-for-the-dependabot.yml-file#versioning-strategy

parberge commented 2 months ago

@tommymalmqvist Yeah I think we actually want to remove lock file in order for dependabot to treat it as a library, and then it should use widen strategy which means it should bump stuff in cargo.toml by increasing the version range if I'm not mistaken. Is that your thoughts as well @Erk- ?

The only question is what happens with deterministic builds, but maybe that's OK.

tommymalmqvist commented 2 months ago

@tommymalmqvist Yeah I think we actually want to remove lock file in order for dependabot to treat it as a library, and then it should use widen strategy which means it should bump stuff in cargo.toml by increasing the version range if I'm not mistaken. Is that your thoughts as well @Erk- ?

The only question is what happens with deterministic builds, but maybe that's OK.

Dependabot only supports auto, lockfile-only for cargo

Screenshot 2024-04-17 at 09 14 54
parberge commented 2 months ago

@tommymalmqvist Yeah I think we actually want to remove lock file in order for dependabot to treat it as a library, and then it should use widen strategy which means it should bump stuff in cargo.toml by increasing the version range if I'm not mistaken. Is that your thoughts as well @Erk- ? The only question is what happens with deterministic builds, but maybe that's OK.

Dependabot only supports auto, lockfile-only for cargo

Screenshot 2024-04-17 at 09 14 54

Yes, and when dependabot has identified this as a library (which it does when it can't find cargo.lock ) it will use widen it seem:

image