Adding display of which target failed to compile #11636

aortizpimentel · 2023-01-27T19:16:50Z

Attached example.zip is the same as the one mentioned on the issue. You could use it to test the fix by using the new built cargo:
./cargo build --manifest-path C:\Rust\example_cargo_error\Cargo.toml

Before fixing:

error: could not compile `example` due to 2 previous errors
warning: build failed, waiting for other jobs to finish...

After fixing:

error: could not compile `example` (build script)  due to 2 previous errors
warning: build failed, waiting for other jobs to finish...

example.zip

rustbot · 2023-01-27T19:16:55Z

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

Thanks for the pull request!

What do you think of following what #10834 (comment) told? That is "doing what a warning already does".

Besides, here is a guide for writing tests for cargo. Adding tests to verify the change is beneficial for reviewers to look into the change, making it more likely to get merged.

Edit: The Cargo project prefers a descriptive title for the pull request. That makes it easier when scrolling back to search a PR in the haystack.

weihanglo · 2023-01-27T21:06:12Z

r? @weihanglo

ehuss · 2023-01-28T00:04:25Z

Can you edit the title of the PR to say what it does instead of referencing an issue number? Perhaps something like "Display which target failed to compile", or something similarly descriptive. That can make it easier to understand what the PR does without needing to reference something else.

aortizpimentel · 2023-01-28T07:05:43Z

What do you think of following what #10834 (comment) told? That is "doing what a warning already does".

Thanks for replying @weihanglo, sorry but i'm new to Rust & OS world 😅 I've doubts about the first sentence. I think i just included what the comment said, adding the "(unit.target.description_named)" to the display output, so the message would output:

error: could not compile `example` (build script) due to

What am i missing here? 🥲

I'm also checking the testing guide you pointed out, in this case, i've seen other tests that would get invalidated by this PR, for example here . Should i also update those tests?

weihanglo · 2023-01-28T09:46:53Z

What am i missing here? 🥲

Nothing wrong with your current fix. I am just suggesting the already existing approarch. It is effectively your solution plus more compile mode information. It would be great to have a consistent style between warning and error messages.

cargo/src/cargo/core/compiler/job_queue.rs

Lines 1265 to 1273 in ca0557a

    
           let mut message = format!("`{}` ({}", unit.pkg.name(), unit.target.description_named()); 
        
           if unit.mode.is_rustc_test() && !(unit.target.is_test() || unit.target.is_bench()) { 
        
               message.push_str(" test"); 
        
           } else if unit.mode.is_doc_test() { 
        
               message.push_str(" doctest"); 
        
           } else if unit.mode.is_doc() { 
        
               message.push_str(" doc"); 
        
           } 
        
           message.push_str(") generated ");

I'm also checking the testing guide you pointed out, in this case, i've seen other tests that would get invalidated by this PR, for example here. Should i also update those tests?

Ahh! They are the missing pieces in the contributing guide. We should include the expectation of a pull request in it. And yeah, a pull is supposed to pass all test suites before merge. That is to say, if your fix affects some other error messages beyond your expectation, you should figure out why it has changed. Then decide to either fix them or fix your patch.

This reverts commit 09f3293.

weihanglo · 2023-03-02T13:44:38Z

Ping @aortizpimentel. Just checking in to see if you are still interested in working on this. Let us know if you had any questions.

aortizpimentel · 2023-03-02T13:51:36Z

Ping @aortizpimentel. Just checking in to see if you are still interested in working on this. Let us know if you had any questions.

Hi @weihanglo, let me try to resume this, problably this saturday morning. The problem i've encountered so far was the "overwhelming" failed tests that i've got. I think the problem was the previous message hardcoded on several tests if i can remember well.

weihanglo · 2023-03-02T14:10:02Z

That was intentionally hardcoded. Today we could write tests in UI test flavour, so that we don't get the pain of updating assertion strings — they will be done automatically. However, we haven't yet managed to migrate tests from the old style, and some of them should not be migrated. Contributor experience is also a part we want to improve, so if you got some please share thoughts with us.

aortizpimentel · 2023-03-03T06:43:55Z

With current build, there's still 86 tests to review & fix 😅

test result: FAILED. 2504 passed; 86 failed; 165 ignored; 0 measured; 0 filtered out; finished in 396.84s

weihanglo · 2023-03-03T09:08:06Z

