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

detect-targets: Add fallback to windows #650

Merged
merged 1 commit into from
Jun 4, 2023

Conversation

NobodyXu
Copy link
Member

@NobodyXu NobodyXu commented Jan 4, 2023

Fixed #642

  • Add new dep windows-sys v0.42.0 for win only
  • Add new dep windows-dll v0.4.1 for win only
  • Add x86_64 fallback targets for windows
  • Add x86 fallback targets for windows
  • Add arm32 fallback targets for windows
  • Add arm64 fallback targets for windows
  • Add gnu/gnu-llvm fallback targets for windows

Signed-off-by: Jiahao XU Jiahao_XU@outlook.com

@NobodyXu NobodyXu force-pushed the feat/detect-targets/windows-x64-on-aarch64 branch from a4e39a7 to 8314cb3 Compare January 4, 2023 07:29
@NobodyXu NobodyXu requested a review from a team January 4, 2023 07:37
@NobodyXu NobodyXu added the help wanted Extra attention is needed label Jan 4, 2023
@NobodyXu
Copy link
Member Author

NobodyXu commented Jan 4, 2023

The CI has failed silently on windows with no error message or whatever.

The integration CI failed with exit code 127.

@NobodyXu NobodyXu changed the title detect-targets: Fallback to x64 on aarch64 detect-targets: Add fallback to windows Jan 4, 2023
@passcod
Copy link
Member

passcod commented Jan 4, 2023

Possibly to do with that API's requirement of windows build 22000. Maybe we can test for the build number and only call this if it's >=22000?

@NobodyXu
Copy link
Member Author

NobodyXu commented Jan 4, 2023

Possibly to do with that API's requirement of windows build 22000.

Aha, so it panics instead of returning an error?

Maybe we can test for the build number and only call this if it's >=22000?

How do we test that?

Edit:

I've found GetVersion which is deprecated after windows 8, GetProductInfo which seems to be a replacement for GetVersion, then finally IsWindowsVersionOrGreater.

@NobodyXu
Copy link
Member Author

NobodyXu commented Jan 4, 2023

Possibly to do with that API's requirement of windows build 22000. Maybe we can test for the build number and only call this if it's >=22000?

And that would also means that we cannot test the fallback logic on GHA...

@NobodyXu
Copy link
Member Author

NobodyXu commented Jan 4, 2023

Possibly to do with that API's requirement of windows build 22000. Maybe we can test for the build number and only call this if it's >=22000?

@passcod I added a version check using GetVersionExA, but now the tests failed with exit code: 0xc0000139 in unit test and still failed with exit code 127 in integration test.

@passcod
Copy link
Member

passcod commented Jan 4, 2023

hmm, 0xc0000139 is a windows exception for failing to load a dll, so I guess it could be a need to detect even earlier... but I don't really know at this point.

@NobodyXu NobodyXu force-pushed the feat/detect-targets/windows-x64-on-aarch64 branch from c7b2381 to f5b6bee Compare January 4, 2023 10:20
@NobodyXu
Copy link
Member Author

NobodyXu commented Jan 4, 2023

hmm, 0xc0000139 is a windows exception for failing to load a dll, so I guess it could be a need to detect even earlier... but I don't really know at this point.

So basically it's giving out runtime link error because GetMachineTypeAttributes isn't available...
The only option seems to be using a min-size x64 binary, or maybe we can run an external shell command on windows @passcod ?

I thought about using something like dlsym but I don't know that windows counterpart and not sure how to the raw function pointer obtained.

@NobodyXu NobodyXu linked an issue Jan 4, 2023 that may be closed by this pull request
@passcod
Copy link
Member

passcod commented Jan 4, 2023

Apparently the proper way to do this is to dynamically load Kernel32.dll with any of LoadLibraryA, LoadLibraryExA, LoadPackagedLibrary, or GetModuleHandleExA (dunno which to prefer, maybe try the last one given kernel is... probably always loaded? idk. Then use GetProcAddress to lookup and check the function exists.

@NobodyXu
Copy link
Member Author

NobodyXu commented Jan 4, 2023

@passcod Thanks, this makes me even more appreciate the separation of libc and syscalls in linux, meaning that for missing syscalls we would simply get a ENOSYS.

@passcod
Copy link
Member

passcod commented Jan 4, 2023

Might be simpler to try using delay-loading:

fn main() {
    println!("cargo:rustc-link-arg=/DELAYLOAD:Kernel32.dll");
    println!("cargo:rustc-link-lib=static=delayimp");
}

which if I'm reading some threads correctly means it should error normally (via result) at call time...

@passcod
Copy link
Member

passcod commented Jan 4, 2023

welp, nevermind:

Delay loading Kernel32.dll isn't supported. This DLL must be loaded for the delay-load helper routines to work.

@passcod
Copy link
Member

passcod commented Jan 4, 2023

windows-dll crate seems to handle the machinery for us, if it's too hard to get working directly. With default-features = false, features = ["windows"] it will also not use the old winapi crate

@NobodyXu
Copy link
Member Author

NobodyXu commented Jan 4, 2023

@passcod Thanks, I would use it to wrap GetMachineTypeAttributes, which I believe is the raw C/C++ symbol for the function.

@NobodyXu NobodyXu marked this pull request as ready for review January 4, 2023 14:22
@NobodyXu NobodyXu enabled auto-merge (squash) January 4, 2023 14:25
@NobodyXu NobodyXu requested a review from passcod January 4, 2023 14:26
@NobodyXu
Copy link
Member Author

NobodyXu commented Jan 4, 2023

@passcod I've fixed the code according to your advice, but we can't test the code on the GHA since it's still on windows 10.

If you have a windows 11 build system with cargo installed, can you run the tests locally please?

While I have a windows 11 laptop, it's up for sale so I can't install cargo on it.

@NobodyXu
Copy link
Member Author

@passcod If you are free and have access to a windows machine, can you test this PR please?

@NobodyXu
Copy link
Member Author

It's been stale for a really long time.

@passcod
Copy link
Member

passcod commented Mar 11, 2023

Going to do this today!

@passcod
Copy link
Member

passcod commented Mar 11, 2023

Well it builds and the standard test suite runs. I wasn't able to run e2e-tests yet because windows is a poopy head. Will try more tomorrow.

@NobodyXu
Copy link
Member Author

Trying to run this on windows 11 gives me:

image

Windows is such a pain to deal with

@NobodyXu
Copy link
Member Author

This PR has been stale for quite long.
@passcod What should we do with this PR?
Maybe we could just release it and roll it back if it goes seriously wrong?

@NobodyXu
Copy link
Member Author

Or maybe we can just drop it.
Even running cargo-binstall on windows is a pain.

@passcod
Copy link
Member

passcod commented May 14, 2023

Yeah, let's drop it. Someone who wants to get those can use --target manually, and we've had no feedback that this is a missing feature really.

@NobodyXu NobodyXu closed this May 14, 2023
@NobodyXu NobodyXu deleted the feat/detect-targets/windows-x64-on-aarch64 branch May 14, 2023 09:53
@NobodyXu
Copy link
Member Author

NobodyXu commented Jun 3, 2023

Trying to run this on windows 11 gives me:

image

Windows is such a pain to deal with

Just realize that I failed to extract the file and run it directly in file explorer preview like #1099 (comment), so I want to reopen this and reevaluate this.

@NobodyXu NobodyXu restored the feat/detect-targets/windows-x64-on-aarch64 branch June 3, 2023 11:18
@NobodyXu NobodyXu reopened this Jun 3, 2023
* Add new dep windows-sys v0.42.0 for win only
* Add new dep windows-dll v0.4.1 for win only
* Add x86_64 fallback targets for windows
* Add x86 fallback targets for windows
* Add arm32 fallback targets for windows
* Add arm64 fallback targets for windows
* Add gnu/gnu-llvm fallback targets for windows

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu NobodyXu force-pushed the feat/detect-targets/windows-x64-on-aarch64 branch from 8bf07f7 to 6b34a7c Compare June 3, 2023 11:22
@NobodyXu
Copy link
Member Author

NobodyXu commented Jun 3, 2023

I successfully run this on my aarch64 windows VM.

It checks for target:

  • aarch64-pc-windows-msvc
  • aarch64-pc-windows-gnu
  • aarch64-pc-windows-gnullvm
  • x86_64-pc-windows-msvc
  • x86_64-pc-windows-gnu
  • x86_64-pc-windows-gnullvm
  • i586-pc-windows-msvc
  • i586-pc-windows-gnu
  • i586-pc-windows-gnullvm
  • i686-pc-windows-msvc
  • i686-pc-windows-gnu
  • i686-pc-windows-gnullvm

It's working as intended and given that the CI also passes, so I guess this is mostly ok-ish.
@passcod can you review this PR and approve this if it looks ok to you?

@NobodyXu NobodyXu added this pull request to the merge queue Jun 4, 2023
@passcod
Copy link
Member

passcod commented Jun 4, 2023

Yeah let's give it a go. Best test is to put it in front of people, we can always revert if some windows user finds an edge case.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 4, 2023
@NobodyXu NobodyXu added this pull request to the merge queue Jun 4, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 4, 2023
@NobodyXu NobodyXu added this pull request to the merge queue Jun 4, 2023
Merged via the queue into main with commit e87e353 Jun 4, 2023
26 checks passed
@NobodyXu NobodyXu deleted the feat/detect-targets/windows-x64-on-aarch64 branch June 4, 2023 04:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed PR: improvement PR that improves existing features or code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Add x86_64 fallback for aarch64 windows in detect-targets
2 participants