perf(toml): Avoid inferring when targets are known #13849

epage · 2024-05-02T14:51:57Z

What does this PR try to resolve?

We read the file system to infer two different data points

Implicit targets
Implicit path values for targets

I took a shortcut for this case and recognize the scenario where we can
bypass both and do so.
I went with a bypass, rather than this being integrating into the
inferring code because the inferring code is complex and I didn't want
to add to it further in isolating the inferring to only when its needed.

The validation gets duplicated because having it in the middle of the resolve code provides a better user experience and it would be messy to add the conditionals to get all of that working. I at least worked to keep the duplicated validation close to each other.

How should we test and review this PR?

Additional information

We read the file system to infer two different data points - Implicit targets - Implicit `path` values for targets I took a shortcut for this case and recognize the scenario where we can bypass both and do so. I went with a bypass, rather than this being integrating into the inferring code because the inferring code is complex and I didn't want to add to it further in isolating the inferring to only when its needed.

rustbot · 2024-05-02T14:52:02Z

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

weihanglo · 2024-05-02T19:40:49Z

Looks pretty neat. Thanks!

@bors r+

bors · 2024-05-02T19:40:52Z

📌 Commit f5892f2 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2024-05-02T19:41:59Z

⌛ Testing commit f5892f2 with merge 9e57a8d...

bors · 2024-05-02T20:10:31Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 9e57a8d to master...

