Add workflow to build SwiftFormat. #544
Conversation
There was a problem hiding this comment.
@tristanlabelle I think that this would a good time to investigate the GUID handling.
There was a problem hiding this comment.
Looking good! It would be nice to take this opportunity to try out simplifying the .wxs file from the guidance we got in https://github.com/orgs/wixtoolset/discussions/7337 . (Could be in a follow-up PR)
|
|
||
| <!-- Components --> | ||
| <ComponentGroup Id="SwiftFormat"> | ||
| <Component Id="swiftformat.exe" Directory="Tools" Guid="77126634-5f91-40a7-b344-035ce99ef46f"> |
There was a problem hiding this comment.
This would be a good opportunity to try removing Component-specific guids. I believe ComponentGuidGenerationSeed enables all of these guids to be generated deterministically. Full discussion here:
https://github.com/orgs/wixtoolset/discussions/7337
| <Component Id="swiftformat.exe" Directory="Tools" Guid="77126634-5f91-40a7-b344-035ce99ef46f"> | |
| <Component Id="swiftformat.exe" Directory="Tools"> |
There was a problem hiding this comment.
@tristanlabelle I'm getting this error after dropping Guid=...
D:\a\swift-build\swift-build\SourceCache\installer-scripts\SwiftFormat.wxs(35): error WIX0230: The Component/@Guid attribute's value '*' is not valid for this component because it does not meet the criteria for having an automatically generated guid. Components using a Directory as a KeyPath or containing ODBCDataSource child elements cannot use an automatically generated guid. Make sure your component doesn't have a Directory as the KeyPath and move any ODBCDataSource child elements to components with explicit component guids. [D:\a\swift-build\swift-build\SourceCache\installer-scripts\SwiftFormat.wixproj]
Component indeed has a Directory attribute:
<Component Id="swiftformat.exe" Directory="Tools">
There was a problem hiding this comment.
I wonder if we should just merge the directory layout and component bits. That would allow us to remove the Directory attribute right?
There was a problem hiding this comment.
Ah if that syntax is valid then that would make sense! I was wondering what was the alternative to what we are doing.
There was a problem hiding this comment.
@compnerd Any examples or reference on how to merge directory/component? Apple's wsx files follow this unmerged pattern.
There was a problem hiding this comment.
@hjyamauchi the wxs files there were authored by me and originally in this repository :)
I think that the ComponentGroup will need to be replaced by Components as per the WiX Schema: https://wixtoolset.org/docs/schema/wxs/directory/
There was a problem hiding this comment.
Ok I think I found a way to avoid guid in the .exe component by removing the component group and moving the component into a directory. But it still complains about the EnvironmentVariables component's missing guid when it's moved into a directory.
There was a problem hiding this comment.
According to this: "Generatable guids are supported only for components with a single file as the component's keypath or no files and a registry value as the keypath." It sounds like the .exe component can have generated guid but not the environment variable one because the latter doesn't have a file or registery value as a child in this case.
There was a problem hiding this comment.
#549 is what I got far. @compnerd @tristanlabelle Do you see a way to use autogenerated GUID for the environment variable component?
There was a problem hiding this comment.
I don't but it's probably no big deal? I think the guids were local to the installer and even if they were not, it might make sense for the guids to clash if you tried to run multiple installers setting the same environment variable. (not sure)
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <Compile Include="SwiftFormat-$(ProductArchitecture).wxs" /> |
There was a problem hiding this comment.
This is also a good opportunity to prototype having a single .wxs file for both architectures and using variables to account for any differences. It would inform if we can do the same thing for the toolset.
There was a problem hiding this comment.
I think the wixtoolset maintainer also said that component GUIDs were not system-global so we could consolidate architectures even if we can't figure out how to remove the GUIDs.
| runs-on: windows-latest | ||
|
|
||
| strategy: | ||
| matrix: |
There was a problem hiding this comment.
I'm not super familiar with github actions but do we need the matrix here if we'll be building the same tag/branch on all architectures of the build?
There was a problem hiding this comment.
It is originally in @compnerd action because the plan is that this is built for an arbitrary number of toolchain releases when (a future version of) this is upstreamed to nick's SwiftFormat repo. Right now, the matrix is no-op.
There was a problem hiding this comment.
The matrix I was using was for the Swift compiler version. Since we are not ABI stable on Windows, we need the release version matrix for the compile still as the standard library will change.
There was a problem hiding this comment.
@mangini did we test building with the 5.7 toolchain and running against a 5.9 toolchain? I'm worried about the ABI instability in the runtime meaning that we won't be able to use the releases with newer toolchains.
There was a problem hiding this comment.
@compnerd Do you mean that this SwiftFormat binary is dynamically-linked and that its version needs to include the version of the swift compiler that compiled it? If so, I interpret it as that the matrix needs to have both the compiler version and the SwiftFormat version.
There was a problem hiding this comment.
@hjyamauchi correct. swiftCore, Foundation, and dispatch are always dynamically linked, and the version needs to be the version from the runtime distributed with the toolchain that it was built with. Extending the matrix to both sounds right.
hjyamauchi
force-pushed
the
SwiftFormat
branch
2 times, most recently
from
f741fc3 to
a4c6244
Compare
|
Will handle the wix improvement in a follow up PR and go ahead with this PR in the interest of making SwiftFormat available sooner. |
hjyamauchi
force-pushed
the
SwiftFormat
branch
2 times, most recently
from
7229ad5 to
62d60e4
Compare
|
Merging to test and get a build |
No description provided.