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

Add i686-pc-windows-gnullvm target #2961

Merged
merged 1 commit into from
Apr 3, 2024
Merged

Add i686-pc-windows-gnullvm target #2961

merged 1 commit into from
Apr 3, 2024

Conversation

jeremyd2019
Copy link
Contributor

Adds i686-pc-windows-gnullvm target.

Fixes: #2960

@jeremyd2019
Copy link
Contributor Author

jeremyd2019 commented Apr 1, 2024

On my fork, the "cross" test fails now. Oddly, I modified the "cross" workflow first, and it actually succeeded before I started adding i686_gnullvm elsewhere. I guess it's testing a mix between the current git crates/libs/targets and older published crates/targets/..., where there is no such i686_gnullvm version? That was the only failure I was seeing though.

@jeremyd2019 jeremyd2019 changed the title add i686-pc-windows-gnullvm target Add i686-pc-windows-gnullvm target Apr 1, 2024
@jeremyd2019
Copy link
Contributor Author

jeremyd2019 commented Apr 2, 2024

On my fork, the "cross" test fails now.

I'm an idiot, the build.rs I based on x86_64_gnullvm, and changed arch != "x86_64" to arch != "i686", but looking at i686_gnu it should have been x86. That was the cause of the failure.

@riverar riverar requested a review from kennykerr April 2, 2024 00:46
@jeremyd2019
Copy link
Contributor Author

Should i686_gnu's build.rs be modified also, to add || !target.ends_with("-gnu") as is present in x86_64_gnu? I only just noticed that.

@kennykerr
Copy link
Collaborator

kennykerr commented Apr 2, 2024

I think that came from #2774 courtesy of @seritools.

@kennykerr
Copy link
Collaborator

Tests pass, so I'm going to merge this PR. If you have a repro do let us know. Thanks for the contribution!

@kennykerr kennykerr merged commit a08ab56 into microsoft:master Apr 3, 2024
69 checks passed
@jeremyd2019
Copy link
Contributor Author

If you have a repro do let us know.

I now believe that the "cross" test I added failed to fail as I expected because of #2774. Specifically,