With current build, there's still 86 tests to review & fix 😅

test result: FAILED. 2504 passed; 86 failed; 165 ignored; 0 measured; 0 filtered out; finished in 396.84s

Can you figure out why it failed on so many tests? It didn't look like that many in Cargo CI pipeline (only 8 failures), and only 11 failures on my machine with commit d1309e3. You could leave error messages here or ping me on Zuilp if that make you feel more comfortable :)

BTW, before fixing assertions, is it possible to extract this piece of code and reuse it for both warnings and errors? That will make messages more consistent. Thank you!

cargo/src/cargo/core/compiler/job_queue/mod.rs

Lines 1003 to 1010 in aaf08a0

    
           let mut message = format!("`{}` ({}", unit.pkg.name(), unit.target.description_named()); 
        
           if unit.mode.is_rustc_test() && !(unit.target.is_test() || unit.target.is_bench()) { 
        
               message.push_str(" test"); 
        
           } else if unit.mode.is_doc_test() { 
        
               message.push_str(" doctest"); 
        
           } else if unit.mode.is_doc() { 
        
               message.push_str(" doc"); 
        
           }

aortizpimentel · 2023-03-03T13:06:32Z

This is weird, i've run into this issue #6943. But there's nothing running, as i've tried to restart also.

Deleting/Moving each conflict is manual + time consuming and deleting all target folder takes so much also (but auto)

aortizpimentel · 2023-03-03T17:18:36Z

Hi @weihanglo, it's ok now?

src/cargo/core/compiler/mod.rs

weihanglo

Thanks for extracting function for reuse. That looks pretty in general! I left some minor style suggestions.

Also, before this gets merged, could you

Assert each error message verbatim if possible. Don't use [..] to omit them.
Clean up commit history. You don't need to squash all into one.
I personally follow the creed of “atomic commits” and cleanup intermediate commits generated during development, such as “run rustfmt” or “revert X”. Those commits make little sense to others in collaboration.

weihanglo · 2023-03-04T08:15:50Z

tests/testsuite/build.rs

