Skip to content

Build break: Shorten Windows targeting pack CAB name #29605

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 25, 2021
Merged

Build break: Shorten Windows targeting pack CAB name #29605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions eng/targets/Wix.Common.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,19 +27,24 @@
<OutputName Condition="'$(OutputNamePrefix)' != '' AND '$(OutputNameSuffix)' != ''">$(OutputNamePrefix)$(_GeneratedPackageVersion)$(OutputNameSuffix)</OutputName>

<EmbedCab Condition="'$(EmbedCab)' == ''">yes</EmbedCab>
<Cabinet Condition="'$(Cabinet)' == ''">$(OutputName.Replace('-', '_')).cab</Cabinet>
<InstallDir>$(ProductName)</InstallDir>

<DefineConstants Condition="'$(Configuration)' == 'Debug'">$(DefineConstants);Debug</DefineConstants>
<DefineConstants>$(DefineConstants);EmbedCab=$(EmbedCab)</DefineConstants>
<DefineConstants>$(DefineConstants);Cabinet=$(Cabinet)</DefineConstants>
</PropertyGroup>

<PropertyGroup Condition=" '$(OutputType)' == 'package' AND '$(Cabinet)' == '' ">
<Cabinet>$(OutputName.Replace('-', '_')).cab</Cabinet>
<Cabinet Condition=" '$(EmbedCab)' != 'yes' ">$(OutputName.Replace('_win', '')).cab</Cabinet>
Copy link
Contributor Author

@dougbu dougbu Jan 25, 2021

Choose a reason for hiding this comment

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

This is just defence-in-depth. Nothing should depend in the CAB name whether it's embedded or not.

Copy link
Contributor Author

@dougbu dougbu Jan 25, 2021

Choose a reason for hiding this comment

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

Side note: This will affect the runtime pack as well. I don't see a problem there but want to call it out.

Copy link
Member

@joeloff joeloff Jan 25, 2021

Choose a reason for hiding this comment

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

should be fine until the cab is made external again, mostly because of the shared wixlib we produce to share the MSI between multiple installers

</PropertyGroup>

<PropertyGroup>
<Culture Condition=" '$(Culture)' == '' ">en-US</Culture>
<Cultures Condition=" '$(Cultures)' == '' ">$(Culture)</Cultures>
<InstallerPlatform>$(Platform)</InstallerPlatform>
<OutDir Condition=" '$(OutDir)' == '' ">$(OutputPath)</OutDir>

<DefineConstants>$(DefineConstants);Cabinet=$(Cabinet)</DefineConstants>
<DefineConstants>$(DefineConstants);BinPath=$(OutputPath)$(Culture)\</DefineConstants>
<DefineConstants>$(WixVariables);$(DefineConstants)</DefineConstants>
</PropertyGroup>
Expand Down
8 changes: 7 additions & 1 deletion src/Installers/Windows/TargetingPack/Product.wxs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,13 @@
<Package InstallerVersion="$(var.InstallerVersion)" Compressed="yes" InstallScope="perMachine" />

<MajorUpgrade DowngradeErrorMessage="A newer version of [ProductName] is already installed." Schedule="afterInstallFinalize" />
<Media Id="1" Cabinet="$(var.Cabinet)" CompressionLevel="high" EmbedCab="$(var.EmbedCab)" />

<?if $(var.EmbedCab)=yes?>
<!-- Ignore var.Cabinet This element should choose an appropriate name for the embedded CAB. -->
<MediaTemplate CompressionLevel="high" EmbedCab="yes" />
Copy link
Contributor Author

@dougbu dougbu Jan 25, 2021

Choose a reason for hiding this comment

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

Is an id attribute needed in this element❔

Copy link
Member

@joeloff joeloff Jan 25, 2021

Choose a reason for hiding this comment

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

No, the MediaTemplate is just a convenience element in the toolset that defers some of the mundane tasks to the compiler/linker when generating the table.

Copy link
Member

@joeloff joeloff Jan 25, 2021

Choose a reason for hiding this comment

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

Do you think ASP.NET would move back to external cabinets? If not, you can likely just use the MediaTemplate. The downside is that with external cabinets and the template and multiple MSIs is that they can't exist in the same folder (becuase the cabs are just numerically named and will clash).

Copy link
Contributor Author

@dougbu dougbu Jan 25, 2021

Choose a reason for hiding this comment

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

I doubt it but prefer to leave the option available without needing additional work to avoid those clashes.

<?else?>
<Media Id="1" Cabinet="$(var.Cabinet)" CompressionLevel="high" EmbedCab="no" />
<?endif?>

<WixVariable Id="WixUILicenseRtf" Value="$(var.files)\eula.rtf" />
<UIRef Id="WixUI_Minimal" />
Expand Down