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

WiX: add custom actions to handle Clang resources #198

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

stevapple
Copy link
Contributor

@stevapple stevapple commented May 25, 2023

The Swift compiler requires Clang resources in /usr/lib/swift/clang to use Clang builtins. The layout and content is identical to /usr/lib/clang/<clang_version>, where we point a symlink on Unix platforms. Since symlinks work differently on Windows and require elevated permission, we copied the directory when building the toolchain, but failed to make it through the installer.

A prior attempt was made in #144 using HarvestDirectory, but @compnerd pointed out that it'll add to built time, so here comes the custom actions for handling it.

Closes apple/swift#60534

@stevapple stevapple changed the title WiX: add custom action to handle Clang resources WiX: add custom actions to handle Clang resources May 25, 2023
@compnerd
Copy link
Collaborator

There is a large refactoring that is currently being worked upon, lets hold off on this for a bit to get that through so we can make some progress on Swift in Swift for Windows.

@stevapple
Copy link
Contributor Author

I’m okay if it can catch up with the 5.9 release, or else we’d take it into the 5.9 branch first.

@compnerd
Copy link
Collaborator

Fortunately, this is limited to the installer - so I think that we should be able to bring it into the 5.9 branch once everything is settled.

@stevapple
Copy link
Contributor Author

Rebased upon latest changes, tested locally and ready for review @compnerd @shahmishal

@stevapple
Copy link
Contributor Author

Ping

@stevapple
Copy link
Contributor Author

Ping @shahmishal @compnerd

@stevapple
Copy link
Contributor Author

Ping

@compnerd
Copy link
Collaborator

There are a few more invasive changes planned for this, so I would rather that we wait a little bit. BTW, can you verify that this works without elevated privileges for the normal user which may not have the SeSymbolicLinkPrivilege enabled? The installer is going to drop the need for elevated privileges and switch to per-user installs. That should make it easier to enable the Swift-in-Swift use case.

Due to MSBuild restrictions, the `CustomActions` build is out-of-tree. We change the directory to `.output` so it won't go in the Git working tree.
@stevapple
Copy link
Contributor Author

stevapple commented Aug 9, 2023

@compnerd I think this is ready. I've tested locally with both per-machine (with UAC) and per-user installation (without UAC) and it works smoothly.

Since it's basically copy-and-paste, I believe there's nothing to do with SeSymbolicLinkPrivilege.

@compnerd
Copy link
Collaborator

compnerd commented Aug 9, 2023

@stevapple oh, of course! It doesn't need that because it is copying not creating a junction! Okay, that makes sense. BTW, what is the motivation for this again? With the latest fixes, atomics should build fine without this link which I believe is what prompted you to do this in the first place? Could you also please check that this also cleans up on uninstall?

@stevapple
Copy link
Contributor Author

BTW, what is the motivation for this again? With the latest fixes, atomics should build fine without this link which I believe is what prompted you to do this in the first place?

This is required to bring consistency between what the Swift compiler sees and Clang sees — by exposing Clang’s internal resources to Swift. Atomics is just an example where Clang’s header is involved.

Without the piece, the Swift compiler may expand the header differently with Clang, causing mismatch when importing Clang targets. It’s also necessary for Swift driver to link against libclang_rt for profiling.

Could you also please check that this also cleans up on uninstall?

Already checked✅

@compnerd
Copy link
Collaborator

BTW, what is the motivation for this again? With the latest fixes, atomics should build fine without this link which I believe is what prompted you to do this in the first place?

This is required to bring consistency between what the Swift compiler sees and Clang sees — by exposing Clang’s internal resources to Swift. Atomics is just an example where Clang’s header is involved.

I think that this is going in the opposite direction. Swift sees MSVC's headers, clang should see the same. I believe that the bug in modularisation was the cause of the issue. Other than swift-atomics (which I just tested with SPM) do you have any concrete examples of failures?

Without the piece, the Swift compiler may expand the header differently with Clang, causing mismatch when importing Clang targets. It’s also necessary for Swift driver to link against libclang_rt for profiling.

I'd be curious to understand this first. I don't see this as being the case as at all. The profiling support already works properly.

https://github.com/compnerd/swift-win32/blob/main/.github/workflows/coverage.yml#L42 is a concrete example, --enable-code-coverage to SPM already works. Could you share the exact failure with profiling that you are observing?

Could you also please check that this also cleans up on uninstall?

Already checked✅

Perfect!

@stevapple
Copy link
Contributor Author

stevapple commented Aug 11, 2023

I think that this is going in the opposite direction. Swift sees MSVC's headers, clang should see the same. I believe that the bug in modularisation was the cause of the issue. Other than swift-atomics (which I just tested with SPM) do you have any concrete examples of failures?

Both Swift and Clang can see MSVC and other global headers, but Clang has internal headers acting as shims. Usually they won't affect existing headers, but they potentially can. Atomics is just an example where Clang provides its own alternative stdatomic.h with LLVM intrinsics. Swift should see these headers since it will consume Clang modules that may rely on them.

I'd be curious to understand this first. I don't see this as being the case as at all. The profiling support already works properly.

compnerd/swift-win32@main/.github/workflows/coverage.yml#L42 is a concrete example, --enable-code-coverage to SPM already works. Could you share the exact failure with profiling that you are observing?

This is because Swift Driver's profile generation support is implemented differently on Windows than on Linux. Take a look at the highlighted lines below:

On Linux, Swift Driver will find libclang_rt exactly in usr/lib/swift/clang/lib and pass the absolute path to linker. On Windows we simply hand -lclang_rt over to the linker — which is Clang itself. (In theory we can say the linked library is path/to/swift/../lib/swift/clang/lib/linux/libclang_rt.profile-x86_64.a for Linux and path/to/clang/.../lib/clang/x.y.z/lib/windows/clang_rt.profile-x86_64.lib for Windows. They're totally different paths.)

I didn't mean to judge which way is better, but it's cleat that on every other platform we assume Clang resources are accessible in usr/lib/swift/clang and we hard-code the path for every platform in the driver (https://github.com/apple/swift-driver/blob/688a840aa3334e462ae9f648f3d3eaac017d8299/Sources/SwiftDriver/Jobs/Toolchain%2BLinkerSupport.swift#L25-L37).

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.

Swift doesn’t recognize Clang’s header on Windows
2 participants