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 raw-dylib, link_ordinal, import_name_type and -Cdlltool #109677

Merged
merged 1 commit into from May 6, 2023

Conversation

dpaoliello
Copy link
Contributor

@dpaoliello dpaoliello commented Mar 27, 2023

This stabilizes the raw-dylib feature (#58713) for all architectures (i.e., x86 as it is already stable for all other architectures).

Changes:

  • Permit the use of the raw-dylib link kind for x86, the link_ordinal attribute and the import_name_type key for the link attribute.
  • Mark the raw_dylib feature as stable.
  • Stabilized the -Zdlltool 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.

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)

Big thanks to @tbu- for the first version of this PR (#104218)

@rustbot
Copy link
Collaborator

rustbot commented Mar 27, 2023

r? @oli-obk

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

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 27, 2023
@tbu-
Copy link
Contributor

tbu- commented Mar 27, 2023

Thanks for picking this up! :)

@dpaoliello
Copy link
Contributor Author

r? @michaelwoerister

@est31
Copy link
Member

est31 commented Mar 28, 2023

using \n in the messages.ftl file doesn't work

FTR if you think that the stdout contains useful information, you can directly do newlines in fluent files, they need to just be aligned correctly to the previous line. The Dlltool error doesn't seem to be the only place this mistake has happened in ftl files, and I suppose it's not that hard to think that \n works in them, so I've opened #109686.

@petrochenkov petrochenkov removed their assignment Mar 28, 2023
@bors
Copy link
Contributor

bors commented Mar 30, 2023

