Fix split-debuginfo support detection #11347

kamirr · 2022-11-06T23:32:17Z

What does this PR try to resolve?

cargo assumed that if -Csplit-debuginfo=packed worked, all values would be correct. This however is not the case -- as of Rust 1.65.0, rustc supports packed, but not unpacked or off on Windows. Because of this, having split-debuginfo="unpacked" on Windows has caused builds to fail, as cargo assumed that the option is fine (split-debuginfo=packed worked), but rustc then failed when being passed -Csplit-debuginfo=unpacked.

How should we test and review this PR?

Consider an empty project with the following change to Cargo.toml:

[profile.dev]
split-debuginfo="unpacked"

cargo +1.64.0 build will work, but cargo +1.65.0 build will fail with the following error message:

PS C:\REDACTED> cargo build
   Compiling tmp v0.1.0 (C:\REDACTED)
error: `-Csplit-debuginfo=unpacked` is unstable on this platform

error: could not compile `tmp` due to previous error

With this patch and 1.65.0 rustc, cargo will ignore all split-debuginfo settings, but with rust-lang/rust#104104 rustc (approved, awaiting merge), it will properly detect supported values for split-debuginfo and only ignore those that are unsupported.

rustbot · 2022-11-06T23:32:21Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

weihanglo

Good call on this! Thank you for the pull request.

src/cargo/core/compiler/build_context/target_info.rs

Add split-debuginfo print option This option prints all supported values for `-Csplit-debuginfo=..`, i.e. only stable ones on stable/beta and all of them on nightly/dev. Motivated by 1.65.0 regression causing builds with the following entry in `Cargo.toml` to fail on Windows: ```toml [profile.dev] split-debuginfo = "unpacked" ``` See rust-lang/cargo#11347 for details. This will lead to closing rust-lang#103976.

cargo assumed that if -Csplit-debuginfo=packed worked, all values would be correct. This however is not the case -- as of Rust 1.65.0, rustc supports packed, but not unpacked or off on Windows. Because of this, having split-debuginfo="unpacked" on Windows has caused builds to fail, as cargo assumed that the option is fine (split-debuginfo=packed worked), but rustc then failed when being passed -Csplit-debuginfo=unpacked. This patch invokes rustc with the --print=split-debuginfo to query supported values and ignores the Cargo.toml entry if not supported.

weihanglo

Glad to see your patch for rustc going to be landed!

Two possible enhancements could be done in this pull request:

I found there is no test around this feature. How about writing some basics test validating we're passing correct -Csplit-debuginfo to rustc? Maybe we could exclude Windows from these tests at this time.
~~Should we add a warning if given split-debuginfo option is not supported? We can spin off this to a separate issue though.~~ Might be too noisy. Leave it.

src/cargo/core/compiler/mod.rs

kamirr · 2022-11-08T12:10:42Z