Update cargo 18 commits in 6087566b3fa73bfda29702632493e938b12d19e5..05364cb2f61a2c2b091e061c1f42b207dfb5f81f 2024-04-30 20:45:20 +0000 to 2024-05-03 16:48:59 +0000 - chore(deps): update msrv (3 versions) to v1.76 (rust-lang/cargo#13857) - Stabilize `-Zcheck-cfg` as always enabled (rust-lang/cargo#13571) - fix(lints): Prevent inheritance from bring exposed for published packages (rust-lang/cargo#13852) - refactor: remove unnecessary branch for link binary on macOS (rust-lang/cargo#13851) - perf(toml): Avoid inferring when targets are known (rust-lang/cargo#13849) - Update continuous-integration.md: Include CircleCI reference (rust-lang/cargo#13850) - chore(deps): update msrv (1 version) to v1.78 (rust-lang/cargo#13848) - Workaround copying file returning EAGAIN on ZFS on mac OS (rust-lang/cargo#13845) - Clean package perf improvements (rust-lang/cargo#13818) - fix(toml): Validate crates_types/proc-macro for bin like others (rust-lang/cargo#13841) - fix(toml): On 2024 Edition, disallow ignored `default-features` when inheriting (rust-lang/cargo#13839) - chore(ci): Ignore openssl deps (rust-lang/cargo#13840) - fix(lints): Remove ability to specify `-` in lint name (rust-lang/cargo#13837) - fix(resolver): Treat unset MSRV as compatible (rust-lang/cargo#13791) - fix(toml): Don't lose 'public' when inheriting a dep (rust-lang/cargo#13836) - chore(deps): update compatible (rust-lang/cargo#13834) - Error when unstable lints are specified but not enabled (rust-lang/cargo#13805) - fix(lint): Warn not Error on unsupported lint tool (rust-lang/cargo#13833) r? ghost Note: this includes the fix that was beta backported in rust-lang#124647

Update cargo 18 commits in 6087566b3fa73bfda29702632493e938b12d19e5..05364cb2f61a2c2b091e061c1f42b207dfb5f81f 2024-04-30 20:45:20 +0000 to 2024-05-03 16:48:59 +0000 - chore(deps): update msrv (3 versions) to v1.76 (rust-lang/cargo#13857) - Stabilize `-Zcheck-cfg` as always enabled (rust-lang/cargo#13571) - fix(lints): Prevent inheritance from bring exposed for published packages (rust-lang/cargo#13852) - refactor: remove unnecessary branch for link binary on macOS (rust-lang/cargo#13851) - perf(toml): Avoid inferring when targets are known (rust-lang/cargo#13849) - Update continuous-integration.md: Include CircleCI reference (rust-lang/cargo#13850) - chore(deps): update msrv (1 version) to v1.78 (rust-lang/cargo#13848) - Workaround copying file returning EAGAIN on ZFS on mac OS (rust-lang/cargo#13845) - Clean package perf improvements (rust-lang/cargo#13818) - fix(toml): Validate crates_types/proc-macro for bin like others (rust-lang/cargo#13841) - fix(toml): On 2024 Edition, disallow ignored `default-features` when inheriting (rust-lang/cargo#13839) - chore(ci): Ignore openssl deps (rust-lang/cargo#13840) - fix(lints): Remove ability to specify `-` in lint name (rust-lang/cargo#13837) - fix(resolver): Treat unset MSRV as compatible (rust-lang/cargo#13791) - fix(toml): Don't lose 'public' when inheriting a dep (rust-lang/cargo#13836) - chore(deps): update compatible (rust-lang/cargo#13834) - Error when unstable lints are specified but not enabled (rust-lang/cargo#13805) - fix(lint): Warn not Error on unsupported lint tool (rust-lang/cargo#13833) r? ghost Note: this includes the fix that was beta backported in #124647

Turbo87 · 2024-06-20T09:30:40Z

src/cargo/util/toml/targets.rs

+fn is_resolved(toml_targets: Option<&Vec<TomlTarget>>, autodiscover: Option<bool>) -> bool {
+    if autodiscover != Some(false) {
+        return false;
+    }
+
+    let Some(toml_targets) = toml_targets else {
+        return true;
+    };
+    toml_targets
+        .iter()
+        .all(|t| t.name.is_some() && t.path.is_some())
+}


does this mean that if in the Cargo.toml (whether normalized or not) all [[bin]] entries contain a name and path field then the auto-discovery of additional bins is disabled? that seems to have potential for false positives and would not match https://doc.rust-lang.org/cargo/reference/cargo-targets.html#target-auto-discovery. last time this behavior was changed it was done at an edition boundary 🤔

We only get here if autodiscover is explicitly disabled. We reuse autodiscover logic is reused to fill in name or path and this is checking to see if that is needed.

If there is a behavior change you noticed, could you be more explicit about what it is?

We only get here if autodiscover is explicitly disabled.

ahhh, you're right. I misinterpreted the double negation in the code. in that case: sorry for the noise! :)

I just published all of my packages using Cargo 1.80 and wanted to see the effect of the enumerating targets during publish (rust-lang#13849).

chore: Update dependencies I just published all of my packages using Cargo 1.80 and wanted to see the effect of the enumerating targets during publish (#13849).

PR rust-lang#5335 added `autobins`, etc for rust-lang#5330. Nowhere in there is discussion of `autolib`. Cargo script disables support for additional build-targets by disabling discovery. Except we don't have a way to disable discovery of `autolib`, leading to rust-lang#14476. By adding `autolib`, we can continue in that direction. This also allows us to bypass inferring of libs on published packages, like all other build-targets which were handled in rust-lang#13849. As this seems fairly low controversy, this insta-stabilizes the field. In prior versions of Cargo, users will get an "unused manifest key" warning. For packags where this is set by `cargo publish`, the warning will be suppressed and things will work as normal. For `cargo vendor`, the same except there will be some churn in the vendored source as this field will now be set. For local development, it should be rare to set `autolib` so the lack of error by discovering a file when this is set shouldn't be a problem. Fixes rust-lang#14476

feat(toml): Add `autolib` ### What does this PR try to resolve? PR #5335 added `autobins`, etc for #5330. Nowhere in there is discussion of `autolib`. Cargo script disables support for additional build-targets by disabling discovery. Except we don't have a way to disable discovery of `autolib`, leading to #14476. By adding `autolib`, we can continue in that direction. This also allows us to bypass inferring of libs on published packages, like all other build-targets which were handled in #13849. Fixes #14476 ### How should we test and review this PR? ### Additional information As this seems fairly low controversy, this insta-stabilizes the field. In prior versions of Cargo, users will get an "unused manifest key" warning. For packages where this is set by `cargo publish`, the warning will be suppressed and things will work as normal. For `cargo vendor`, the same except there will be some churn in the vendored source as this field will now be set. For local development, it should be rare to set `autolib` so the lack of error by discovering a file when this is set shouldn't be a problem.

epage added 2 commits May 1, 2024 19:42

refactor(toml): Defer inferred discovery

3740bbb

rustbot assigned ehuss May 2, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 2, 2024

weihanglo approved these changes May 2, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 2, 2024

bors merged commit 9e57a8d into rust-lang:master May 2, 2024
21 checks passed

Muscraft mentioned this pull request May 3, 2024

Update cargo rust-lang/rust#124684

Merged

rustbot added this to the 1.80.0 milestone May 4, 2024

epage deleted the no-infer branch May 17, 2024 19:30

Turbo87 reviewed Jun 20, 2024

View reviewed changes

epage mentioned this pull request Jul 10, 2024

The file path in the diagnostic message contains duplicate separators on windows #14227

Closed

epage mentioned this pull request Jul 25, 2024

chore: Update dependencies #14299

Merged

epage added a commit to epage/cargo that referenced this pull request Jul 25, 2024

chore: Update dependencies

8debb54

I just published all of my packages using Cargo 1.80 and wanted to see the effect of the enumerating targets during publish (rust-lang#13849).

bors added a commit that referenced this pull request Jul 26, 2024

Auto merge of #14299 - epage:deps, r=weihanglo

9a0c8b3

chore: Update dependencies I just published all of my packages using Cargo 1.80 and wanted to see the effect of the enumerating targets during publish (#13849).

epage mentioned this pull request Sep 24, 2024

feat(toml): Add autolib #14591

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(toml): Avoid inferring when targets are known #13849

perf(toml): Avoid inferring when targets are known #13849

epage commented May 2, 2024

rustbot commented May 2, 2024

weihanglo commented May 2, 2024

bors commented May 2, 2024

bors commented May 2, 2024

bors commented May 2, 2024

Turbo87 Jun 20, 2024

epage Jun 20, 2024 •

edited

Loading

Turbo87 Jun 20, 2024

perf(toml): Avoid inferring when targets are known #13849

perf(toml): Avoid inferring when targets are known #13849

Conversation

epage commented May 2, 2024

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented May 2, 2024

weihanglo commented May 2, 2024

bors commented May 2, 2024

bors commented May 2, 2024

bors commented May 2, 2024

Turbo87 Jun 20, 2024

Choose a reason for hiding this comment

epage Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Turbo87 Jun 20, 2024

Choose a reason for hiding this comment

epage Jun 20, 2024 •

edited

Loading