perf(package): Speed up verify with 'check' #14930
Conversation
In testing workspace publishing, I was really wishing for the build to be faster when I noticed the sea of status messages was "Compiling", instead of "Checking". I've been glossing over those messages for years and never noticed! This makes builds faster by skipping the compiler backend / codegen at the cost of not getting post-monomorphization errors. That seems like a small price to pay. I searched through the issues, open and closed, and saw no previous discussions of this. In particular, I was looking to see if this was previously rejected. My only assumption is this was missed when `cargo check` was added.
rustbot
added
Command-package
S-waiting-on-review
epage
changed the title
epage
force-pushed
the
package-verify
branch
2 times, most recently
from
3ac2b9d to
3ca10ad
Compare
| @@ -1157,7 +1157,7 @@ fn run_verify( | |||
| opts.jobs.clone(), | |||
| opts.keep_going, | |||
| &opts.targets, | |||
| CompileMode::Build, | |||
| CompileMode::Check { test: false }, | |||
There was a problem hiding this comment.
For myself I would keep it a full build.
One reason is that cargo publish is not an operation easy to revert.
The other is, per RFC 3477,
A Rust program must compile with cargo build to be covered by Rust's standard stability guarantee.
If we want the same level of stability guarantee for published crates, we'd better do a full build.
There was a problem hiding this comment.
I guess the question is what is it we intend to be verified and at what cost.
Things that can be verified
- Basic check
- Post-monomorphization errors
- feature combinations
- platform combinations
- logic (ie
cargo test,cargo packagecould run tests in the packaged tree #14685) - Miri for verifying
unsafe
In commonly used crates, there is most likely a CI checking for what the author feels is important enough. I don't think cargo publish, especially be default, should or can replicate that.
What verify can help with is helping specifically with packaging specific issues
- Are all the right files present
- Can this build in isolation (to a degree)
For those, check is sufficient.
For people who don't have a CI, verify can tell them some but not much. Personally, I don't think we need to be verifying the stability guarantees in these cases, especially at the cost of expensive packaging, which has the most impact when doing it in dry-run mode which is where I most notice this (and tend to avoid dry run because of it).
There was a problem hiding this comment.
On one hand, commonly used crates usually tend to have better CI checking support, so less likely to hit errors like this. Yet it depends on where they run cargo build or cargo test under release profile.
On the other hand, we are getting more API in const contexts (81 in 1.83). While const fn doesn't always contribute to error during monomorphization, it still increases the chances of hitting it.
(Check the minimal example in rust-lang/rust#112301 to understand how to get bitten).
There are also diagnostics during MIR passes cannot be caught by cargo check. By just looking at how easy the example is. I am afraid of this may be more troublesome than monomorphization errors.
I searched through the issues, open and closed, and saw no previous discussions of this.
Supposedly this is a starting point of examining all verification Cargo provides and re-position cargo commands, to embrace the potential plumbing commands reorganization (rust-lang/rust-project-goals#178 perhaps). However, instead of merging into this change now, perhaps we could create an issue and solicit feedback first?
There was a problem hiding this comment.
By all means, I love how you’re thinking ahead with all these verification categories.
There was a problem hiding this comment.
Yet it depends on where they run cargo build or cargo test under release profile.
Good point that there is also --release
While const fn doesn't always contribute to error during rust-lang/rust#99682, it still increases the chances of hitting it.
There are also rust-lang/rust#49292 cannot be caught by cargo check
If post-check errors are getting that bad, then I have strong words for T-lang... The RFC test focuses on linker errors and doesn't justify its existence with it being prevalent.
epage
force-pushed
the
package-verify
branch
3 times, most recently
from
49523a5 to
10bc9cb
Compare
|
I don't personally think this should be the default, because If people want this, for performance, it might make sense as an option people could choose to use. I personally don't think that option should be the default, but zero objections to an option if someone wants it. That said, if we had an option for |
|
☔ The latest upstream changes (possibly 0276088) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Just pitching in my 2 cents. I find that most crates publish new version once or twice a week at most. Although I do think having the ability skip the build during publishing would be useful for projects that are large and publish frequently. (ie. internal business logic libraries that change frequently) |
What does this PR try to resolve?
In testing workspace publishing, I was really wishing for the build to be faster when I noticed the sea of status messages was "Compiling", instead of "Checking".
I've been glossing over those messages for years and never noticed!
This makes builds faster by skipping the compiler backend / codegen at the cost of not getting post-monomorphization errors. That seems like a small price to pay.
Fixes #14941
How should we test and review this PR?
I searched through the issues, open and closed, and saw no previous discussions of this. In particular, I was looking to see if this was previously rejected.
My only assumption is this was missed when
cargo checkwas added.Additional information