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

cfg-if 0.1.2 is minimal version of dependency #112

Merged
merged 1 commit into from
Oct 1, 2019

Conversation

dekellum
Copy link
Contributor

Some external minimal version testing (incl. rust 1.32.0 and recent nightly) suggests that any getrandom release since 0.1.7 actually requires cfg-if 0.1.2. Otherwise it will fail to build with errors like:

  --> /home/david/.cargo/registry/src/github.com-1ecc6299db9ec823/getrandom-0.1.7/src/util_libc.rs:25:6
   |
25 |     }
   |      ^ missing tokens in macro arguments

error[E0425]: cannot find function `errno_location` in this scope
  --> /home/david/.cargo/registry/src/github.com-1ecc6299db9ec823/getrandom-0.1.7/src/util_libc.rs:29:27
   |
29 |     let errno = unsafe { *errno_location() };
   |                           ^^^^^^^^^^^^^^ not found in this scope

error: aborting due to 2 previous errors

This just updates the dependency to the minimum required.

@josephlr
Copy link
Member

So our CI actually has a check for minimal version compatibility. However, if you look at the CI that runs, you'll note we end up using cfg-if v0.1.3 instead of cfg-if v0.1.0.

This happens because cargo generate-lockfile -Z minimal-versions brings in optional dependencies and some of our optional/dev deps (namely wasm-bindgen-test and log) have transitive dependencies on higher cfg-if versions.

@newpavlov @dhardy I don't know how to stop this issue from coming up in the future. Maybe we should mention it in rust-lang/cargo#5657?

Regardless, this PR is fine. I just wish we had a way to check that we are doing things correctly.

@dekellum
Copy link
Contributor Author

Not to overstate how easy any of this is, but maybe you could include some sort of --no-default-dependencies generation of minimal lock file and test step? Thanks.

@josephlr
Copy link
Member

josephlr commented Oct 1, 2019

Not to overstate how easy any of this is, but maybe you could include some sort of --no-default-dependencies generation of minimal lock file and test step? Thanks.

Unfortunately, that's not exactly what we need, as the dependencies aren't on by default. Also, using -Z avoid-dev-deps doesn't fix this problem. We essentially need a "target/feature aware" version of cargo generate-lockfile to make this work.

It might just also be the case that the tooling for actually checking minimal versions isn't good enough yet. A lot of people (reasonably) want this feature. See: rust-random/rand#874, rust-random/rand#867, and @BurntSushi's comments about minimal versions checks in BurntSushi/quickcheck#241 (comment). I want minimum version specifications to work, I'm just worried that if the tooling can't correctly find the minimal version, crates will have trouble knowing if they're done things correctly.

My rant aside, #114 fixes the CI issues. @dekellum when that PR merges could you rebase?

@newpavlov
Copy link
Member

I don't know how to stop this issue from coming up in the future. Maybe we should mention it in rust-lang/cargo#5657?

Yes, I agree. I don't think we can do much on our side, so it's a tooling issue.

@dhardy
Copy link
Member

dhardy commented Oct 1, 2019

Yes, in this case I don't think we can do much other than accept fixes like this one. Thank you @dekellum. The CI failure is clearly unrelated so lets just merge.

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.

4 participants