`cargo package` (and hence `cargo publish`) verify step is slow from doing a full build

[epage]

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!

If we switch from cargo build to `cargo check it would make 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.

[epage]

@weihanglo from https://github.com/rust-lang/cargo/pull/14930#discussion_r1883135407

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.

[epage]

@epage from https://github.com/rust-lang/cargo/pull/14930#discussion_r1883266586

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 package could 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).

[epage]

@weihanglo from https://github.com/rust-lang/cargo/pull/14930#discussion_r1887536024

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?

[epage]

In rust-lang/rust#49292, it sounds like its being treated as a bug that MIR diagnostics are not running during cargo check though it can come at the cost of performance for dependency builds.

[epage]

We discussed this in the recent Cargo team meeting.

cargo publish is generally not a command where performance matters (except in more extreme cases like #14955) as publishes are not in an inner loop. However, we need to keep in mind what workflows we don't have today and how performance problems can shape whether people exercise it. In this case, we don't have workspace publishing support today and for a tool to implement --dry-run, like cargo-release, they have to pass --no-verify to because dependencies won't be present in the registry. With workspace publishing, users can do a --dry-run with verification which can better help them test or debug their publishing process. If someone has to iterate on their package.include or an unrelated part of their release process (like cargo-release pre-release-replacements) then a build-verification can really slow things down. There is also the possibility of testing the release process in CI to catch breakages on the PR that would cause them, rather than at release.

If we had a somewhat general solution to #14685, this is mostly a question of defaults. Without it, we would be locking people into an answer which puts more pressure on the decision. However, there are several questions to work out for #14685 that could slow down this issue if we block on it.

An important question for us to answer is what the default purpose of the verify step should be

• Sanity check packaging (e.g. were all needed files included)
  • If you consider testing of a .crate file, then running tests is the only way to sanity check it because they can rely on data files, see #14685 for more details
• Last-ditch baseline quality check
  • In case no CI
  • In case changes were made locally before publishing, particularly with --allow-dirty

If its a last-ditch baseline quality check, then running build adds extra assurances in light of rust-lang/rfcs#3477. The question then becomes, what is the right default baseline?

• A fast "most build problems" check
• Current: The default feature on a dev profile on the current platform
• Different feature combinations?
• Different platform combinations?
• Different profile combinations?
• Extra testing, unsafe checking, or formal proofs

See #14685 for more on that.

If we go with a fast, but imperfect, check, we are dealing with

• Probability of a breakage covered by rust-lang/rfcs#3477
  • Some cases that are only caught today in build don't have to be and could change, see rust-lang/rust#49292
• Probability that CI is used or the crate is left unchanged since the CI run
• Number of users impacted by a version with a problem
  • More widely used crates are more likely to have CI
  • When changing our lockfile policy, part of it came from an assumption of CI use
  • However, some crates purportedly don't get much value out of CI and so just do checks locally as part of their release

The scariest part of rust-lang/rfcs#3477 is that a crate that builds with cargo check is not subject to Rust's stability guarantees, only cargo build. However, when users run cargo run or cargo test, their dependency would have likely failed because a build` is happening at that point, so its unlikely anyone will be using a version that could be broken in a future release.

If its just as a sanity check, then this is a question of whether anything would be missed by check that build would have caught. Maybe a linker error from something a build script does, like -sys crates.

[epage]

Started a discussion on Internals

[weihanglo]

@joshtriplett from https://github.com/rust-lang/cargo/pull/14930#issuecomment-2548760274:

I don't personally think this should be the default, because cargo build does catch things cargo check does not, per RFC 3477 as mentioned.

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 package/publish to do a full cargo test before packaging up (since test needs files that might not go into the package), then I think it'd make sense to only do check on the resulting package, because you'd already know tests pass so you only need to smoke-test that you've included everything you need in the package.

[weihanglo]

@ranger-ross from https://github.com/rust-lang/cargo/pull/14930#issuecomment-2563467348:

Just pitching in my 2 cents. I also tend to lean towards keeping cargo build as part cargo publish for the additional verification.

I find that most crates publish new version once or twice a week at most. So to me the tradeoff is saving of one or two builds a week vs keeping the additional validation. With that framing, I think the extra time building are worth the additional protection against publishing a broken library.

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) So having something like cargo publish --skip-build would make sense to me :+1:

[RalfJung]

In rust-lang/rust#49292, it sounds like its being treated as a bug that MIR diagnostics are not running during cargo check though it can come at the cost of performance for dependency builds.

Yes, that one is a bug -- MIR passes run pre-monomorphization so ideally we can emit those diagnostics even for check builds.

However, the larger problems is errors that occur during monomorphization. The fact that those are not emitted during a check build is not currently considered a bug, it is a deliberate compiler team decision. Detecting those errors during check builds would make check builds significantly slower, since rustc would have to detect all required monomorphizations of all functions and ensure they are all valid -- a task that is currently entirely skipped.

IOW, https://github.com/rust-lang/rust/issues/99682 is largely not-a-bug.