@@ -639,7 +639,7 @@ fn cargo_compile_with_invalid_code() {

    p.cargo("build")
        .with_status(101)
-        .with_stderr_contains("[ERROR] could not compile `foo` due to previous error\n")
+        .with_stderr_contains("[ERROR] could not compile `foo` [..] due to previous error\n")


I feel like it shouldn't be omitted. Could you assert the actual error message? And other tests as well. Thanks!

Suggested change

.with_stderr_contains("[ERROR] could not compile `foo` [..] due to previous error\n")

.with_stderr_contains("[ERROR] could not compile `foo` (bin "foo") due to previous error\n")

weihanglo · 2023-03-04T08:56:00Z

src/cargo/core/compiler/mod.rs

+/// Provides a common compilation errors/warnings base message
+fn compile_error_msg(
+    msg_prefix: Option<&str>,
+    name: &str,
+    target: &Target,
+    mode: &CompileMode,
+) -> String {
+    let mut message: String = "".to_string();
+
+    match msg_prefix {
+        None => {}
+        Some(m) => message.push_str(m),
+    }
+    message.push_str(&format!("`{}` ({}", name, target.description_named()));
+    if mode.is_rustc_test() && !(target.is_test() || target.is_bench()) {
+        message.push_str(" test");
+    } else if mode.is_doc_test() {
+        message.push_str(" doctest");
+    } else if mode.is_doc() {
+        message.push_str(" doc");
+    }
+    message.push_str(") generated ");
+    message


To make the purpose of this fn clearer, we can scope it to provide a descriptive package name. That also reduce one function argument.

The current message looks like
could not compile `foo` (bin "foo") generated due to previous error
which generated is weird being there, and an extra space appears. I think it should be
- could not compile `foo` (bin "foo") generated due to previous error + could not compile `foo` (bin "foo") due to previous error

How about change it to this one?

Suggested change

/// Provides a common compilation errors/warnings base message

fn compile_error_msg(

msg_prefix: Option<&str>,

name: &str,

target: &Target,

mode: &CompileMode,

) -> String {

let mut message: String = "".to_string();

match msg_prefix {

None => {}

Some(m) => message.push_str(m),

}

message.push_str(&format!("`{}` ({}", name, target.description_named()));

if mode.is_rustc_test() && !(target.is_test() || target.is_bench()) {

message.push_str(" test");

} else if mode.is_doc_test() {

message.push_str(" doctest");

} else if mode.is_doc() {

message.push_str(" doc");

}

message.push_str(") generated ");

message

/// Provides a package name with descriptive target information,

/// e.g., `` `foo` (bin "bar" test) ``, `` `foo` (lib doctest) ``.

fn descriptive_pkg_name(name: &str, target: &Target, mode: &CompileMode) -> String {

let desc_name = target.description_named();

let mode = if mode.is_rustc_test() && !(target.is_test() || target.is_bench()) {

" test"

} else if mode.is_doc_test() {

" doctest"

} else if mode.is_doc() {

" doc"

} else {

""

};

format!("`{name}` ({desc_name}{mode})")

}

weihanglo · 2023-03-04T08:57:48Z

src/cargo/core/compiler/mod.rs

+                    let mut message =
+                        compile_error_msg(Some("could not compile "), &name, &target, &mode);
+                    message.push_str(&errors);
+                    message.push_str(&warnings);
+                    message


Can do this if using the suggested descriptive_pkg_name function.

Suggested change

let mut message =

compile_error_msg(Some("could not compile "), &name, &target, &mode);

message.push_str(&errors);

message.push_str(&warnings);

message

let name = descriptive_pkg_name(&name, &target, &mode);

format!("could not compile {name}{errors}{warnings}")

weihanglo · 2023-03-04T08:58:45Z

src/cargo/core/compiler/job_queue/mod.rs

-            message.push_str(" doc");
-        }
-        message.push_str(") generated ");
+        let mut message = compile_error_msg(None, &unit.pkg.name(), &unit.target, &unit.mode);


Can do this if using the suggested descriptive_pkg_name function.

Suggested change

let mut message = compile_error_msg(None, &unit.pkg.name(), &unit.target, &unit.mode);

let mut message = descriptive_pkg_name(&unit.pkg.name(), &unit.target, &unit.mode);

message.push_str(" generated ");

aortizpimentel · 2023-03-05T06:16:18Z

Can you figure out why it failed on so many tests?

They're failing by "os error 3" (windows paths problem maybe?) but there's also something weird apening, this test was run OK and a few seconds later re-executed (no changes) and it's failing by permissionDenied:

running 1 test
running `C:\Rust\cargo\target\debug\cargo.exe build -Z bindeps`
test artifact_dep::lib_with_bin_artifact_and_lib_false ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 2781 filtered out; finished in 0.54s

running 1 test
thread 'artifact_dep::lib_with_bin_artifact_and_lib_false' panicked at 'failed to remove "C:\\Rust\\cargo\\target\\tmp\\cit\\testsuite\\artifact_dep\\lib_with_bin_artifact_and_lib_false": Os { code: 5, kind: PermissionDenied

cargo clean is also failing:

PS C:\Rust\cargo> cargo clean
error: failed to remove build artifact

Caused by:
  failed to remove file `C:\Rust\cargo\target\tmp\cit\t1365\home\.cargo`

Caused by:
  Acceso denegado. (os error 5)

weihanglo · 2023-03-05T09:46:53Z

They're failing by "os error 3" (windows paths problem maybe?)

Interesting! On Windows then it is probably related to #11442.

- Adding display of which target failed to compile - Consistent messages for warnings/errors. - Fixing assertions on related tests.

weihanglo

We are finally here. Looks awesome! Thank you for the contribution :)

weihanglo · 2023-03-07T07:59:01Z

@bors r+

bors · 2023-03-07T07:59:03Z

📌 Commit db457d9 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2023-03-07T07:59:17Z

⌛ Testing commit db457d9 with merge 66789e5...

bors · 2023-03-07T08:59:50Z

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

23 commits in 9880b408a3af50c08fab3dbf4aa2a972df71e951..c1334b059c6dcceab3c10c81413f79bb832c8d9d 2023-02-28 19:39:39 +0000 to 2023-03-07 19:21:50 +0000 - Add `CARGO_PKG_README` (rust-lang/cargo#11645) - path dependency: fix cargo-util version (rust-lang/cargo#11807) - Adding display of which target failed to compile (rust-lang/cargo#11636) - Fix `CARGO_CFG_` vars for configs defined both with and without value (rust-lang/cargo#11790) - Breaking endless loop on cyclic features in added dependency in cargo-add (rust-lang/cargo#11805) - Enhance the doc of timing report with graphs (rust-lang/cargo#11798) - Make `sparse` the default protocol for crates.io (rust-lang/cargo#11791) - Use sha2 to calculate SHA256 (rust-lang/cargo#11795) - gitoxide progress bar fixes (rust-lang/cargo#11800) - Check publish_to_alt_registry publish content (rust-lang/cargo#11799) - chore: fix missing files in autolabel trigger_files (rust-lang/cargo#11797) - chore: Update base64 (rust-lang/cargo#11796) - Fix some doc typos (rust-lang/cargo#11794) - chore(ci): Enforce cargo-deny in CI (rust-lang/cargo#11761) - Some cleanup for unstable docs (rust-lang/cargo#11793) - gitoxide integration: fetch (rust-lang/cargo#11448) - patch can conflict on not activated packages (rust-lang/cargo#11770) - fix(toml): Provide a way to show unused manifest keys for dependencies (rust-lang/cargo#11630) - Improve error for missing crate in --offline mode for sparse index (rust-lang/cargo#11783) - feat(resolver): `-Zdirect-minimal-versions` (rust-lang/cargo#11688) - feat: Use test name for dir when running tests (rust-lang/cargo#11738) - Jobserver cleanup (rust-lang/cargo#11764) - Fix help string for "--charset" option of "cargo tree" (rust-lang/cargo#11785) Note that some 3rd-party licensing allowed list changed due to the introducion of `gix` dependency

Update cargo 25 commits in 9880b408a3af50c08fab3dbf4aa2a972df71e951..7d3033d2e59383fd76193daf9423c3d141972a7d 2023-02-28 19:39:39 +0000 to 2023-03-08 17:05:08 +0000 - Revert "rust-lang/cargo#11738" - Use test name for dir when running tests (rust-lang/cargo#11812) - Update CHANGELOG for 1.68 backports (rust-lang/cargo#11810) - Add `CARGO_PKG_README` (rust-lang/cargo#11645) - path dependency: fix cargo-util version (rust-lang/cargo#11807) - Adding display of which target failed to compile (rust-lang/cargo#11636) - Fix `CARGO_CFG_` vars for configs defined both with and without value (rust-lang/cargo#11790) - Breaking endless loop on cyclic features in added dependency in cargo-add (rust-lang/cargo#11805) - Enhance the doc of timing report with graphs (rust-lang/cargo#11798) - Make `sparse` the default protocol for crates.io (rust-lang/cargo#11791) - Use sha2 to calculate SHA256 (rust-lang/cargo#11795) - gitoxide progress bar fixes (rust-lang/cargo#11800) - Check publish_to_alt_registry publish content (rust-lang/cargo#11799) - chore: fix missing files in autolabel trigger_files (rust-lang/cargo#11797) - chore: Update base64 (rust-lang/cargo#11796) - Fix some doc typos (rust-lang/cargo#11794) - chore(ci): Enforce cargo-deny in CI (rust-lang/cargo#11761) - Some cleanup for unstable docs (rust-lang/cargo#11793) - gitoxide integration: fetch (rust-lang/cargo#11448) - patch can conflict on not activated packages (rust-lang/cargo#11770) - fix(toml): Provide a way to show unused manifest keys for dependencies (rust-lang/cargo#11630) - Improve error for missing crate in --offline mode for sparse index (rust-lang/cargo#11783) - feat(resolver): `-Zdirect-minimal-versions` (rust-lang/cargo#11688) - feat: Use test name for dir when running tests (rust-lang/cargo#11738) - Jobserver cleanup (rust-lang/cargo#11764) - Fix help string for "--charset" option of "cargo tree" (rust-lang/cargo#11785) --- ~~This update is primarily for making rust-lang/cargo#11630 into 1.69~~ (will file a beta backport then). However, just look into the licenses and dependencies permitted list, it looks a bit unfortunate but inevitable I guess? r? `@ehuss` cc `@Muscraft`

Fixes issue rust-lang#10834

09f3293

rustbot assigned ehuss Jan 27, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 27, 2023

weihanglo reviewed Jan 27, 2023

View reviewed changes

rustbot assigned weihanglo and unassigned ehuss Jan 27, 2023

aortizpimentel changed the title ~~Fixes issue https://github.com/rust-lang/cargo/issues/10834~~ Adding display of which target failed to compile Jan 28, 2023

Revert "Fixes issue rust-lang#10834"

8d1f4ad

This reverts commit 09f3293.

weihanglo added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 30, 2023

rustbot added the A-build-execution Area: anything dealing with executing the compiler label Mar 3, 2023

weihanglo reviewed Mar 3, 2023

View reviewed changes

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

Merge branch 'rust-lang:master' into master

12dbca3

weihanglo reviewed Mar 4, 2023

View reviewed changes

Fixing issue rust-lang#10834:

db457d9

- Adding display of which target failed to compile - Consistent messages for warnings/errors. - Fixing assertions on related tests.

aortizpimentel force-pushed the fix/10834 branch from bbf6143 to db457d9 Compare March 7, 2023 05:47

weihanglo approved these changes Mar 7, 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-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Mar 7, 2023

bors merged commit 66789e5 into rust-lang:master Mar 7, 2023

weihanglo mentioned this pull request Mar 7, 2023

Update cargo rust-lang/rust#108665

Merged

ehuss added this to the 1.70.0 milestone Mar 9, 2023

weihanglo mentioned this pull request Jun 2, 2023

Cargo prints "Compiling <crate>" when build script runs, and gives no indication when the final crate artefact is being built #833

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding display of which target failed to compile #11636

Adding display of which target failed to compile #11636

aortizpimentel commented Jan 27, 2023

rustbot commented Jan 27, 2023

weihanglo left a comment •

edited

Loading

weihanglo commented Jan 27, 2023

ehuss commented Jan 28, 2023

aortizpimentel commented Jan 28, 2023

weihanglo commented Jan 28, 2023

weihanglo commented Mar 2, 2023

aortizpimentel commented Mar 2, 2023

weihanglo commented Mar 2, 2023

aortizpimentel commented Mar 3, 2023

weihanglo commented Mar 3, 2023

aortizpimentel commented Mar 3, 2023

aortizpimentel commented Mar 3, 2023

weihanglo left a comment

weihanglo Mar 4, 2023 •

edited

Loading

weihanglo Mar 4, 2023 •

edited

Loading

weihanglo Mar 4, 2023

weihanglo Mar 4, 2023

aortizpimentel commented Mar 5, 2023

weihanglo commented Mar 5, 2023

weihanglo left a comment

weihanglo commented Mar 7, 2023

bors commented Mar 7, 2023

bors commented Mar 7, 2023

bors commented Mar 7, 2023

	.with_stderr_contains("[ERROR] could not compile `foo` [..] due to previous error\n")
	.with_stderr_contains("[ERROR] could not compile `foo` (bin "foo") due to previous error\n")

	let mut message = compile_error_msg(None, &unit.pkg.name(), &unit.target, &unit.mode);
	let mut message = descriptive_pkg_name(&unit.pkg.name(), &unit.target, &unit.mode);
	message.push_str(" generated ");

Adding display of which target failed to compile #11636

Adding display of which target failed to compile #11636

Conversation

aortizpimentel commented Jan 27, 2023

rustbot commented Jan 27, 2023

weihanglo left a comment • edited Loading

Choose a reason for hiding this comment

weihanglo commented Jan 27, 2023

ehuss commented Jan 28, 2023

aortizpimentel commented Jan 28, 2023

weihanglo commented Jan 28, 2023

weihanglo commented Mar 2, 2023

aortizpimentel commented Mar 2, 2023

weihanglo commented Mar 2, 2023

aortizpimentel commented Mar 3, 2023

weihanglo commented Mar 3, 2023

aortizpimentel commented Mar 3, 2023

aortizpimentel commented Mar 3, 2023

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo Mar 4, 2023 • edited Loading

Choose a reason for hiding this comment

weihanglo Mar 4, 2023 • edited Loading

Choose a reason for hiding this comment

weihanglo Mar 4, 2023

Choose a reason for hiding this comment

weihanglo Mar 4, 2023

Choose a reason for hiding this comment

aortizpimentel commented Mar 5, 2023

weihanglo commented Mar 5, 2023

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo commented Mar 7, 2023

bors commented Mar 7, 2023

bors commented Mar 7, 2023

bors commented Mar 7, 2023

weihanglo left a comment •

edited

Loading

weihanglo Mar 4, 2023 •

edited

Loading

weihanglo Mar 4, 2023 •

edited

Loading