Skip to content

Conversation

@dougbu
Copy link
Contributor

@dougbu dougbu commented Jan 25, 2021

  • use <MediaTemplate> and auto-generated name when embedding
  • otherwise, stick with <Media> but remove _win from the CAB name

- use `<MediaTemplate>` and auto-generated name when embedding
- otherwise, stick with `<Media>` but remove `_win` from the CAB name
@dougbu
Copy link
Contributor Author

dougbu commented Jan 25, 2021

Hard to tell whether this is doing the right thing. @joeloff what's the easy way to see the name of the cabinet inside aspnetcore-targeting-pack-6.0.0-preview.1.21075.10-win-x64.msi❔ That was produced in an official build of this branch ➡️ https://dev.azure.com/dnceng/internal/_build/results?buildId=965053

@dougbu dougbu marked this pull request as ready for review January 25, 2021 21:50
@dougbu dougbu requested a review from a team as a code owner January 25, 2021 21:50
@dougbu dougbu requested a review from joeloff January 25, 2021 21:50

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

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

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

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


<?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

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

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

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

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.

@joeloff
Copy link
Member

joeloff commented Jan 25, 2021

@dougbu simplest is to open the MSI in Orca (it's part of the Win SDK), then look at the Media table's Cabinet column

image

Copy link
Member

@joeloff joeloff left a comment

Choose a reason for hiding this comment

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

Looks good, and the MSI contents looks good too. Getting rid of being able to support external cabs is just a suggestion. It won't impact anything.

@dougbu dougbu merged commit 2f61171 into main Jan 25, 2021
@dougbu dougbu deleted the dougbu/installer.build.break branch January 25, 2021 22:43
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.

3 participants