if family != "windows" || arch != "x86" || env != "gnu" {
does not explicitly exclude -gnullvm as is done in
let target = std::env::var("TARGET").unwrap();
if family != "windows" || arch != "x86_64" || env != "gnu" || !target.ends_with("-gnu") {

@kennykerr
Copy link
Collaborator

Some failure tests would be great, although I'm not sure how exactly it would fail since windows-targets would/should still exclude it?

@jeremyd2019
Copy link
Contributor Author

I'm talking about the status quo before this PR was merged - the cfg expression in

[target.'cfg(all(target_arch = "x86", target_env = "gnu", not(windows_raw_dylib)))'.dependencies]
windows_i686_gnu = { path = "../../targets/i686_gnu", version = "0.52" }
does not check target_abi, so both i686-pc-windows-gnu and -gnullvm would match, pulling in windows_i686_gnu. The build.rs of i686_windows_gnu before #2774 would have returned before adding the lib directory because the target was not exactly i686-pc-windows-gnu or i686-uwp-windows-gnu. After #2774, it would not return early, because the env is gnu for gnullvm, and it does not do the target.ends_with check that would exclude gnullvm.

@kennykerr
Copy link
Collaborator

Do you have a repro? I don't see that here:

D:\git\windows-rs>rustup default
nightly-i686-pc-windows-gnu (default)

D:\git\windows-rs>cargo check -p windows-targets
   Compiling windows_i686_gnu v0.52.4 (D:\git\windows-rs\crates\targets\i686_gnu)
    Checking windows-targets v0.52.4 (D:\git\windows-rs\crates\libs\targets)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.53s

@jeremyd2019
Copy link
Contributor Author

jeremyd2019 commented Apr 3, 2024

C:\>curl -LO https://github.com/mstorsjo/llvm-mingw/releases/download/20231128/llvm-mingw-20231128-ucrt-x86_64.zip
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100  128M  100  128M    0     0  26.8M      0  0:00:04  0:00:04 --:--:-- 34.9M

C:\>tar -xf llvm-mingw-20231128-ucrt-x86_64.zip

C:\>git clone --depth 1 --single-branch -b 0.52.0 https://github.com/microsoft/windows-rs

C:\windows-rs>path c:\llvm-mingw-20231128-ucrt-x86_64\bin;%PATH%

C:\windows-rs>rustup default
nightly-i686-pc-windows-gnu (default)

C:\windows-rs>rustup component add rust-src
info: downloading component 'rust-src'
info: installing component 'rust-src'
C:\windows-rs>del \llvm-mingw-20231128-ucrt-x86_64\bin\*gcc.exe

C:\windows-rs>del \llvm-mingw-20231128-ucrt-x86_64\bin\*g++.exe

C:\windows-rs>cargo test -p test_win32 --target i686-pc-windows-gnullvm -Zbuild-std

...
  = note: clang-17: warning: argument unused during compilation: '-nolibc' [-Wunused-command-line-argument]
          clang-17: warning: argument unused during compilation: '-no-pie' [-Wunused-command-line-argument]
          lld: error: unable to find library -lwindows.0.52.0
          lld: error: unable to find library -lwindows.0.52.0
          clang-17: error: linker command failed with exit code 1 (use -v to see invocation)


error: could not compile `test_win32` (test "winsock") due to 1 previous error
warning: `test_win32` (test "win32") generated 3 warnings
error: could not compile `test_win32` (test "win32") due to 1 previous error; 3 warnings emitted

C:\windows-rs>cd ..

C:\>rmdir /s windows-rs
windows-rs, Are you sure (Y/N)? y
C:\>git clone --depth 1 --single-branch https://github.com/microsoft/windows-rs

C:\>cd windows-rs

C:\windows-rs>cargo test -p test_win32 --target i686-pc-windows-gnullvm -Zbuild-std

...
     Running unittests src\lib.rs (target\i686-pc-windows-gnullvm\debug\deps\test_win32-a6badaa2d7e7e7e5.exe)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests\hresult.rs (target\i686-pc-windows-gnullvm\debug\deps\hresult-3b4ba2f37ba503d2.exe)

running 1 test
test test_message ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests\win32.rs (target\i686-pc-windows-gnullvm\debug\deps\win32-e90a6c871585c2a0.exe)

running 14 tests
test callback ... ok
test size32 ... ok
test bool_as_error ... ok
test constant ... ok
test dxgi_mode_desc ... ok
test function ... ok
test rect ... ok
test com ... ok
test signed_enum32 ... ok
test unsigned_enum32 ... ok
test empty_struct ... ok
test interface ... ok
test com_inheritance ... ok
test onecore_imports ... ok

test result: ok. 14 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.13s

     Running tests\winsock.rs (target\i686-pc-windows-gnullvm\debug\deps\winsock-e6e3a30783a5f0b9.exe)

running 6 tests
test in6_addr ... ok
test sockaddr_inet6 ... ok
test sockaddr_inet4 ... ok
test sockaddr_in ... ok
test sockaddr_in6 ... ok
test in_addr ... ok

test result: ok. 6 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s

Is that what you're looking for? BTW, as expected, 0.53.0 through 0.55.0 succeed due to the build.rs change referenced above. I hadn't noticed that before, I was dealing with breakages of 0.52.0 and 0.48.x.

@jeremyd2019
Copy link
Contributor Author

Also, once rust-lang/rust#121712 goes somewhere, it should be possible to get more test coverage of gnullvm (without having to use nightly so that -Zbuild-std can be used)

@riverar
Copy link
Collaborator

riverar commented Apr 4, 2024

@jeremyd2019 Is there any remaining work to be done here? I can't quite tell by reading the latest messages here.

@jeremyd2019
Copy link
Contributor Author

I created #2970 for completeness' sake. The only other thing is I want to try to add more testing of the -gnullvm targets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add i686-pc-windows-gnullvm target
3 participants