☔ The latest upstream changes (presumably #109734) made this pull request unmergeable. Please resolve the merge conflicts.

@michaelwoerister michaelwoerister added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 30, 2023
@michaelwoerister
Copy link
Member

Thanks a lot, @dpaoliello! The PR looks good to me.

The only thing I'm missing is a mention of the import_name_type (added in #100732) in the stabilization report and in the documentation update PR. Once we have that, we can start the FCP 🙂

@michaelwoerister
Copy link
Member

@petrochenkov, you mentioned potential interactions between -Cdlltool and -Cself-contained in #109575. Are there any blocking issues around that? It sounds like the two features are not in conflict with each other, right?

@petrochenkov
Copy link
Contributor

you mentioned potential interactions between -Cdlltool and -Cself-contained in #109575. Are there any blocking issues around that? It sounds like the two features are not in conflict with each other, right?

No, there shouldn't be any major issues.

@dpaoliello dpaoliello changed the title Stabilize raw-dylib, link_ordinal and -Cdlltool Stabilize raw-dylib, link_ordinal, import_name_type and -Cdlltool Mar 30, 2023
@dpaoliello
Copy link
Contributor Author

The only thing I'm missing is a mention of the import_name_type (added in #100732) in the stabilization report and in the documentation update PR. Once we have that, we can start the FCP 🙂

@michaelwoerister Done!

@michaelwoerister
Copy link
Member

Thanks, @dpaoliello! Let's start the FCP then. This includes the lang team for final sign off on the import_name_type field of the #[link] attribute. In #100732 (comment), @joshtriplett gave a general OK for the new key, but let's make it part of an FCP.

@dpaoliello's extensive stabilization report is in the tracking issue at #58713 (comment).

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Mar 31, 2023

Team member @michaelwoerister has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Mar 31, 2023
@nikomatsakis
Copy link
Contributor

@rfcbot fcp reviewed

@bors
Copy link
Contributor

bors commented May 1, 2023

📌 Commit 1ece1ea has been approved by michaelwoerister,wesleywiser

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 May 1, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 1, 2023
…ister,wesleywiser

Stabilize raw-dylib, link_ordinal, import_name_type and -Cdlltool

This stabilizes the `raw-dylib` feature (rust-lang#58713) for all architectures (i.e., `x86` as it is already stable for all other architectures).

Changes:
* Permit the use of the `raw-dylib` link kind for x86, the `link_ordinal` attribute and the `import_name_type` key for the `link` attribute.
* Mark the `raw_dylib` feature as stable.
* Stabilized the `-Zdlltool` 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`.

NOTE: As previously noted (rust-lang#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: rust-lang#104218 (comment)

Big thanks to `@tbu-` for the first version of this PR (rust-lang#104218)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 2, 2023
…ister,wesleywiser

Stabilize raw-dylib, link_ordinal, import_name_type and -Cdlltool

This stabilizes the `raw-dylib` feature (rust-lang#58713) for all architectures (i.e., `x86` as it is already stable for all other architectures).

Changes:
* Permit the use of the `raw-dylib` link kind for x86, the `link_ordinal` attribute and the `import_name_type` key for the `link` attribute.
* Mark the `raw_dylib` feature as stable.
* Stabilized the `-Zdlltool` 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`.

NOTE: As previously noted (rust-lang#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: rust-lang#104218 (comment)

Big thanks to ``@tbu-`` for the first version of this PR (rust-lang#104218)
@wesleywiser wesleywiser added the relnotes Marks issues that should be documented in the release notes of the next release. label May 2, 2023
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 3, 2023
…ister,wesleywiser

Stabilize raw-dylib, link_ordinal, import_name_type and -Cdlltool

This stabilizes the `raw-dylib` feature (rust-lang#58713) for all architectures (i.e., `x86` as it is already stable for all other architectures).

Changes:
* Permit the use of the `raw-dylib` link kind for x86, the `link_ordinal` attribute and the `import_name_type` key for the `link` attribute.
* Mark the `raw_dylib` feature as stable.
* Stabilized the `-Zdlltool` 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`.

NOTE: As previously noted (rust-lang#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: rust-lang#104218 (comment)

Big thanks to ```@tbu-``` for the first version of this PR (rust-lang#104218)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 3, 2023
…ister,wesleywiser

Stabilize raw-dylib, link_ordinal, import_name_type and -Cdlltool

This stabilizes the `raw-dylib` feature (rust-lang#58713) for all architectures (i.e., `x86` as it is already stable for all other architectures).

Changes:
* Permit the use of the `raw-dylib` link kind for x86, the `link_ordinal` attribute and the `import_name_type` key for the `link` attribute.
* Mark the `raw_dylib` feature as stable.
* Stabilized the `-Zdlltool` 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`.

NOTE: As previously noted (rust-lang#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: rust-lang#104218 (comment)

Big thanks to ````@tbu-```` for the first version of this PR (rust-lang#104218)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 3, 2023
…ister,wesleywiser

Stabilize raw-dylib, link_ordinal, import_name_type and -Cdlltool

This stabilizes the `raw-dylib` feature (rust-lang#58713) for all architectures (i.e., `x86` as it is already stable for all other architectures).

Changes:
* Permit the use of the `raw-dylib` link kind for x86, the `link_ordinal` attribute and the `import_name_type` key for the `link` attribute.
* Mark the `raw_dylib` feature as stable.
* Stabilized the `-Zdlltool` 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`.

NOTE: As previously noted (rust-lang#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: rust-lang#104218 (comment)

Big thanks to `````@tbu-````` for the first version of this PR (rust-lang#104218)
bors added a commit to rust-lang-ci/rust that referenced this pull request May 6, 2023
Rollup of 8 pull requests

Successful merges:

 - rust-lang#109677 (Stabilize raw-dylib, link_ordinal, import_name_type and -Cdlltool)
 - rust-lang#110780 (rustdoc-search: add slices and arrays to index)
 - rust-lang#110830 (Add FreeBSD cpuset support to `std::thread::available_concurrency`)
 - rust-lang#111139 (Fix MXCSR configuration dependent timing)
 - rust-lang#111239 (Remove unnecessary attribute from a diagnostic)
 - rust-lang#111246 (forbid escaping bound vars in combine)
 - rust-lang#111251 (Issue 109502 follow up, remove unnecessary Vec::new() from compile_test())
 - rust-lang#111261 (Mark `ErrorGuaranteed` constructor as deprecated so people don't use it)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 923a5a2 into rust-lang:master May 6, 2023
11 checks passed
@rustbot rustbot added this to the 1.71.0 milestone May 6, 2023
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label May 25, 2023
@dpaoliello dpaoliello deleted the rawdylib branch January 16, 2024 18:24
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 disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet