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.

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

Better diagnostics for dlltool errors. #112591

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

jfgoog
Copy link
Contributor

@jfgoog jfgoog commented Jun 13, 2023

When dlltool fails, show the full command that was executed. In particular, llvm-dlltool is not very helpful, printing a generic usage message rather than what actually went wrong, so stdout and stderr aren't of much use when troubleshooting.

@rustbot
Copy link
Collaborator

rustbot commented Jun 13, 2023

r? @fee1-dead

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 13, 2023
@jfgoog
Copy link
Contributor Author

jfgoog commented Jun 13, 2023

Example of output:

error: Dlltool could not create import library with /usr/local/google/home/jamesfarrell/src/rust-toolchain/prebuilts/clang/host/linux-x86/clang-r487747c/bin/llvm-dlltool -d /tmp/rustcNiX2JN/kernel32.def -D kernel32.dll -l /tmp/rustcNiX2JN/kernel32.lib -m i386:x86-64 -f --64 --no-leading-underscore --temp-prefix /tmp/rustcNiX2JN/kernel32.dll:
       OVERVIEW: llvm-dlltool

       USAGE: llvm-dlltool [options] file...

       OPTIONS:
         -D <value> Specify the input DLL Name
         -d <value> Input .def File
         -f <value> Assembler Flags
         -k         Kill @n Symbol from export
         -l <value> Generate an import lib
         -m <value> Set target machine
         -S <value> Assembler

       TARGETS: i386, i386:x86-64, arm, arm64