macOS tests fail due to missing .dSYM artifacts, this is expected as explained in the top-level comment (we ignore split-debuginfo with stable rustc, but will handle it correctly on master after rust-lang/rust#104143 is merged). Does that require any action from my side?

I found there is no test around this feature. How about writing some basics test validating we're passing correct -Csplit-debuginfo to rustc? Maybe we could exclude Windows from these tests at this time.

While I can write such tests, they won't pass now anyway due to reasons outlined above. What do you think about it?

Add split-debuginfo print option This option prints all supported values for `-Csplit-debuginfo=..`, i.e. only stable ones on stable/beta and all of them on nightly/dev. Motivated by 1.65.0 regression causing builds with the following entry in `Cargo.toml` to fail on Windows: ```toml [profile.dev] split-debuginfo = "unpacked" ``` See rust-lang/cargo#11347 for details. This will lead to closing rust-lang#103976.

weihanglo · 2022-11-08T16:04:18Z

Does that require any action from my side?

No. Not really. I guess we are required to wait for your rustc patch hitting stable (presumably 1.67). Only after that this pull request can be merged.

Add split-debuginfo print option This option prints all supported values for `-Csplit-debuginfo=..`, i.e. only stable ones on stable/beta and all of them on nightly/dev. Motivated by 1.65.0 regression causing builds with the following entry in `Cargo.toml` to fail on Windows: ```toml [profile.dev] split-debuginfo = "unpacked" ``` See rust-lang/cargo#11347 for details. This will lead to closing rust-lang#103976.

kamirr · 2022-11-09T09:15:53Z

The patch was merged, so it's indeed on track to 1.67. One question about cargo release cycle -- if we get this PR merged after 1.67 is released, will that mean that the patched cargo will be released in 1.68, or later?

weihanglo · 2022-11-09T10:21:50Z

The patch was merged, so it's indeed on track to 1.67. One question about cargo release cycle -- if we get this PR merged after 1.67 is released, will that mean that the patched cargo will be released in 1.68, or later?

Cargo, and several other official components, follow Rust's train release model. The release schedule is found here. That is, if we get this merged after 1.67 is released, it should hit stable channel on 1.69.

ehuss · 2022-12-02T15:04:33Z

Just to be clear, will this be blocked until after Jan 26?

It's a bit unfortunate to still incur a second rustc invocation. I wonder if some day in the future we should consider adding some kind of query command to rustc that will print a json blob with all the information we need once.

weihanglo · 2022-12-02T16:04:33Z

Just to be clear, will this be blocked until after Jan 26?

It's a bit unfortunate to still incur a second rustc invocation. I wonder if some day in the future we should consider adding some kind of query command to rustc that will print a json blob with all the information we need once.

I feel like it can be merged into the other with some tricks. But yeah, it is blocked until the change of rustc get stabilized.

kamirr · 2023-01-26T15:46:49Z

@weihanglo this patch should now work with stable Rust, can you re-run CI?

weihanglo · 2023-01-26T15:47:59Z

@bors try

Fix split-debuginfo support detection ### What does this PR try to resolve? cargo assumed that if `-Csplit-debuginfo=packed` worked, all values would be correct. This however is not the case -- as of Rust 1.65.0, rustc supports `packed`, but not `unpacked` or `off` on Windows. Because of this, having `split-debuginfo="unpacked`" on Windows has caused builds to fail, as cargo assumed that the option is fine (`split-debuginfo=packed` worked), but rustc then failed when being passed `-Csplit-debuginfo=unpacked`. ### How should we test and review this PR? Consider an empty project with the following change to `Cargo.toml`: ```toml [profile.dev] split-debuginfo="unpacked" ``` `cargo +1.64.0 build` will work, but `cargo +1.65.0 build` will fail with the following error message: ``` PS C:\REDACTED> cargo build Compiling tmp v0.1.0 (C:\REDACTED) error: `-Csplit-debuginfo=unpacked` is unstable on this platform error: could not compile `tmp` due to previous error ``` With this patch and 1.65.0 rustc, cargo will ignore all `split-debuginfo` settings, but with rust-lang/rust#104104 rustc (approved, awaiting merge), it will properly detect supported values for `split-debuginfo` and only ignore those that are unsupported.

bors · 2023-01-26T15:48:07Z

⌛ Trying commit fe95630 with merge 641a4e1...

bors · 2023-01-26T17:48:06Z

💥 Test timed out

weihanglo

This try looks good https://github.com/rust-lang/cargo/actions/runs/4016766934. Thank you for the contribution and long waiting!

weihanglo · 2023-01-26T18:14:00Z

@bors r+

BTW, to optimize it a bit, we could explore a way merging multiple rustc calls into one.

bors · 2023-01-26T18:14:03Z

📌 Commit fe95630 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2023-01-26T18:14:12Z

⌛ Testing commit fe95630 with merge c03cc59...

bors · 2023-01-26T19:14:48Z

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

…=weihanglo Fix split-debuginfo support detection ### What does this PR try to resolve? cargo assumed that if `-Csplit-debuginfo=packed` worked, all values would be correct. This however is not the case -- as of Rust 1.65.0, rustc supports `packed`, but not `unpacked` or `off` on Windows. Because of this, having `split-debuginfo="unpacked`" on Windows has caused builds to fail, as cargo assumed that the option is fine (`split-debuginfo=packed` worked), but rustc then failed when being passed `-Csplit-debuginfo=unpacked`. ### How should we test and review this PR? Consider an empty project with the following change to `Cargo.toml`: ```toml [profile.dev] split-debuginfo="unpacked" ``` `cargo +1.64.0 build` will work, but `cargo +1.65.0 build` will fail with the following error message: ``` PS C:\REDACTED> cargo build Compiling tmp v0.1.0 (C:\REDACTED) error: `-Csplit-debuginfo=unpacked` is unstable on this platform error: could not compile `tmp` due to previous error ``` With this patch and 1.65.0 rustc, cargo will ignore all `split-debuginfo` settings, but with rust-lang/rust#104104 rustc (approved, awaiting merge), it will properly detect supported values for `split-debuginfo` and only ignore those that are unsupported.

[beta-1.68] Backport fixes of split-debuginfo detection Beta backports: * #11347 — Fix split-debuginfo support detection * #11633 — Reduce target info rustc query calls In order to make CI pass, the following PR are also cherry-picked: * #11619 * #11609 * #11610

18 commits in 3c5af6bed9a1a243a693e8e22ee2486bd5b82a6c..e84a7928d93a31f284b497c214a2ece69b4d7719 2023-01-24 15:48:15 +0000 to 2023-01-31 22:18:09 +0000 - chore: Add autolabel for `Command-*` labels (rust-lang/cargo#11664) - Update cross test instructions for aarch64-apple-darwin (rust-lang/cargo#11663) - Make cargo install report needed features (rust-lang/cargo#11647) - docs(contrib): Remove out-of-date process step (rust-lang/cargo#11662) - Do not error for `auth-required: true` without `-Z sparse-registry` (rust-lang/cargo#11661) - Warn on commits to non-default branches. (rust-lang/cargo#11655) - Avoid saving the same future_incompat warning multiple times (rust-lang/cargo#11648) - Mention current default value in `publish.timeout` docs (rust-lang/cargo#11652) - Make cargo aware of dwp files. (rust-lang/cargo#11572) - Reduce target info rustc query calls (rust-lang/cargo#11633) - Bump to 0.70.0; update changelog (rust-lang/cargo#11640) - Enable sparse protocol in CI (rust-lang/cargo#11632) - Fix split-debuginfo support detection (rust-lang/cargo#11347) - refactor(toml): Move `TomlWorkspaceDependency` out of `TomlDependency` (rust-lang/cargo#11565) - book: describe how the current resolver sometimes duplicates deps (rust-lang/cargo#11604) - `cargo add` check `[dependencies]` order without considering the dotted item (rust-lang/cargo#11612) - Link CoC to www.rust-lang.org/conduct.html (rust-lang/cargo#11622) - Add more labels to triagebot (rust-lang/cargo#11621)

Update cargo 18 commits in 3c5af6bed9a1a243a693e8e22ee2486bd5b82a6c..e84a7928d93a31f284b497c214a2ece69b4d7719 2023-01-24 15:48:15 +0000 to 2023-01-31 22:18:09 +0000 - chore: Add autolabel for `Command-*` labels (rust-lang/cargo#11664) - Update cross test instructions for aarch64-apple-darwin (rust-lang/cargo#11663) - Make cargo install report needed features (rust-lang/cargo#11647) - docs(contrib): Remove out-of-date process step (rust-lang/cargo#11662) - Do not error for `auth-required: true` without `-Z sparse-registry` (rust-lang/cargo#11661) - Warn on commits to non-default branches. (rust-lang/cargo#11655) - Avoid saving the same future_incompat warning multiple times (rust-lang/cargo#11648) - Mention current default value in `publish.timeout` docs (rust-lang/cargo#11652) - Make cargo aware of dwp files. (rust-lang/cargo#11572) - Reduce target info rustc query calls (rust-lang/cargo#11633) - Bump to 0.70.0; update changelog (rust-lang/cargo#11640) - Enable sparse protocol in CI (rust-lang/cargo#11632) - Fix split-debuginfo support detection (rust-lang/cargo#11347) - refactor(toml): Move `TomlWorkspaceDependency` out of `TomlDependency` (rust-lang/cargo#11565) - book: describe how the current resolver sometimes duplicates deps (rust-lang/cargo#11604) - `cargo add` check `[dependencies]` order without considering the dotted item (rust-lang/cargo#11612) - Link CoC to www.rust-lang.org/conduct.html (rust-lang/cargo#11622) - Add more labels to triagebot (rust-lang/cargo#11621) r? `@ghost`

rustbot assigned ehuss Nov 6, 2022

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 6, 2022

This was referenced Nov 6, 2022

Fix CI failing since rustc 1.65.0 golemfactory/yagna#2280

Merged

CI fails on master golemfactory/yagna#2279

Closed

re-enable split-debuginfo golemfactory/yagna#2283

Open

weihanglo reviewed Nov 7, 2022

View reviewed changes

src/cargo/core/compiler/build_context/target_info.rs Outdated Show resolved Hide resolved

kamirr mentioned this pull request Nov 7, 2022

Add split-debuginfo print option rust-lang/rust#104104

Merged

kamirr force-pushed the fix-split-debuginfo-windows branch from 329c9d9 to 856ed94 Compare November 8, 2022 10:40

kamirr requested a review from weihanglo November 8, 2022 11:00

Extract logic to TargetInfo::supports_debuginfo_split

fe95630

weihanglo reviewed Nov 8, 2022

View reviewed changes

src/cargo/core/compiler/mod.rs Outdated Show resolved Hide resolved

src/cargo/core/compiler/mod.rs Outdated Show resolved Hide resolved

kamirr commented Nov 8, 2022

View reviewed changes

src/cargo/core/compiler/mod.rs Outdated Show resolved Hide resolved

kamirr requested a review from weihanglo November 8, 2022 14:57

fvaleye mentioned this pull request Nov 8, 2022

Fix cargo clippy issues 0.1.65 in Rust delta-io/delta-rs#923

Merged

weihanglo added S-blocked and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 8, 2022

paul-hansen mentioned this pull request Nov 30, 2022

-Csplit-debuginfo=unpacked is unstable on this platform emilk/egui#2368

Open

neezer mentioned this pull request Dec 31, 2022

error: failed to wait on curl Multi #11435

Open

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked labels Jan 26, 2023

weihanglo approved these changes Jan 26, 2023

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 Jan 26, 2023

bors merged commit c03cc59 into rust-lang:master Jan 26, 2023

weihanglo mentioned this pull request Jan 27, 2023

Reduce target info rustc query calls #11633

Merged

ehuss added the beta-nominated Nominated to backport to the beta branch. label Jan 28, 2023

weihanglo mentioned this pull request Jan 29, 2023

[beta-1.68] Backport fixes of split-debuginfo detection #11649

Merged

weihanglo mentioned this pull request Feb 1, 2023

Update cargo rust-lang/rust#107541

Merged

ehuss modified the milestones: 1.69.0, 1.68.0 Feb 2, 2023

ehuss mentioned this pull request Jun 14, 2023

In 1.64 split-debuginfo = "unpacked" was ignored on Windows while in 1.65 it fails to build rust-lang/rust#103976

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix split-debuginfo support detection #11347

Fix split-debuginfo support detection #11347

kamirr commented Nov 6, 2022 •

edited

Loading

rustbot commented Nov 6, 2022

weihanglo left a comment

weihanglo left a comment •

edited

Loading

kamirr commented Nov 8, 2022 •

edited

Loading

weihanglo commented Nov 8, 2022

kamirr commented Nov 9, 2022 •

edited

Loading

weihanglo commented Nov 9, 2022

ehuss commented Dec 2, 2022

weihanglo commented Dec 2, 2022

kamirr commented Jan 26, 2023

weihanglo commented Jan 26, 2023

bors commented Jan 26, 2023

bors commented Jan 26, 2023

weihanglo left a comment

weihanglo commented Jan 26, 2023

bors commented Jan 26, 2023

bors commented Jan 26, 2023

bors commented Jan 26, 2023

Fix split-debuginfo support detection #11347

Fix split-debuginfo support detection #11347

Conversation

kamirr commented Nov 6, 2022 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

rustbot commented Nov 6, 2022

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo left a comment • edited Loading

Choose a reason for hiding this comment

kamirr commented Nov 8, 2022 • edited Loading

weihanglo commented Nov 8, 2022

kamirr commented Nov 9, 2022 • edited Loading

weihanglo commented Nov 9, 2022

ehuss commented Dec 2, 2022

weihanglo commented Dec 2, 2022

kamirr commented Jan 26, 2023

weihanglo commented Jan 26, 2023

bors commented Jan 26, 2023

bors commented Jan 26, 2023

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo commented Jan 26, 2023

bors commented Jan 26, 2023

bors commented Jan 26, 2023

bors commented Jan 26, 2023

kamirr commented Nov 6, 2022 •

edited

Loading

weihanglo left a comment •

edited

Loading

kamirr commented Nov 8, 2022 •

edited

Loading

kamirr commented Nov 9, 2022 •

edited

Loading