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

ci: commit Cargo.lock #117

Closed

Conversation

agostbiro
Copy link
Contributor

Fixes #116

I could reproduce the issue when running cargo build --verbose --target wasm32-unknown-unknown --no-default-features --features "browser" --locked in the repo root without a Cargo.lock file.

It should be fixed by committing the Cargo.lock file which I did in this PR. This should have no undesirable effects as it'll be ignored when users of the library compile it.

@prestwich
Copy link
Member

hmmm, I think i'd be more inclined to remove the --locked argument. Official docs recommend not commiting the lockfile for libs

https://doc.rust-lang.org/cargo/guide/cargo-toml-vs-cargo-lock.html

@agostbiro
Copy link
Contributor Author

agostbiro commented Mar 10, 2023

Sure, that would work too.

I think the docs are more of a guidance as committing the Cargo.lock file has no effect from the end user's perspective for libraries. But it's ok to commit it if reproducible builds in the CI are important (eg. ethers-rs and elliptic-curves do it). The advantage of not having it committed imo is that the CI can serve as a canary if one of the dependencies has a new version that is allowed by semantic versioning, but causes some issues.

Wdyt, want me to remove the --locked argument instead?

@prestwich
Copy link
Member

I went ahead and removed the locked arg this morning, thank you!

@prestwich prestwich closed this Apr 2, 2023
@agostbiro agostbiro deleted the agostbiro/commit-cargo-lock branch April 2, 2023 16:11
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.

fix wasm CI
2 participants