Skip to content

[Packaging] Add No-asserts toolchain to setup #425

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

Draft
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

mhegazy
Copy link

@mhegazy mhegazy commented May 20, 2025

This is a follow up to swiftlang/swift#81644 which adds build support for no-asserts toolchain variant along with the default asserts variant.

Note to reviewers: Reviewing this change one commit at a time should give you a better experience than looking at all changes at once.

Changes:

  • Split bld.msi, ide.msi, cli.msi and dbg.msi into two separate msi's each. the current authoring with no changes (Same upgrade codes, and component names) goes to a <product>.asserts.msi and <product>.asserts.cab, and a new msi is created using the same authoring but new upgrade codes to <product>.noasserts.msi and <product>.noasserts.cab
  • The new msi's are only built if the command line option is passed to build.cmd/build.ps1. this ensures that build time do not regress as a result of this change
  • To enable that without duplication, the authoring is parametrized.; the authoring for bld.msi, ide.msi, cli.msi and dbg.msi moves from <product>.wxs to <product>.wxi and is shared between the two msi variants.
    • To see the differences few differences between the two msi's look at asserts/<product>.asserts.wxs vs noasserts/<product>.noasserts.wxs
  • Create a new folder structure for no-asserts toolchain, by duplicating all our folders under ToolchainsVersioned defined in shared.wxs
    • Since the no-assert toolchain build output is not a complete layout, i.e. not all needed files are built twice, only ones that are different between the two toolchain variants are built. we need to pull filles from both output folders to make a complete layout.
  • Add a new UI option to select one or both of the two variants of the toolchain
  • UI makes it so at least one toolchain should be enabled at anytime. users can choose to add both
    • Path env var is set to Asserts if it is selected. if users select only no-asserts variant, then the PATH will be set to this one.

Still Open

  • UI for the installer. This changes extends the UI by 40px to add new options; this should be fixed in a separate PR before this change lands

@@ -103,6 +130,10 @@
<Property Id="TOOLCHAINSVERSIONED">
<RegistrySearch Root="HKCU" Key="SOFTWARE\!(loc.ManufacturerName)\Installer\$(ProductVersion)\!(bind.Property.UpgradeCode)" Name="ToolchainsVersioned" Type="directory" />
Copy link
Member

Choose a reason for hiding this comment

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

We should name this to include the variant.

Copy link
Author

Choose a reason for hiding this comment

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

that would break the upgrade scenario.

@@ -86,6 +108,11 @@
<RegistryValue Root="HKCU" Key="SOFTWARE\!(loc.ManufacturerName)\Installer\$(ProductVersion)\!(bind.Property.UpgradeCode)" Name="ToolchainsVersioned" Value="[ToolchainsVersioned]" />
<util:RemoveFolderEx Property="TOOLCHAINSVERSIONED" Condition="TOOLCHAINSVERSIONED" />
</Component>

<Component Id="VersionedDirectoryCleanupToolchains_noasserts">
Copy link
Member

Choose a reason for hiding this comment

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

Can this not be merged into the singular VersionedDirectoryCleanupToolchains? We should be cleaning up %ROOT%\Toolchains.

Copy link
Author

Choose a reason for hiding this comment

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

no. they are two different directories: <INSTALLROOT>\Toolchains\0.0.0+Asserts and <INSTALLROOT>\Toolchains\0.0.0+NoAsserts

Copy link
Member

Choose a reason for hiding this comment

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

My understanding was that it was meant to clean up any orphaned directories in <INSTALLROOT>\Toolchains. Is that not the case?

Copy link
Author

Choose a reason for hiding this comment

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

there are two folders. Toolchain, and ToolchainVersioned:

    <DirectoryRef Id="INSTALLROOT">
        <Directory Id="Toolchains" Name="Toolchains">
          <Directory Id="ToolchainsVersioned" Name="$(ProductVersion)+Asserts">

So for every top level folder we create we need to remember to remove it. so this is why we have similar entries for VersionedDirectoryCleanupRedistVersion, VersionedDirectoryCleanupRuntimes and VersionedDirectoryCleanupRedistVersion

</Directory>
</Directory>
</DirectoryRef>
</Fragment>
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand why the _noasserts suffix is needed at all. The layout is going to be identical, its just that the content source is different. The only piece that changes is the ToolchainsVersioned directory, which can include the suffix. If we do this, we can actually share the definition across the variants no?

Copy link
Author

Choose a reason for hiding this comment

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

they are diffrent layouts. they are rooted at diffrent directores. one at 0.0.0+Asserts and one at 0.0.0+NoAsserts. a directory entry in an msi is just a name and parent. since the parents are diffrent, we need a new entry

Copy link
Member

Choose a reason for hiding this comment

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

I see; lets make that a prefix I think.

Copy link
Author

Choose a reason for hiding this comment

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

why? all the other names it is a suffix. e.g. dbg.noasserts. why would component names and folder names be different?

@mhegazy mhegazy requested a review from compnerd May 21, 2025 01:17
<PropertyGroup>
<DefineConstants>
$(DefineConstants);
_USR_LIB_CLANG_NOASSERTS=$(ImageRoot)\Toolchains\$(ProductVersion)+Asserts\usr\lib\clang;
Copy link
Member

Choose a reason for hiding this comment

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

Can we make the variant a prefix instead of a suffix?

</Directory>
</Directory>
</DirectoryRef>
</Fragment>
Copy link
Member

Choose a reason for hiding this comment

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

I see; lets make that a prefix I think.

@@ -86,6 +108,11 @@
<RegistryValue Root="HKCU" Key="SOFTWARE\!(loc.ManufacturerName)\Installer\$(ProductVersion)\!(bind.Property.UpgradeCode)" Name="ToolchainsVersioned" Value="[ToolchainsVersioned]" />
<util:RemoveFolderEx Property="TOOLCHAINSVERSIONED" Condition="TOOLCHAINSVERSIONED" />
</Component>

<Component Id="VersionedDirectoryCleanupToolchains_noasserts">
Copy link
Member

Choose a reason for hiding this comment

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

My understanding was that it was meant to clean up any orphaned directories in <INSTALLROOT>\Toolchains. Is that not the case?

@mhegazy mhegazy requested a review from compnerd May 21, 2025 04:35
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.

2 participants