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

Pass the arch rather than full target name to windows_registry::find_tool #133955

Merged
merged 1 commit into from
Jan 4, 2025

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Dec 6, 2024

The full target name can be anything with custom target specs. Passing just the arch wasn't possible before cc 1.2, but is now thanks to rust-lang/cc-rs#1285.

try-job: i686-msvc

@bjorn3 bjorn3 added the O-windows-msvc Toolchain: MSVC, Operating system: Windows label Dec 6, 2024
@rustbot
Copy link
Collaborator

rustbot commented Dec 6, 2024

r? @pnkfelix

rustbot has assigned @pnkfelix.
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 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 Dec 6, 2024
@rust-log-analyzer

This comment has been minimized.

@ChrisDenton
Copy link
Member

r? ChrisDenton

@rustbot rustbot assigned ChrisDenton and unassigned pnkfelix Dec 6, 2024
@bjorn3 bjorn3 force-pushed the cc_pass_arch_only branch from 94e6b95 to 5477d85 Compare December 6, 2024 13:59
Copy link
Member

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

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

r=me once CI passes

@bjorn3
Copy link
Member Author

bjorn3 commented Dec 6, 2024

@bors r=ChrisDenton

@bors
Copy link
Contributor

bors commented Dec 6, 2024

📌 Commit 5477d85 has been approved by ChrisDenton

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 Dec 6, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Dec 7, 2024
…enton

Pass the arch rather than full target name to windows_registry::find_tool

The full target name can be anything with custom target specs. Passing just the arch wasn't possible before cc 1.2, but is now thanks to rust-lang/cc-rs#1285.
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 7, 2024
…kingjubilee

Rollup of 11 pull requests

Successful merges:

 - rust-lang#131669 (lint: change help for pointers to dyn types in FFI)
 - rust-lang#133265 (Add a range argument to vec.extract_if)
 - rust-lang#133733 ( compiletest: show the difference between the normalized output and the actual output for lines which didn't match)
 - rust-lang#133955 (Pass the arch rather than full target name to windows_registry::find_tool)
 - rust-lang#133967 ([AIX] Pass -bnoipath when adding rust upstream dynamic crates)
 - rust-lang#133976 (Removed Unnecessary Spaces From RELEASES.md)
 - rust-lang#133980 ([AIX] Remove option "-n" from AIX "ln" command)
 - rust-lang#133987 (Define acronym for thread local storage)
 - rust-lang#133992 (Actually walk into lifetimes and attrs in `EarlyContextAndPass`)
 - rust-lang#133993 (Fix: typo in E0751 error explanation)
 - rust-lang#133996 (Move most tests for `-l` and `#[link(..)]` into `tests/ui/link-native-libs`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 7, 2024
…kingjubilee

Rollup of 11 pull requests

Successful merges:

 - rust-lang#131669 (lint: change help for pointers to dyn types in FFI)
 - rust-lang#133265 (Add a range argument to vec.extract_if)
 - rust-lang#133733 ( compiletest: show the difference between the normalized output and the actual output for lines which didn't match)
 - rust-lang#133955 (Pass the arch rather than full target name to windows_registry::find_tool)
 - rust-lang#133967 ([AIX] Pass -bnoipath when adding rust upstream dynamic crates)
 - rust-lang#133976 (Removed Unnecessary Spaces From RELEASES.md)
 - rust-lang#133980 ([AIX] Remove option "-n" from AIX "ln" command)
 - rust-lang#133987 (Define acronym for thread local storage)
 - rust-lang#133992 (Actually walk into lifetimes and attrs in `EarlyContextAndPass`)
 - rust-lang#133993 (Fix: typo in E0751 error explanation)
 - rust-lang#133996 (Move most tests for `-l` and `#[link(..)]` into `tests/ui/link-native-libs`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 7, 2024
…kingjubilee

Rollup of 11 pull requests

Successful merges:

 - rust-lang#131669 (lint: change help for pointers to dyn types in FFI)
 - rust-lang#133265 (Add a range argument to vec.extract_if)
 - rust-lang#133733 ( compiletest: show the difference between the normalized output and the actual output for lines which didn't match)
 - rust-lang#133955 (Pass the arch rather than full target name to windows_registry::find_tool)
 - rust-lang#133967 ([AIX] Pass -bnoipath when adding rust upstream dynamic crates)
 - rust-lang#133976 (Removed Unnecessary Spaces From RELEASES.md)
 - rust-lang#133980 ([AIX] Remove option "-n" from AIX "ln" command)
 - rust-lang#133987 (Define acronym for thread local storage)
 - rust-lang#133992 (Actually walk into lifetimes and attrs in `EarlyContextAndPass`)
 - rust-lang#133993 (Fix: typo in E0751 error explanation)
 - rust-lang#133996 (Move most tests for `-l` and `#[link(..)]` into `tests/ui/link-native-libs`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 7, 2024
…kingjubilee

Rollup of 11 pull requests

Successful merges:

 - rust-lang#131669 (lint: change help for pointers to dyn types in FFI)
 - rust-lang#133265 (Add a range argument to vec.extract_if)
 - rust-lang#133733 ( compiletest: show the difference between the normalized output and the actual output for lines which didn't match)
 - rust-lang#133955 (Pass the arch rather than full target name to windows_registry::find_tool)
 - rust-lang#133967 ([AIX] Pass -bnoipath when adding rust upstream dynamic crates)
 - rust-lang#133976 (Removed Unnecessary Spaces From RELEASES.md)
 - rust-lang#133980 ([AIX] Remove option "-n" from AIX "ln" command)
 - rust-lang#133987 (Define acronym for thread local storage)
 - rust-lang#133992 (Actually walk into lifetimes and attrs in `EarlyContextAndPass`)
 - rust-lang#133993 (Fix: typo in E0751 error explanation)
 - rust-lang#133996 (Move most tests for `-l` and `#[link(..)]` into `tests/ui/link-native-libs`)

r? `@ghost`
`@rustbot` modify labels: rollup
@workingjubilee
Copy link
Member

@bors r-

@bors try

@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 Dec 8, 2024
@bors
Copy link
Contributor

bors commented Dec 8, 2024

⌛ Trying commit 5477d85 with merge 15df96e...

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 8, 2024
Pass the arch rather than full target name to windows_registry::find_tool

The full target name can be anything with custom target specs. Passing just the arch wasn't possible before cc 1.2, but is now thanks to rust-lang/cc-rs#1285.

try-job: i686-mingw
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Dec 8, 2024

💔 Test failed - checks-actions

@bors
Copy link
Contributor

bors commented Dec 9, 2024

💔 Test failed - checks-actions

@ChrisDenton
Copy link
Member

It looks like it's finding mingw's link instead of msvc's link. This might be an issue with our CI because we should have MSVC's directories earlier in PATH and not be finding mingw at all.

@ChrisDenton
Copy link
Member

Ok, I'm not actually sure why this is happening. I'm minded to wait until cc is updated before digging into too deeply in case that fixes it.

@ChrisDenton ChrisDenton added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Dec 20, 2024
@ChrisDenton
Copy link
Member

@bors try

@ChrisDenton ChrisDenton removed the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Dec 21, 2024
@ChrisDenton
Copy link
Member

@bors ping

@bors
Copy link
Contributor

bors commented Dec 21, 2024

😪 I'm awake I'm awake

@ChrisDenton
Copy link
Member

@bors try

@bors
Copy link
Contributor

bors commented Dec 21, 2024

⌛ Trying commit 5e66869 with merge c0f2f6e...

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 21, 2024
Pass the arch rather than full target name to windows_registry::find_tool

The full target name can be anything with custom target specs. Passing just the arch wasn't possible before cc 1.2, but is now thanks to rust-lang/cc-rs#1285.

try-job: i686-msvc
@bors
Copy link
Contributor

bors commented Dec 21, 2024

💔 Test failed - checks-actions

@rust-log-analyzer
Copy link
Collaborator

The job i686-msvc failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
Updating files:  99% (50434/50943)
Updating files:  99% (50607/50943)
Updating files: 100% (50943/50943)
Updating files: 100% (50943/50943), done.
branch 'try' set up to track 'origin/try'.
Switched to a new branch 'try'
[command]"C:\Program Files\Git\bin\git.exe" log -1 --format=%H
c0f2f6e927e4608d0bd800021e8e5e89d7b30ba0
##[group]Run src/ci/scripts/setup-environment.sh
src/ci/scripts/setup-environment.sh
---
file:.git/config remote.origin.url=https://github.com/rust-lang-ci/rust
file:.git/config remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*
file:.git/config gc.auto=0
file:.git/config http.https://github.com/.extraheader=AUTHORIZATION: basic ***
file:.git/config branch.try.remote=origin
file:.git/config branch.try.merge=refs/heads/try
file:.git/config remote.upstream.fetch=+refs/heads/*:refs/remotes/upstream/*
file:.git/config submodule.library/backtrace.active=true
file:.git/config submodule.library/backtrace.url=https://github.com/rust-lang/backtrace-rs.git
file:.git/config submodule.library/stdarch.active=true
---
[RUSTC-TIMING] alloc test:false 7.622
[RUSTC-TIMING] hashbrown test:false 1.695
error: linking with `link.exe` failed: exit code: 1
  |
  = note: "link.exe" "/DEF:C:\\a\\_temp\\msys64\\tmp\\rustc5Lg5YO\\lib.def" "/NOLOGO" "/LARGEADDRESSAWARE" "/SAFESEH" "C:\\a\\_temp\\msys64\\tmp\\rustc5Lg5YO\\symbols.o" "<1 object files omitted>" "C:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage1-std\\i686-pc-windows-msvc\\release\\deps\\std-90437d65b9885b38.0xaxptrw67d1197n3snh07mqi.rcgu.rmeta" "<1 object files omitted>" "kernel32.lib" "kernel32.lib" "C:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage1-std\\i686-pc-windows-msvc\\release\\deps/{libpanic_unwind-21548079a414409f.rlib,libwindows_targets-4291a0c00630f550.rlib,librustc_demangle-dff39964e7b65931.rlib,libstd_detect-485d0125c53416a6.rlib,libhashbrown-d066a8f9f139e6e2.rlib,librustc_std_workspace_alloc-75fe42d3baf27d68.rlib,libunwind-fa2dfaab3f8a42b7.rlib,libcfg_if-17708c1efb2f15a7.rlib,liballoc-79b71804ba332461.rlib,librustc_std_workspace_core-a67c29f4a9c7a99d.rlib,libcore-76c424679b11e231.rlib,libcompiler_builtins-1c6326f203df102a.rlib}" "advapi32.lib" "ntdll.lib" "userenv.lib" "ws2_32.lib" "dbghelp.lib" "/defaultlib:libcmt" "C:\\a\\_temp\\msys64\\tmp\\rustc5Lg5YO\\bcryptprimitives.dll_imports.lib" "C:\\a\\_temp\\msys64\\tmp\\rustc5Lg5YO\\api-ms-win-core-synch-l1-2-0.dll_imports.lib" "/NXCOMPAT" "/LIBPATH:C:\\Program Files\\Microsoft Visual Studio\\2022\\Enterprise\\VC\\Tools\\MSVC\\14.42.34433\\atlmfc\\lib\\x86" "/LIBPATH:C:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage1-std\\i686-pc-windows-msvc\\release\\build\\compiler_builtins-2c33050784bf0ca1\\out" "/OUT:C:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage1-std\\i686-pc-windows-msvc\\release\\deps\\std-90437d65b9885b38.dll" "/OPT:REF,ICF" "/DLL" "/IMPLIB:C:\\a\\rust\\rust\\build\\i686-pc-windows-msvc\\stage1-std\\i686-pc-windows-msvc\\release\\deps\\std-90437d65b9885b38.dll.lib" "/DEBUG" "/PDBALTPATH:%_PDB%"
  = note: some arguments are omitted. use `--verbose` to show all linker arguments
  = note: link: extra operand '/LARGEADDRESSAWARE'
          Try 'link --help' for more information.

note: `link.exe` returned an unexpected error


note: in the Visual Studio installer, ensure the "C++ build tools" workload is selected
[RUSTC-TIMING] std test:false 21.470
error: could not compile `std` (lib) due to 1 previous error
Build completed unsuccessfully in 0:20:18
Build completed unsuccessfully in 0:20:18
make: *** [Makefile:106: ci-msvc-ps1] Error 1
  network time: Sat, 21 Dec 2024 21:22:35 GMT
##[error]Process completed with exit code 2.
Post job cleanup.
[command]"C:\Program Files\Git\bin\git.exe" version

@ChrisDenton
Copy link
Member

ChrisDenton commented Dec 21, 2024

Oh! I just had a thought. Internally rustc uses "x86" as the target instead of "i686" because it covers "i586" as well:

/// Architecture to use for ABI considerations. Valid options include: "x86",
/// "x86_64", "arm", "aarch64", "mips", "powerpc", "powerpc64", and others.
pub arch: StaticCow<str>,

Whereas I guess cc is expecting the Rust target arch. Either we need to do a little translation or ask cc to support both.

@ChrisDenton
Copy link
Member

Ok, I (hopefully) handled this in cc-rs so let's try this again...

@bors try

@bors
Copy link
Contributor

bors commented Jan 4, 2025

⌛ Trying commit 5e66869 with merge f9d35b2...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 4, 2025
Pass the arch rather than full target name to windows_registry::find_tool

The full target name can be anything with custom target specs. Passing just the arch wasn't possible before cc 1.2, but is now thanks to rust-lang/cc-rs#1285.

try-job: i686-msvc
@bors
Copy link
Contributor

bors commented Jan 4, 2025

☀️ Try build successful - checks-actions
Build commit: f9d35b2 (f9d35b27eabb86eed0e2ffa57ea51d4433236d44)

@ChrisDenton
Copy link
Member

That seems to have worked! I'll mark this as iffy though, just in case.

@bors r+ iffy

@bors
Copy link
Contributor

bors commented Jan 4, 2025

📌 Commit 5e66869 has been approved by ChrisDenton

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 Jan 4, 2025
@ChrisDenton
Copy link
Member

oops I forgot the rollup=

@bors rollup=iffy

@bors
Copy link
Contributor

bors commented Jan 4, 2025

⌛ Testing commit 5e66869 with merge 2a8af4f...

@bors
Copy link
Contributor

bors commented Jan 4, 2025

☀️ Test successful - checks-actions
Approved by: ChrisDenton
Pushing 2a8af4f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 4, 2025
@bors bors merged commit 2a8af4f into rust-lang:master Jan 4, 2025
7 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 4, 2025
@bjorn3 bjorn3 deleted the cc_pass_arch_only branch January 4, 2025 18:51
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (2a8af4f): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (secondary -0.9%)

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)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.9% [-0.9%, -0.9%] 1
All ❌✅ (primary) - - 0

Cycles

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

Binary size

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

Bootstrap: 763.089s -> 762.575s (-0.07%)
Artifact size: 325.61 MiB -> 325.64 MiB (0.01%)

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. O-windows-msvc Toolchain: MSVC, Operating system: Windows 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.

9 participants