(Note that the underlying problem causing this particular error is that llvm-dlltool doesn't support --temp-prefix, which I intend to address separately.)

@rust-log-analyzer

This comment has been minimized.

@fee1-dead
Copy link
Member

I'm not familiar with this.

r? compiler

@rustbot rustbot assigned WaffleLapkin and unassigned fee1-dead Jun 16, 2023
Copy link
Member

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

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

Can you squash the commits together?

@jfgoog jfgoog force-pushed the better-dlltool-diagnostics branch from 4ca05d1 to 4bdba91 Compare June 20, 2023 14:36
@jfgoog
Copy link
Contributor Author

jfgoog commented Jun 20, 2023

Squashed. Thanks for the review.

Copy link
Member

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

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

Thanks!

@WaffleLapkin
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 20, 2023

📌 Commit 4bdba91 has been approved by WaffleLapkin

It is now in the queue for this repository.

@bors 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 Jun 20, 2023
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 20, 2023
… r=WaffleLapkin

Better diagnostics for dlltool errors.

When dlltool fails, show the full command that was executed. In particular, llvm-dlltool is not very helpful, printing a generic usage message rather than what actually went wrong, so stdout and stderr aren't of much use when troubleshooting.
@compiler-errors
Copy link
Member

@bors r- failed in rollup: #112851 (comment)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 20, 2023
@WaffleLapkin
Copy link
Member

@jfgoog you need to find a way to bless the test in such a way, it passes both in CI and locally (this might be non-trivial, given that the test includes paths...)

@jfgoog jfgoog force-pushed the better-dlltool-diagnostics branch from 4bdba91 to d1f68ea Compare June 22, 2023 15:31
@jfgoog
Copy link
Contributor Author

jfgoog commented Jun 22, 2023

Done. I'm not able to test locally, since I'm building on Linux and using llvm-dlltool, which produces different output, which is the entire reason for this change.

Is there a way to trigger the build that failed without sending to submit queue?

@WaffleLapkin
Copy link
Member

@jfgoog you should be able to add the x86_64-mingw to CI-on-pull-requests, but I'm not sure about the details.

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Jun 26, 2023

📌 Commit d1f68ea has been approved by WaffleLapkin

It is now in the queue for this repository.

@bors 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: This is awaiting some action (such as code changes or more information) from the author. labels Jun 26, 2023
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jul 9, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 9, 2023
@Noratrieb
Copy link
Member

This looks like a legitimate failure, probably needs some blessing.

@Noratrieb Noratrieb added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 9, 2023
@jfgoog
Copy link
Contributor Author

jfgoog commented Jul 17, 2023

I've been on vacation. Will take a look soon.

When dlltool fails, show the full command that was executed. In
particular, llvm-dlltool is not very helpful, printing a generic usage
message rather than what actually went wrong, so stdout and stderr
aren't of much use when troubleshooting.
@jfgoog jfgoog force-pushed the better-dlltool-diagnostics branch from d1f68ea to c59b823 Compare July 17, 2023 20:20
@jfgoog
Copy link
Contributor Author

jfgoog commented Jul 18, 2023

OK, I think it's fixed, but I unfortunately don't have a good way to test locally, as I mentioned previously.

@WaffleLapkin
Copy link
Member

@bors r+

Let's try it

@bors
Copy link
Contributor

bors commented Jul 18, 2023

📌 Commit c59b823 has been approved by WaffleLapkin

It is now in the queue for this repository.

@bors 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: This is awaiting some action (such as code changes or more information) from the author. labels Jul 18, 2023
@bors
Copy link
Contributor

bors commented Jul 18, 2023

⌛ Testing commit c59b823 with merge 75bcdc515f778584d42b0050cc105ab12d5a0c98...

@bors
Copy link
Contributor

bors commented Jul 18, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 18, 2023
@rust-log-analyzer
Copy link
Collaborator

The job dist-arm-linux failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
Caused by:
  failed to query replaced source registry `crates-io`

Caused by:
  download of cm/ak/cmake failed
Caused by:
Caused by:
  failed to get successful HTTP response from `https://index.crates.io/cm/ak/cmake` (18.165.83.65), got 421
  debug headers:
  x-amz-cf-pop: IAD12-P1
  x-cache: Error from cloudfront
  x-amz-cf-pop: IAD55-P3
  x-amz-cf-id: LM2aafdvxyhixbf2HJOaAisxMiZe_5-jGUvdbgUa6hysujfWYO5VqQ==
  <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
  <HTML><HEAD><META HTTP-EQUIV="Content-Type" CONTENT="text/html; charset=iso-8859-1">
  <TITLE>ERROR: The request could not be satisfied</TITLE>
  </HEAD><BODY>
---
[TIMING] compile::Sysroot { compiler: Compiler { stage: 0, host: x86_64-unknown-linux-gnu }, force_recompile: false } -- 0.000
[TIMING] builder::Builder::sysroot_libdir::Libdir { compiler: Compiler { stage: 0, host: x86_64-unknown-linux-gnu }, target: x86_64-unknown-linux-gnu } -- 0.000
##[group]Building stage0 library artifacts (x86_64-unknown-linux-gnu)
    Updating crates.io index
error: failed to get `shell-escape` as a dependency of package `clippy_dev v0.0.1 (/checkout/src/tools/clippy/clippy_dev)`
Caused by:
  failed to query replaced source registry `crates-io`

Caused by:
Caused by:
  download of sh/el/shell-escape failed
Caused by:
Caused by:
  failed to get successful HTTP response from `https://index.crates.io/sh/el/shell-escape` (18.165.83.98), got 421
  debug headers:
  x-amz-cf-pop: IAD12-P1
  x-cache: Error from cloudfront
  x-amz-cf-pop: IAD55-P3
  x-amz-cf-id: 4NHRuTpfPbKJKpjf-GMiOpboRXbyfxAbOQtyJ3y7yWB9lhezenp9nQ==
  <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
  <HTML><HEAD><META HTTP-EQUIV="Content-Type" CONTENT="text/html; charset=iso-8859-1">
  <TITLE>ERROR: The request could not be satisfied</TITLE>
  </HEAD><BODY>

@WaffleLapkin
Copy link
Member

@bors retry crates.io error

@bors 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 Jul 19, 2023
@bors
Copy link
Contributor

bors commented Jul 19, 2023

⌛ Testing commit c59b823 with merge 77e24f9...

@bors
Copy link
Contributor

bors commented Jul 19, 2023

☀️ Test successful - checks-actions
Approved by: WaffleLapkin
Pushing 77e24f9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 19, 2023
@bors bors merged commit 77e24f9 into rust-lang:master Jul 19, 2023
@rustbot rustbot added this to the 1.73.0 milestone Jul 19, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (77e24f9): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.8% [0.8%, 0.8%] 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
6.4% [5.9%, 7.2%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.4% [-2.5%, -2.3%] 2
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 647.114s -> 647.294s (0.03%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants