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 --use-winsysroot-style option to splat #119

Closed
wants to merge 1 commit into from

Conversation

sgraham
Copy link
Contributor

@sgraham sgraham commented May 22, 2024

This option changes the layout of the splat a little to support /winsysroot (ref: https://reviews.llvm.org/D95534) which is more convenient when compiling with clang-cl.

Example:
target\release\xwin --accept-license --arch x86_64 --variant desktop splat --include-debug-libs --include-debug-symbols --preserve-ms-arch-notation --disable-symlinks --use-winsysroot-style --output winsysroot

Then:
clang-cl.exe -fuse-ld=lld-link /winsysroot winsysroot src/main.cc

I didn't realize when I wrote it, but I see this was requested by someone else in Issue #51.

This option changes the layout of the splat a little to support
/winsysroot (ref: https://reviews.llvm.org/D95534) which is more
convenient when compiling with clang-cl.

Example:
`target\release\xwin --accept-license --arch x86_64 --variant desktop splat --include-debug-libs --include-debug-symbols --preserve-ms-arch-notation --disable-symlinks --use-winsysroot-style --output winsysroot`
@sgraham sgraham requested a review from Jake-Shadle as a code owner May 22, 2024 03:22
@sgraham sgraham changed the title Add --use-winsysroot-layout option to splat Add --use-winsysroot-style option to splat May 22, 2024
@MarijnS95
Copy link
Contributor

MarijnS95 commented May 22, 2024

Does this allow clang-cl to run with just one argument, instead of specifying all include folders with -imsvc by hand?

CL_FLAGS="-Wno-unused-command-line-argument -fuse-ld=lld-link /imsvc/xwin/crt/include /imsvc/xwin/sdk/include/ucrt /imsvc/xwin/sdk/include/um /imsvc/xwin/sdk/include/shared /imsvc/xwin/sdk/include/winrt" \

If so, might be nice to update the dockerfile with that new option.

And if the current file layout is "ad-hoc", maybe it makes sense to deprecate/remove it in favour of winsysroot?
EDIT: It looks like this layout is a relatively new concept, so it might be nice to introduce it directly as the standard layout for xwin to use?

@sgraham
Copy link
Contributor Author

sgraham commented May 22, 2024

Does this allow clang-cl to run with just one argument, instead of specifying all include folders with -imsvc by hand?

Yes, that's correct.

CL_FLAGS="-Wno-unused-command-line-argument -fuse-ld=lld-link /imsvc/xwin/crt/include /imsvc/xwin/sdk/include/ucrt /imsvc/xwin/sdk/include/um /imsvc/xwin/sdk/include/shared /imsvc/xwin/sdk/include/winrt" \

If so, might be nice to update the dockerfile with that new option.

I can change it blindly, but I don't know anything about docker. Will that get tested on a CI run?

And if the current file layout is "ad-hoc", maybe it makes sense to deprecate/remove it in favour of winsysroot? EDIT: It looks like this layout is a relatively new concept, so it might be nice to introduce it directly as the standard layout for xwin to use?

That would be nice for me as I'm only using the output with clang-cl on Windows. But I don't know how other people are using the outputs, so maybe it would break other workflows/compilers/platforms?

@MarijnS95
Copy link
Contributor

MarijnS95 commented May 22, 2024

That would be nice for me as I'm only using the output with clang-cl on Windows. But I don't know how other people are using the outputs, so maybe it would break other workflows/compilers/platforms?

I'm only using clang-cl from Linux to cross-compile, and manually setting up those /imsvc flags linked above from the Dockerfile. I'm totally happy to test this.

EDIT: It seems we could have already simplified with /winsdkdir (and /vctoolsdir?) before even heading into /winsysroot.

@sgraham
Copy link
Contributor Author

sgraham commented May 22, 2024

That would be nice for me as I'm only using the output with clang-cl on Windows. But I don't know how other people are using the outputs, so maybe it would break other workflows/compilers/platforms?

I'm only using clang-cl from Linux to cross-compile, and manually setting up those /imsvc flags linked above from the Dockerfile. I'm totally happy to test this.

Looking at the .dockerfile it looks like RUSTFLAGS would need to be changed in that case. I'm not sure if it's really easier then, because it appears it wants to point directly at the libdirs, and so they'll have version numbers in them in the case of doing /winsysroot.

EDIT: It seems we could have already simplified with /winsdkdir (and /vctoolsdir?) before even heading into /winsysroot.

Yes, I think so. I haven't used those (older?) versions myself.

@Jake-Shadle Jake-Shadle mentioned this pull request Jun 3, 2024
@MarijnS95
Copy link
Contributor

@Jake-Shadle any comment on whether it makes sense to use this as default format globally?

@Jake-Shadle
Copy link
Owner

I personally don't see the benefit of going from 2 -> 1 command line args, perhaps it's nicer for clang if library directories are also automatically inferred.

@MarijnS95
Copy link
Contributor

It'd simplify the code and testability here a little if there's only one set-in-stone directory layout. Especially if the original one isn't specifically based on anything (is it?).

@Jake-Shadle
Copy link
Owner

I'd consider it if this project had a maintenance burden, but it doesn't.

@sgraham
Copy link
Contributor Author

sgraham commented Jun 3, 2024

Thanks!

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.

3 participants