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.

Sign up for GitHub

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

perf(package): Speed up verify with 'check' #14930

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+135 −135
Open

perf(package): Speed up verify with 'check' #14930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/cargo/ops/cargo_package.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1157,7 +1157,7 @@ fn run_verify(
opts.jobs.clone(),
opts.keep_going,
&opts.targets,
CompileMode::Build,
CompileMode::Check { test: false },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the question is what is it we intend to be verified and at what cost.

Things that can be verified

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By all means, I love how you’re thinking ahead with all these verification categories.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #14941

)?,
cli_features: opts.cli_features.clone(),
spec: ops::Packages::Packages(Vec::new()),
Expand Down
10 changes: 5 additions & 5 deletions tests/testsuite/alt_registry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,8 +351,8 @@ See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for
[VERIFYING] foo v0.0.1 ([ROOT]/foo)
[DOWNLOADING] crates ...
[DOWNLOADED] bar v0.0.1 (registry `alternative`)
[COMPILING] bar v0.0.1 (registry `alternative`)
[COMPILING] foo v0.0.1 ([ROOT]/foo/target/package/foo-0.0.1)
[CHECKING] bar v0.0.1 (registry `alternative`)
[CHECKING] foo v0.0.1 ([ROOT]/foo/target/package/foo-0.0.1)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[UPLOADING] foo v0.0.1 ([ROOT]/foo)
[UPLOADED] foo v0.0.1 to registry `alternative`
Expand Down Expand Up @@ -515,7 +515,7 @@ See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for
[PACKAGING] foo v0.0.1 ([ROOT]/foo)
[PACKAGED] 4 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
[VERIFYING] foo v0.0.1 ([ROOT]/foo)
[COMPILING] foo v0.0.1 ([ROOT]/foo/target/package/foo-0.0.1)
[CHECKING] foo v0.0.1 ([ROOT]/foo/target/package/foo-0.0.1)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[UPLOADING] foo v0.0.1 ([ROOT]/foo)
[UPLOADED] foo v0.0.1 to registry `alternative`
Expand Down Expand Up @@ -596,8 +596,8 @@ See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for
[VERIFYING] foo v0.0.1 ([ROOT]/foo)
[DOWNLOADING] crates ...
[DOWNLOADED] bar v0.0.1 (registry `dummy-registry`)
[COMPILING] bar v0.0.1
[COMPILING] foo v0.0.1 ([ROOT]/foo/target/package/foo-0.0.1)
[CHECKING] bar v0.0.1
[CHECKING] foo v0.0.1 ([ROOT]/foo/target/package/foo-0.0.1)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[UPLOADING] foo v0.0.1 ([ROOT]/foo)
[UPLOADED] foo v0.0.1 to registry `alternative`
Expand Down
2 changes: 1 addition & 1 deletion tests/testsuite/cargo_features.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -639,7 +639,7 @@ See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for
[PACKAGING] a v0.0.1 ([ROOT]/foo)
[PACKAGED] 4 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
[VERIFYING] a v0.0.1 ([ROOT]/foo)
[COMPILING] a v0.0.1 ([ROOT]/foo/target/package/a-0.0.1)
[CHECKING] a v0.0.1 ([ROOT]/foo/target/package/a-0.0.1)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[UPLOADING] a v0.0.1 ([ROOT]/foo)
[UPLOADED] a v0.0.1 to registry `crates-io`
Expand Down
4 changes: 2 additions & 2 deletions tests/testsuite/cross_publish.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ fn simple_cross_package() {
[PACKAGING] foo v0.0.0 ([ROOT]/foo)
[PACKAGED] 4 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
[VERIFYING] foo v0.0.0 ([ROOT]/foo)
[COMPILING] foo v0.0.0 ([ROOT]/foo/target/package/foo-0.0.0)
[CHECKING] foo v0.0.0 ([ROOT]/foo/target/package/foo-0.0.0)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s

"#]])
Expand Down Expand Up @@ -110,7 +110,7 @@ fn publish_with_target() {
[PACKAGING] foo v0.0.0 ([ROOT]/foo)
[PACKAGED] 4 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
[VERIFYING] foo v0.0.0 ([ROOT]/foo)
[COMPILING] foo v0.0.0 ([ROOT]/foo/target/package/foo-0.0.0)
[CHECKING] foo v0.0.0 ([ROOT]/foo/target/package/foo-0.0.0)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[UPLOADING] foo v0.0.0 ([ROOT]/foo)
[UPLOADED] foo v0.0.0 to registry `crates-io`
Expand Down
2 changes: 1 addition & 1 deletion tests/testsuite/features_namespaced.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1064,7 +1064,7 @@ fn publish() {
[UPDATING] crates.io index
[PACKAGED] 4 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
[VERIFYING] foo v0.1.0 ([ROOT]/foo)
[COMPILING] foo v0.1.0 ([ROOT]/foo/target/package/foo-0.1.0)
[CHECKING] foo v0.1.0 ([ROOT]/foo/target/package/foo-0.1.0)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[UPLOADING] foo v0.1.0 ([ROOT]/foo)
[UPLOADED] foo v0.1.0 to registry `crates-io`
Expand Down
16 changes: 8 additions & 8 deletions tests/testsuite/inheritable_workspace_fields.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ fn inherit_own_workspace_fields() {
[PACKAGING] foo v1.2.3 ([ROOT]/foo)
[PACKAGED] 6 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
[VERIFYING] foo v1.2.3 ([ROOT]/foo)
[COMPILING] foo v1.2.3 ([ROOT]/foo/target/package/foo-1.2.3)
[CHECKING] foo v1.2.3 ([ROOT]/foo/target/package/foo-1.2.3)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[UPLOADING] foo v1.2.3 ([ROOT]/foo)
[UPLOADED] foo v1.2.3 to registry `crates-io`
Expand Down Expand Up @@ -329,8 +329,8 @@ See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for
[UPDATING] crates.io index
[PACKAGED] 4 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
[VERIFYING] bar v0.2.0 ([ROOT]/foo)
[COMPILING] dep v0.1.2
[COMPILING] bar v0.2.0 ([ROOT]/foo/target/package/bar-0.2.0)
[CHECKING] dep v0.1.2
[CHECKING] bar v0.2.0 ([ROOT]/foo/target/package/bar-0.2.0)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[UPLOADING] bar v0.2.0 ([ROOT]/foo)
[UPLOADED] bar v0.2.0 to registry `crates-io`
Expand Down Expand Up @@ -496,8 +496,8 @@ See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for
[UPDATING] crates.io index
[PACKAGED] 4 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
[VERIFYING] bar v0.2.0 ([ROOT]/foo)
[COMPILING] dep v0.1.2
[COMPILING] bar v0.2.0 ([ROOT]/foo/target/package/bar-0.2.0)
[CHECKING] dep v0.1.2
[CHECKING] bar v0.2.0 ([ROOT]/foo/target/package/bar-0.2.0)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[UPLOADING] bar v0.2.0 ([ROOT]/foo)
[UPLOADED] bar v0.2.0 to registry `crates-io`
Expand Down Expand Up @@ -758,7 +758,7 @@ fn inherit_workspace_fields() {
`license` should be used if the package license can be expressed with a standard SPDX expression.
`license-file` should be used if the package uses a non-standard license.
See https://doc.rust-lang.org/cargo/reference/manifest.html#the-license-and-license-file-fields for more information.
[COMPILING] bar v1.2.3 ([ROOT]/foo/target/package/bar-1.2.3)
[CHECKING] bar v1.2.3 ([ROOT]/foo/target/package/bar-1.2.3)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[UPLOADING] bar v1.2.3 ([ROOT]/foo/bar)
[UPLOADED] bar v1.2.3 to registry `crates-io`
Expand Down Expand Up @@ -931,8 +931,8 @@ See https://doc.rust-lang.org/cargo/reference/manifest.html#package-metadata for
[UPDATING] crates.io index
[PACKAGED] 4 files, [FILE_SIZE]B ([FILE_SIZE]B compressed)
[VERIFYING] bar v0.2.0 ([ROOT]/foo/bar)
[COMPILING] dep v0.1.2
[COMPILING] bar v0.2.0 ([ROOT]/foo/target/package/bar-0.2.0)
[CHECKING] dep v0.1.2
[CHECKING] bar v0.2.0 ([ROOT]/foo/target/package/bar-0.2.0)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[UPLOADING] bar v0.2.0 ([ROOT]/foo/bar)
[UPLOADED] bar v0.2.0 to registry `crates-io`
Expand Down
Loading
Loading