Stream: git-wasmtime

Topic: wasmtime / Issue #2387 Fix incorrect minimum Rust version

Wasmtime GitHub notifications bot (Nov 10 2020 at 17:46):

cfallin commented on Issue #2387:

Hi @Voultapher -- so, at least the core codegen bits of Cranelift should build with stable Rust (1.43+ currently) -- we check this on CI here: link. wasmtime and clif-util should also build on stable Rust. Could you say a little more about the issues you're seeing that are requiring nightly?

(re the PR overall, I agree we should update the badge and info -- thanks for taking the time to do this!)

Wasmtime GitHub notifications bot (Nov 10 2020 at 17:55):

Voultapher commented on Issue #2387:

First ran into the issue mentioned in the #2377 and then it complained that feature(test) is nightly only. Even if those core bits are 1.43 we should be more explicit that does not mean, that clone + cargo build will work.

Wasmtime GitHub notifications bot (Nov 10 2020 at 17:55):

Voultapher edited a comment on Issue #2387:

First ran into the issue mentioned in #2377 and then it complained that feature(test) is nightly only. Even if those core bits are 1.43 we should be more explicit that does not mean, that clone + cargo build will work.

Wasmtime GitHub notifications bot (Nov 12 2020 at 15:18):

alexcrichton commented on Issue #2387:

I personally feel that nightly shouldn't really be mentioned at all in the README. If lightbeam really is the only consumer of nightly that's not heavily developed right now anyway and it's not worth saying we require nightly in the README. I think that the readme should say "works on the current stable Rust" so we don't have to update it every 6 weeks personally.

Wasmtime GitHub notifications bot (Nov 12 2020 at 19:25):

Voultapher commented on Issue #2387:

So here is my personal experience with wanting to get started contributing. I read the README and it says 'so for some cargo commands, such as cargo test, the --all is needed to tell cargo to visit all of the crates.', in the moment I understood that as 'please run cargo test --all' otherwise you're missing important tests. So I do that, and get a build failure, I think to myself, oh right haven't updated rust for a while. Go check it, hmm, 1.44 but the badge says 1.37, whatever. I update, then it gets a bit further, and complains, ey that's nightly only. Alright I switch to nightly, and then more than hour later I've finally finished my first build + test cycle, which also throws up some errors. That does not feel nice.

Now I suggest we adjust the README to avoid future contributors from experiencing the same frustrations. If you want to avoid nightly in your README because it looks bad, well then don't use nightly. Saying it works with stable while hiding the fact, that if you follow the recommendation 2 lines below it won't work with stable, that feels disingenuous. I tried being clear in the latest proposed wording what works with stable and what doesn't. I suggest we are either open and honest with what is required and expected, or somehow feature gate out those nightly parts, so that only those few actively working on Lightbeam and knowing what they are doing require it, even when running cargo test --all.

Wasmtime GitHub notifications bot (Nov 13 2020 at 14:22):

tschneidereit commented on Issue #2387:

Thank you for sharing that feedback, @Voultapher. I think you're right that there's a seeming inconsistency in the documentation that we should resolve. I also think that in isolation the individual parts you mention are true, but combined I can see how they're confusing for newcomers, which isn't great. I think your proposed changes go in the right direction, but we might want to go even further, in particular around clarifying what's expected to pass in CI.

ISTM like we should clarify the section on testing to make explicit what testing should be done in which circumstances. Ideally, this would state something like "for most development, running the tests for the crate you're changing should be sufficient, but you may also want to run the same tests we run in our CI system", and then describe how to run individual crate tests with an example, and how to run the CI tests. Then we could end with a note on running all tests, but mention that some of them require a Nightly version of Rust, and some crates are expected to fail.

It's unfortunate that there's no way to specify a default set that'd be run with cargo test. @alexcrichton, AFAICT that's not possible, right? Alternatively perhaps we can extract the cargo test invocation from the CI config into a shell script and refer to that?

Wasmtime GitHub notifications bot (Nov 16 2020 at 15:53):

alexcrichton commented on Issue #2387:

I agree with @tschneidereit that the best fix here seems to be updating the documentation on what tests are expected to be run by users (or how to run tests). Running cargo test --all not only requires nightly because of one crate right now but it also tests a lot more than you probably want to (e.g. building z3 for peepmatic).

Also, unfortunately, yeah, I don't think there's a way through Cargo to customize the default set of tests run via cargo test. (other than the shell script, which I'm not a huge fan of because it doesn't support Windows well)

Last updated: Nov 22 2024 at 16:03 UTC