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

Stabilize -Cdlltool #109575

Closed
wants to merge 1 commit into from
Closed

Stabilize -Cdlltool #109575

wants to merge 1 commit into from

Conversation

dpaoliello
Copy link
Contributor

As a prerequisite to stabilizing raw-dylib (tracking: #58713) the -Zdlltool argument (that permits a user to specify a custom location for the dlltool executable that is used to generate import libraries on windows-gnu targets) also needs to be stabilized so that users don't need to reach for an unstable option to configure a stable feature.

Changes:

  • Stabilized the argument as -Cdlltool.
  • Note the path to dlltool if invoking it failed (we don't need to do this if dlltool returns an error since it prints its path in the error message).
  • Adds tests for -Cdlltool.
  • Adds tests for being unable to find the dlltool executable, and dlltool failing.
  • Fixes a bug where we were checking the exit code of dlltool to see if it failed, but dlltool always returns 0 (indicating success), so instead we need to check if anything was written to stderr.
  • If dlltool returns an error only print its stderr as stdout doesn't contain anything interesting (and using \n in the messages.ftl file doesn't work).

NOTE: As previously noted (#104218 (comment)) using dlltool within rustc is temporary, but this is not the first time that Rust has added a temporary tool use and argument: #104218 (comment)

@rustbot
Copy link
Collaborator

rustbot commented Mar 24, 2023

r? @TaKO8Ki

(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 Mar 24, 2023
@dpaoliello
Copy link
Contributor Author

r? @michaelwoerister

@rustbot rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Mar 24, 2023
@@ -5,6 +5,7 @@
-C debug-assertions=val -- explicitly enable the `cfg(debug_assertions)` directive
-C debuginfo=val -- debug info emission level (0 = no debug info, 1 = line tables only, 2 = full debug info with variable and type information; default: 0)
-C default-linker-libraries=val -- allow the linker to link its default libraries (default: no)
-C dlltool=val -- import library generation tool (windows-gnu only)
Copy link
Member

Choose a reason for hiding this comment

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

Can you replace (windows-gnu only) with (ignored except when targeting windows-gnu). I think that's more clear.

@michaelwoerister
Copy link
Member

Thanks, @dpaoliello, looks great!

What do you think about extending this PR to also stabilize raw-dylibs on x86? Or are there any blockers for that left?

Since we'll need an FCP for this anyway, we could take care of x86 too.

@michaelwoerister michaelwoerister 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 Mar 27, 2023
@petrochenkov
Copy link
Contributor

Are there any interactions with -Clink-self-contained here?
Do we ship dlltool with Rust toolchain or it's always found somewhere else on the system?

@petrochenkov petrochenkov self-assigned this Mar 27, 2023
@mati865
Copy link
Contributor

mati865 commented Mar 27, 2023

Do we ship dlltool with Rust toolchain or it's always found somewhere else on the system?

We do:

let target_tools = [compiler, "ld.exe", "dlltool.exe", "libwinpthread-1.dll"];
but I don't remember more details.

@dpaoliello
Copy link
Contributor Author

dpaoliello commented Mar 27, 2023

Are there any interactions with -Clink-self-contained here?

Currently no. Given the wording of that argument is this flag controls whether the linker will use libraries and objects shipped with Rust instead or those in the system I wouldn't expect it to either (since dlltool is a tool used by the compiler, rather than an object or library that is linked in). One could argue that it should also cover the tools used by the compiler, but I feel like that's out-of-scope for this change (since it would affect how other tools are discovered).

Do we ship dlltool with Rust toolchain or it's always found somewhere else on the system?

dlltool ships with the pc-windows-gnu rustc component if the dist.include-mingw-linker option is set (which it is by default): #108581

@dpaoliello
Copy link
Contributor Author

Thanks, @dpaoliello, looks great!

What do you think about extending this PR to also stabilize raw-dylibs on x86? Or are there any blockers for that left?

Since we'll need an FCP for this anyway, we could take care of x86 too.

There are no more blockers that I know of: I'll merge this with the x86 stabilization for raw-dylib and create a new PR.

@dpaoliello
Copy link
Contributor Author

Closing in favor of stabilizing the entire raw-dylib feature: #109677

@dpaoliello dpaoliello closed this Mar 27, 2023
@petrochenkov
Copy link
Contributor

@dpaoliello

One could argue that it should also cover the tools used by the compiler

Yeah, that's exactly the plan, see the discussion on #96884.
I need to check how the search for dlltool is currently happening, but we'll likely be able to make the -Cself-contained changes in a backward compatible manner anyway.

@dpaoliello
Copy link
Contributor Author

@dpaoliello

One could argue that it should also cover the tools used by the compiler

Yeah, that's exactly the plan, see the discussion on #96884. I need to check how the search for dlltool is currently happening, but we'll likely be able to make the -Cself-contained changes in a backward compatible manner anyway.

Currently we search for dlltool by walking the PATH (

fn find_binutils_dlltool(sess: &Session) -> OsString {
), so it should be possible to add a check in there for -Cself-contained and redirect to a "known" location.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

6 participants