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

Run cargo update before testing #114

Merged
merged 4 commits into from Apr 4, 2023
Merged

Run cargo update before testing #114

merged 4 commits into from Apr 4, 2023

Conversation

cfvescovo
Copy link
Collaborator

@cfvescovo
Copy link
Collaborator Author

cc @j-mendez

@EdJoPaTo
Copy link
Contributor

EdJoPaTo commented Mar 1, 2023

Was the Cargo.lock added for some version pinning? #97 reads a bit like that. If that's the case the pins should be added to the Cargo.toml

@nathaniel-daniel
Copy link

Looks like it was introduced for a reason, with fea28ac, though the reason is not specified. Note that this repo also contains a runnable binary at https://github.com/causal-agent/scraper/blob/master/src/main.rs.

@EdJoPaTo
Copy link
Contributor

EdJoPaTo commented Mar 1, 2023

Looks like it was added/removed more than once then… whatever the problem was, a Cargo.lock file can not resolve it so there has to be another way there.

@adamreichold
Copy link
Contributor

Please see #113 (comment) I don't think this should be removed as long as scraper is both a library and a CLI tool (for which the lock file does serve a purpose).

@adamreichold
Copy link
Contributor

whatever the problem was, a Cargo.lock file can not resolve it so there has to be another way there.

This is not about pinning compatible versions which as you point out must either happen in the Cargo manifest (when widening the version range) or in downstream lock files for library uses (to narrow the version range). But for the binary shipped as part of this crate this lock file is used by e.g. cargo install --locked scraper.

@j-mendez
Copy link
Contributor

j-mendez commented Mar 1, 2023

Please see #113 (comment) I don't think this should be removed as long as scraper is both a library and a CLI tool (for which the lock file does serve a purpose).

Agreed. Lock file should be included.

@EdJoPaTo
Copy link
Contributor

EdJoPaTo commented Mar 1, 2023

The binary is a good point. However it hides errors for library usage. If the lock file is kept there should be some CI or workflow to ensure both the lock file and the newest versions in the specified ranges work.

@adamreichold
Copy link
Contributor

If the lock file is kept there should be some CI or workflow to ensure both the lock file and the newest versions in the specified ranges work.

We could just run cargo update at the beginning of the CI workflows which test the library.

@cfvescovo cfvescovo changed the title Remove Cargo.lock Run cargo update before testing Mar 5, 2023
.gitignore Outdated Show resolved Hide resolved
Co-authored-by: EdJoPaTo <github@edjopato.de>
@EdJoPaTo
Copy link
Contributor

EdJoPaTo commented Mar 5, 2023

Currently it does not use clippy with updates might also hide some dependency changes but the most important ones are probably caught when building the tests.

The more important issue probably is the ignored lock file now in CI. the CI probably should do both: one with the lock file and one with cargo update.

@cfvescovo
Copy link
Collaborator Author

cfvescovo commented Mar 10, 2023

The more important issue probably is the ignored lock file now in CI. the CI probably should do both: one with the lock file and one with cargo update.

I am afraid doing that would considerably increase the time taken to perform our Github Actions. I do not know how many minutes are currently available.

@cfvescovo cfvescovo merged commit aa237be into master Apr 4, 2023
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.

Remove Cargo.lock
5 participants