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

Windows cross-compile support on Linux #2841

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

boberfly
Copy link

Very similar to #2758 but it uses the windows toolchain via https://github.com/Jake-Shadle/xwin

@boberfly boberfly force-pushed the windowsCrossCompile branch 3 times, most recently from fe125bf to 4c5c4fd Compare April 23, 2024 08:44
@boberfly
Copy link
Author

Hmm odd that the CI can't find <stdio.h> it is finding it on my local machine here...

@nurmukhametov
Copy link
Collaborator

Hmm odd that the CI can't find <stdio.h> it is finding it on my local machine here...

It looks like you install winsdk in $LLVM_HOME/xwin but provide $ISPC_HOME/xwin to CMake.

@nurmukhametov nurmukhametov self-requested a review April 23, 2024 11:26
Copy link
Collaborator

@nurmukhametov nurmukhametov left a comment

Choose a reason for hiding this comment

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

In superbuild, we need to pass value of winsdk path to ISPC CMake, something like this is needed around line 723-724:

if (NOT WIN32 AND NOT APPLE)
    if (ISPC_CROSS)
        list(APPEND ISPC_COMMON_CMAKE_ARGS
             -DISPC_WINDOWS_SDK_PATH=${ISPC_WINDOWS_SDK_PATH}
        )
    endif()
endif()

There is also this place https://github.com/ispc/ispc/blob/main/cmake/GenerateBuiltins.cmake#L294-L300 that maybe needs to be changed. Please, leave a TODO there.

cmake/GenerateBuiltins.cmake Outdated Show resolved Hide resolved
@boberfly
Copy link
Author

Hmm odd that the CI can't find <stdio.h> it is finding it on my local machine here...

It looks like you install winsdk in $LLVM_HOME/xwin but provide $ISPC_HOME/xwin to CMake.

Ahh silly me my mind was registering that LLVM_HOME as ISPC_HOME thanks for that!

Copy link
Collaborator

@dbabokin dbabokin left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, I like the proposed changes, it create a good path for solving the issue that we had for some time.

I guess the intention is not just enable Linux->Windows cross compilation for developer builds, but enable it for release builds. If Windows SDK license allows this, I guess we can do that.

@@ -87,6 +87,8 @@ for:
export CROSS_TOOLS=/usr/local/src/cross
export WASM=OFF
export LLVM_TAR=llvm-17.0.6-ubuntu18.04-Release+Asserts-x86.arm.wasm.tar.xz
export XWIN_HOME=$APPVEYOR_BUILD_FOLDER/xwin
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the approach with xwin.

But I'm concerned that it's a personal project, which we supposed to download from personal github releases of project and use in the CI. The project is nice and it's not too hard to review it to make sure that it doesn't pose a security threat. But there's no mechanism to verify that it's not modified (even if we use a fixed version), unless we download a SHA256 and keep in our secrets (which is also a possible mitigation).

What would solve this problem is to distribute xwin through one of the package managers. Do you know if it's available through one of them or is it possible to request that?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah that's definitely a valid point, I've not got the answer to this one except for perhaps using cargo directly at this point. Maybe we can get @Jake-Shadle into the conversation about this one as well.

Choose a reason for hiding this comment

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

I never package projects I maintain, if you don't trust the github releases you can use cargo install I suppose to build it yourself before running it, though obviously at the cost of a lot of CPU work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Jake-Shadle Thanks for commenting on that and thanks for the nice tool.

This is the code that runs in trusted environment, i.e. has access to github secrets, affects what is linked in to the binaries we ship, etc. So it requires some procedures in place to ensure security and integrity of the result. And operating in enterprise environment requires taking these kind of things more serious.

So I guess we can cache the SHA256 of the package in out secrets and use the file from github releases. That's would be the easiest for everyone.

But that would really, really help adopting xwin if it's available through some package manager. apt, snap, or whatever.

Choose a reason for hiding this comment

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

IMO it would be easiest to just create a tarball or container image with the CRT/SDK in it that you can download in your CI, they don't change frequently and it means you don't run any code at all.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we run it in public CI, then the image needs to be in public, this implies that we are responsible for it and we need to review with security people, maintain, etc. And this is something we definitely don't want to do.

xwin seems to be ideal tool for that purpose. If it would only be shipped though some package manager... ;-)

@@ -243,6 +241,9 @@ function(builtin_to_cpp bit os_name arch supported_archs supported_oses resultFi
if (${os_name} STREQUAL "macos")
# -isystem/iusers/MacOSX10.14.sdk.tar/MacOSX10.14.sdk/usr/include
set(includePath -isystem${ISPC_MACOS_SDK_PATH}/usr/include)
elseif (${os_name} STREQUAL "windows")
# -isystem/xwin/crt/include -isystem/xwin/sdk/include/ucrt
set(includePath -isystem${ISPC_WINDOWS_SDK_PATH}/crt/include -isystem${ISPC_WINDOWS_SDK_PATH}/sdk/include/ucrt)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@nurmukhametov Couple you please verify that "default" build without cross compilation enabled is not affected?

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.

None yet

4 participants