Skip to content

[release/3.1] Give .msi's non-stable branding #21801

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

Merged
merged 3 commits into from
May 14, 2020

Conversation

wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented May 13, 2020

Customers have reported issues with our .MSI's when repairing VS. We believe the issue is due to the ProductCode/ProductVersion of the installers not being unique per build, but instead being unique per package version. Giving the MSI's suffixed versions should fix the issue

CC @Pilchie @joeloff

@wtgodbe wtgodbe requested a review from a team May 13, 2020 22:11
@ghost ghost added the feature-installers Includes: Installers label May 13, 2020
@joeloff
Copy link
Member

joeloff commented May 13, 2020

The inputs for the ProductCode haven't changed I think. The version constructed for the MSI itself is coming from

<Version>$(AspNetCoreMajorVersion).$(AspNetCoreMinorVersion).$(AspNetCorePatchVersion).0</Version>

And then combined with a product identifier to eventually generate the GUID:

<GenerateGuid NamespaceGuid="$(NamespaceGuid)" Values="$(ProductNameShort);$(GuidInputs)">

@wtgodbe
Copy link
Member Author

wtgodbe commented May 13, 2020

@joeloff thanks for pointing that out, I pushed eeba5fd to append the build number. $(_BuildNumberLabels) is something like .20263.6

@Pilchie
Copy link
Member

Pilchie commented May 13, 2020

It sounds like we also need this for 2.1? Is that right @joeloff ?

@joeloff
Copy link
Member

joeloff commented May 13, 2020

@Pilchie Yes, the issue has come up for 2.1 as well.

@@ -2,7 +2,7 @@
<!-- Set versioning properties after Arcade SDK targets have been imported. -->
<PropertyGroup>
<!-- Used for generating stable upgrade codes for bundles -->
<Version>$(AspNetCoreMajorVersion).$(AspNetCoreMinorVersion).$(AspNetCorePatchVersion).0</Version>
<Version>$(AspNetCoreMajorVersion).$(AspNetCoreMinorVersion).$(AspNetCorePatchVersion)$(_BuildNumberLabels)</Version>
Copy link
Member

Choose a reason for hiding this comment

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

@wtgodbe is BuildNumberLabels a 2 part version? If so, this will turn Version into a 5 part number. That's fine for generating the GUID, however, the version property will be validated when the MSI is compiled and WiX may reject it I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is. I could replace it with FileRevisionVersion which is just the 20263 part. In that case though it wouldn't match the PackageVersion, is that alright? PackageVersion already contains BuildNumberLabels in non-stable builds

Copy link
Member

@joeloff joeloff May 13, 2020

Choose a reason for hiding this comment

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

Another option might be to just leave the Version as is, but include $(_BuildNumberLabels) in the GuidInputs property so we end up with

<GuidInputs>$(Version);$(Platform);$(VersionSuffix);$(_BuildNumberLabels)</GuidInputs>

@wtgodbe wtgodbe changed the title Give .msi's non-stable branding [release/3.1] Give .msi's non-stable branding May 13, 2020
<_GeneratedPackageVersion>$(PackageVersion)</_GeneratedPackageVersion>
<_GeneratedPackageVersion
Condition="! $(PackageVersion.Contains('$(_PreReleaseLabel)'))">$(PackageVersion)-$(_PreReleaseLabel)$(_BuildNumberLabels)</_GeneratedPackageVersion>
<PackageFileName>$(RuntimeInstallerBaseName)-$(_GeneratedPackageVersion)-win-$(Platform)$(TargetExt)</PackageFileName>
Copy link
Contributor

Choose a reason for hiding this comment

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

Will changing the package filename confuse the dotnet/installer repo?

Copy link
Member Author

Choose a reason for hiding this comment

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

Very possibly - I'll look into that now & see if I need to change anything there

@@ -21,7 +21,7 @@
</PropertyGroup>

<PropertyGroup>
<GuidInputs>$(Version);$(Platform);$(VersionSuffix)</GuidInputs>
<GuidInputs>$(Version);$(Platform);$(VersionSuffix);$(_BuildNumberLabels)</GuidInputs>
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice if this were a public Arcade property. Ah well.

@leecow leecow added the Servicing-approved Shiproom has approved the issue label May 14, 2020
@leecow leecow added this to the 3.1.5 milestone May 14, 2020
@wtgodbe
Copy link
Member Author

wtgodbe commented May 14, 2020

@joeloff was this an issue with the aspnetcore-targeting-pack-x.y.z.msi (as opposed to aspnetcore-runtime-x.y.z.msi)? If so, that would mean we'd need to rebuild the targeting pack again... Otherwise that complicates the ingestion of this in dotnet/installer, because if we change the version of the targeting pack installer, we'd have to find a way to pin that non-stable version in dotnet/installer's dependencies for subsequent releases

@joeloff
Copy link
Member

joeloff commented May 14, 2020

Highly likely. From what I can tell, both the targeting pack and runtime installer followed the same scheme. However, the two installers are grouped together as a single unit in VS. When the runtime fails, the targeting pack won't even be executed as VS suspends the rest of the execution plan after the first failure.

@Pilchie
Copy link
Member

Pilchie commented May 14, 2020

But - similar to the 2.1 PR, we don't have to ship the targeting pack NOW, right? We could just merge the fix so that it's there the next time we need to ship the targeting pack?

@wtgodbe
Copy link
Member Author

wtgodbe commented May 14, 2020

@Pilchie we could do that, but we should still solve "how will dotnet/installer pin the version of the targeting pack .msi?" now, so we don't forget about it if/when we do eventually ship the targeting pack again. I can figure that out as part of my reaction PR in dotnet/installer

@wtgodbe
Copy link
Member Author

wtgodbe commented May 14, 2020

Short of hard-coding a version in dotnet/installer, I think we'd have to start publishing a "microsoft.aspnetcore.app.ref.internal", similar to "microsoft.netcore.app.internal" (from dotnet/runtime), whose only purpose is to identify the non-stable ref pack version. It would only build when the ref pack is building. For now I think I'll hard code the current version in installer (3.1.2) as a property, and open an issue against myself to create the internal ref pack package for 3.1.6

@wtgodbe
Copy link
Member Author

wtgodbe commented May 14, 2020

#21850 - issue for internal ref pack package

dotnet/installer#7487, dotnet/installer#7489 - reaction PRs in dotnet/installer

@wtgodbe wtgodbe merged commit 817127d into release/3.1 May 14, 2020
@wtgodbe wtgodbe deleted the wtgodbe/NonStableMSIs branch May 14, 2020 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-installers Includes: Installers Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants