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 missing \usr\lib\swift\clang #144

Closed
wants to merge 1 commit into from

Conversation

stevapple
Copy link
Contributor

This is required for Swift compiler to consume Clang resources correctly on Windows.

Fixes swiftlang/swift#60534

@stevapple
Copy link
Contributor Author

@compnerd You said that CMake could be used for Atomics, but that's not true for released toolchains — it only works with a development build, where \usr\lib\swift\clang was just there.

@stevapple
Copy link
Contributor Author

These two harvested directories are mostly the same (except that ClangResources are prefixed with Clang version), but I'm not so familiar with WiX to reuse them. This just works (with least modification).

@stevapple
Copy link
Contributor Author

Ping

@shahmishal shahmishal requested a review from compnerd February 8, 2023 01:39
@shahmishal
Copy link
Member

cc: @compnerd

@stevapple
Copy link
Contributor Author

Ping

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

This is not the place to do this. There is no need for the duplicated content - it already is in \usr\lib\clang. The Swift driver should be taught to adjust the -resource-dir.

@stevapple
Copy link
Contributor Author

The Swift driver should be taught to adjust the -resource-dir.

Where do you want to adjust the resource dir to? The resource directory for Swift is /usr/lib/swift for all platforms, and for Clang that’s /usr/lib/clang. We need to expose Clang’s extra resource headers to Swift, that’s why we have a symlink on other platforms.

@compnerd
Copy link
Member

The Swift driver should be taught to adjust the -resource-dir.

Where do you want to adjust the resource dir to? The resource directory for Swift is /usr/lib/swift for all platforms, and for Clang that’s /usr/lib/clang. We need to expose Clang’s extra resource headers to Swift, that’s why we have a symlink on other platforms.

/usr/lib/clang, as the content is just duplicating it. The other platforms have symlinks and de-duplicates that data, Windows does not (and we cannot create them without Administrator rights and a custom action and C++ code).

@stevapple
Copy link
Contributor Author

/usr/lib/clang, as the content is just duplicating it.

Plz don’t do that😅 We have plenty of other stuffs in /usr/lib/swift as Swift resources and this is a conventional path. If you don’t like the idea of symlink (I don’t like it, too), then we’ll end up duplicating this.

@compnerd
Copy link
Member

I don't think that duplicating it is reasonable, the toolchain is growing far too fast. Having the installer hit gigabytes is not a good idea (we are already pushing the boundary on the sizes). I'm working towards restructuring the MSIs to reduce some of the high pressure on the size.

@stevapple
Copy link
Contributor Author

We first need to make things work™ in a correct way — even if that’s not optimal, since we can always refine later.

If size limit cannot be worked around, then we need to bring back custom actions for copying/removing Clang resources for Swift automatically.

@compnerd
Copy link
Member

I don't think that is reasonable. The builds are taking longer and longer, we have increased build times ~100% already, we need to do this the proper way the first time around.

@stevapple
Copy link
Contributor Author

stevapple commented Mar 13, 2023

The build time increase is: 1. not Windows-specific, 2. almost not impacted by packaging. This shouldn’t affect decisions on packaging — I believe you had better complain to the Swift compiler & LLVM team.

@compnerd
Copy link
Member

The packaging is part of the build times.

@stevapple
Copy link
Contributor Author

How was that related? When we had custom actions before, it only takes less than 30 seconds to build, compared to over 5 hours of toolchain (LLVM & Swift) build time. How would you think adding a one-minute step hugely affects the total build time?

@compnerd
Copy link
Member

The compression overheads add a significant amount of time. I'd like to reduce the overzealous compression that we are reliant on currently. Additionally, since the motivation here is the swift-atomics, I'd suggest we understand why building with CMake works and with SPM does not. The same layout is used by both.

@stevapple
Copy link
Contributor Author

You’re making false assumptions again and again around this problem. Building Atomics with CMake never works for a released toolchain because it’s missing Clang resources. If you use a freshly-built toolchain instead, CMake and SwiftPM both work out of the box.

@stevapple
Copy link
Contributor Author

The compression overheads add a significant amount of time.

BTW, using a custom action can avoid adding new resources into the installer, so compression time won’t be affected.

@compnerd
Copy link
Member

You’re making false assumptions again and again around this problem. Building Atomics with CMake never works for a released toolchain because it’s missing Clang resources. If you use a freshly-built toolchain instead, CMake and SwiftPM both work out of the box.

I've built it locally, it wasn't a false assumption. Isolating and resolving those differences is important to make sure that things remain working overtime. A fair amount of work is going into reducing those differences.

@compnerd
Copy link
Member

Yes, the custom action should resolve that. Would you be up for writing a new custom action for that? I think that getting a new toolchain snapshot is a bit more pressing atm, so I won't be able to do that for a while. Guess that we won't be able to support per-user installation after all.

@stevapple
Copy link
Contributor Author

@compnerd I dug into the Atomic case again and still believe CMake is identical to SwiftPM on handling Clang headers.

The good news is: Visual Studio 17.5 introduces support for C11 stdatomic.h, so now Atomics can be built on even older toolchains.

With older MSVC both CMake and SwiftPM should fail to build it because MSVC version of stdatomic.h is either non-existent or unusable.

@stevapple
Copy link
Contributor Author

Superceded by #198

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
3 participants