-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Allow rust staticlib to work with MSVC's /WHOLEARCHIVE #129257
Conversation
@bors try |
…r=<try> Allow rust staticlib to work with MSVC's /WHOLEARCHIVE This renames the `__NULL_IMPORT_DESCRIPTOR` to prevent conflicts. try-job: x86_64-msvc try-job: i686-msvc r? ghost
☀️ Try build successful - checks-actions |
f64ecf1
to
50a28af
Compare
The ar_archive_writer crate has been update so this should be ready for review. Hopefully the rmake test is at least a bit nicer now, r? jieyouxu |
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. This PR modifies cc @jieyouxu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this LGTM, feel free to r=me with or without addressing the nit.
@bors rollup=iffy (funny linker things) |
50a28af
to
d4a14b1
Compare
Since the changes are cosmetic and check passed... @bors r=jieyouxu |
//! It ensures we can use `/WHOLEARCHIVE` to link a rust staticlib into DLL | ||
//! using the MSVC linker | ||
|
||
//@ only-msvc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice (but not required) to also cover windows-gnullvm
that has the same issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've not tried gnullvm specifically but the llvm linker has another issue which I think needs solving on their end first (which is why even the msvc test skips lld-link).
.idata$4 should not refer to special section 0
LLVM seems to not properly recognise .idata$4
and .idata$5
as special unless they're specifically in short import libraries. incidentally this also makes it break on MSVC long import libraries. Though since they haven't been in use since the 90's I guess there's not been much pressure to support it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably should open a new issue to track this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe? How do we usually track upstream issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a special label for waiting on LLVM: S-waiting-for-llvm
for describing the dragon is eepy (and better if it links to a llvm upstream issue)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though since they haven't been in use since the 90's I guess there's not been much pressure to support it.
ld.bfd used by windows-gnu
still uses them as it still cannot output short import libraries 😉
…, r=jieyouxu Allow rust staticlib to work with MSVC's /WHOLEARCHIVE This fixes rust-lang#129020 by renaming the `__NULL_IMPORT_DESCRIPTOR` to prevent conflicts.
Rollup of 6 pull requests Successful merges: - rust-lang#127623 (fix: fs::remove_dir_all: treat internal ENOENT as success) - rust-lang#128627 (Special case DUMMY_SP to emit line 0/column 0 locations on DWARF platforms.) - rust-lang#129187 (bootstrap: fix clean's remove_dir_all implementation) - rust-lang#129190 (Added f16 and f128 to tests/ui/consts/const-float-bits-conv.rs) - rust-lang#129231 (improve submodule updates) - rust-lang#129257 (Allow rust staticlib to work with MSVC's /WHOLEARCHIVE) r? `@ghost` `@rustbot` modify labels: rollup
I'm suspecting this PR caused the failure in #129300 (comment):
Temporarily kicking this out of the queue to check if it's this or another bootstrap PR. @bors r- |
💔 Test failed - checks-actions |
@bors retry spurious "failed to remove file" on x86_64-msvc-ext again |
pinging back #127883 |
…r=jieyouxu Allow rust staticlib to work with MSVC's /WHOLEARCHIVE This fixes rust-lang#129020 by renaming the `__NULL_IMPORT_DESCRIPTOR` to prevent conflicts. try-job: dist-i686-msvc
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
…, r=jieyouxu Allow rust staticlib to work with MSVC's /WHOLEARCHIVE This fixes rust-lang#129020 by renaming the `__NULL_IMPORT_DESCRIPTOR` to prevent conflicts. try-job: dist-i686-msvc
…, r=jieyouxu Allow rust staticlib to work with MSVC's /WHOLEARCHIVE This fixes rust-lang#129020 by renaming the `__NULL_IMPORT_DESCRIPTOR` to prevent conflicts. try-job: dist-i686-msvc
Rollup of 7 pull requests Successful merges: - rust-lang#126985 (Implement `-Z embed-source` (DWARFv5 source code embedding extension)) - rust-lang#128349 (Enable `f16` on x86 and x86-64) - rust-lang#128876 (Ship MinGW-w64 runtime DLLs along with `rust-lld.exe` for `-pc-windows-gnu` targets) - rust-lang#129190 (Add f16 and f128 to tests/ui/consts/const-float-bits-conv.rs) - rust-lang#129257 (Allow rust staticlib to work with MSVC's /WHOLEARCHIVE) - rust-lang#129386 (Use a LocalDefId in ResolvedArg.) - rust-lang#129400 (Update `compiler_builtins` to `0.1.120`) r? `@ghost` `@rustbot` modify labels: rollup
☀️ Test successful - checks-actions |
Finished benchmarking commit (5ad98b4): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary 2.7%)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.
CyclesResults (secondary 9.6%)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.
Binary sizeResults (secondary -0.0%)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.
Bootstrap: 749.851s -> 749.145s (-0.09%) |
This fixes #129020 by renaming the
__NULL_IMPORT_DESCRIPTOR
to prevent conflicts.try-job: dist-i686-msvc