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.

Sign up for GitHub

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

Jump to bottom

Ignore LLVM ABI in dlltool tests since those targets don't use dlltool #124138

Merged
merged 2 commits into from
May 2, 2024
Merged

Ignore LLVM ABI in dlltool tests since those targets don't use dlltool #124138

merged 2 commits into from
May 2, 2024

Conversation

mati865
Copy link
Contributor

@mati865 mati865 commented Apr 18, 2024

Otherwise those two tests fail when running ./x.py test with this target.

@rustbot
Copy link
Collaborator

rustbot commented Apr 18, 2024

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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-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. labels Apr 18, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 18, 2024

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@mati865 mati865 force-pushed the ignore-llvm-abi-in-dlltool-tests branch from c8668eb to b51f11d Compare April 18, 2024 22:38
@bjorn3
Copy link
Member

bjorn3 commented Apr 19, 2024

//@ ignore-llvm reads to me like it ignores the test when using the LLVM codegen backend, not when using the *-gnullvm targets.

@mati865
Copy link
Contributor Author

mati865 commented Apr 19, 2024

I agree it's ambiguous but also in line with other ABIs. I'm open for suggestions though.

Personally I'd love if ignore statements were more precise, like //@ignore-abi llvm or //@ignore-os windows.

@jieyouxu
Copy link
Member

jieyouxu commented Apr 19, 2024 •
edited
Loading

I feel like we should fix needs-dlltool instead, like, what's the point of needs-dlltool if the test still runs on a target without dlltools?

@jeremyd2019
Copy link
Contributor

needs-dlltool seems to answer the question "is dlltool on the PATH", which is a different question than "will the target actually try to use dlltool". For gnullvm, it will not try to use dlltool, so the fact that there is one on the PATH is not particularly relevant.

For reference, the condition that decides whether to use dlltool or use llvm APIs directly to generate an import library is

rust/compiler/rustc_codegen_llvm/src/common.rs

Line 387 in f9b1614

target.vendor == "pc" && target.os == "windows" && target.env == "gnu" && target.abi.is_empty()

@fee1-dead
Copy link
Member

r? compiler

@rustbot rustbot assigned davidtwco and unassigned fee1-dead Apr 20, 2024
@jeremyd2019
Copy link
Contributor

It looks like tests\run-make\raw-dylib-custom-dlltool fails similarly

@jeremyd2019 jeremyd2019 mentioned this pull request Apr 21, 2024
Draft
@davidtwco
Copy link
Member

I feel like we should fix needs-dlltool instead, like, what's the point of needs-dlltool if the test still runs on a target without dlltools?

I'd agree with this, we should extend needs-dlltool so that it isn't just "there is a dlltool" but "the target will use it", ignore-llvm seems like it is ignoring that codegen backend.

@mati865 mati865 force-pushed the ignore-llvm-abi-in-dlltool-tests branch from b51f11d to 53920c8 Compare April 23, 2024 18:57
@rustbot
Copy link
Collaborator

rustbot commented Apr 23, 2024

Some changes occurred in run-make tests.

cc @jieyouxu

@mati865
Copy link
Contributor Author

mati865 commented Apr 23, 2024

Tested only x86_64-pc-windows-gnullvm but the changes are simple enough.

@mati865 mati865 force-pushed the ignore-llvm-abi-in-dlltool-tests branch from 53920c8 to b1fd4a7 Compare April 24, 2024 17:25
@davidtwco
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 2, 2024

📌 Commit b1fd4a7 has been approved by davidtwco

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 2, 2024
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 2, 2024
fmease added a commit to fmease/rust that referenced this pull request May 2, 2024
@fmease
Rollup merge of rust-lang#124138 - mati865:ignore-llvm-abi-in-dlltool…
1bbe36b 
…-tests, r=davidtwco

Ignore LLVM ABI in dlltool tests since those targets don't use dlltool

Otherwise those two tests fail when running `./x.py test` with this target.
@fmease fmease mentioned this pull request May 2, 2024
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request May 2, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 2, 2024
@bors
Auto merge of rust-lang#124629 - matthiaskrgr:rollup-gttvzrg, r=matth…
79734f1 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#124138 (Ignore LLVM ABI in dlltool tests since those targets don't use dlltool)
 - rust-lang#124414 (remove extraneous note on `UnableToRunDsymutil` diagnostic)
 - rust-lang#124579 (Align: add bytes_usize and bits_usize)
 - rust-lang#124622 (Cleanup: Rid the `rmake` test runners of `extern crate run_make_support;`)
 - rust-lang#124623 (shallow resolve in orphan check)
 - rust-lang#124624 (Use `tcx.types.unit` instead of `Ty::new_unit(tcx)`)
 - rust-lang#124627 (interpret: hide some reexports in rustdoc)

r? `@ghost`
`@rustbot` modify labels: rollup
@riverar riverar mentioned this pull request May 2, 2024
Merged
@bors bors merged commit f01e99f into rust-lang:master May 2, 2024
10 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 2, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 2, 2024
@rust-timer
Unrolled build for rust-lang#124138
5353da0 
Rollup merge of rust-lang#124138 - mati865:ignore-llvm-abi-in-dlltool-tests, r=davidtwco

Ignore LLVM ABI in dlltool tests since those targets don't use dlltool

Otherwise those two tests fail when running `./x.py test` with this target.
@matthiaskrgr matthiaskrgr mentioned this pull request May 2, 2024
Closed
@mati865 mati865 deleted the ignore-llvm-abi-in-dlltool-tests branch May 2, 2024 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeremyd2019 jeremyd2019 jeremyd2019 left review comments

@davidtwco davidtwco davidtwco approved these changes

Assignees

@davidtwco davidtwco

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc 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.
Projects
None yet
Milestone
1.80.0
Development

Successfully merging this pull request may close these issues.
9 participants
@mati865 @rustbot @rust-log-analyzer @bjorn3 @jieyouxu @jeremyd2019 @fee1-dead @davidtwco @bors