-
Notifications
You must be signed in to change notification settings - Fork 44
Update implicit metapackage version for 2.2 apps #348
Conversation
@vijayrkn @seancpeters Windows builds fail with this. Any ideas?
|
May be something changed in the build machines. Let me check. |
@natemcmaster - Triggered another build this morning and this seems to work fine. |
<DefaultPatchVersionForAspNetCoreAll2_1 Condition="'$(DefaultPatchVersionForAspNetCoreAll2_1)' == ''">2.1.1</DefaultPatchVersionForAspNetCoreAll2_1> | ||
<DefaultPatchVersionForAspNetCoreApp2_1 Condition="'$(DefaultPatchVersionForAspNetCoreApp2_1)' == ''">2.1.1</DefaultPatchVersionForAspNetCoreApp2_1> | ||
|
||
<LatestPatchVersionForAspNetCoreAll2_1 Condition="'$(LatestPatchVersionForAspNetCoreAll2_1)' == ''">2.1.*</LatestPatchVersionForAspNetCoreAll2_1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dsplaisted how do you plan to handle the latest patch version for NETCore.App 2.1.x in the 2.2 SDK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a hard-coded list of latest patch versions, and then tests which verify that those versions are correct so that we remember to keep them up-to-date. I'd recommend doing the same thing for the ASP.NET latest patch versions. dotnet/cli#9374 puts the default versions in the CLI, I would also add the latest versions and tests to make sure they're up-to-date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should do this. I've opened https://github.com/dotnet/cli/issues/9403 to follow-up on this.
<DefaultPatchVersionForAspNetCoreAll2_1 Condition="'$(DefaultPatchVersionForAspNetCoreAll2_1)' == ''">2.1.1</DefaultPatchVersionForAspNetCoreAll2_1> | ||
<DefaultPatchVersionForAspNetCoreApp2_1 Condition="'$(DefaultPatchVersionForAspNetCoreApp2_1)' == ''">2.1.1</DefaultPatchVersionForAspNetCoreApp2_1> | ||
|
||
<LatestPatchVersionForAspNetCoreAll2_1 Condition="'$(LatestPatchVersionForAspNetCoreAll2_1)' == ''">2.1.*</LatestPatchVersionForAspNetCoreAll2_1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a hard-coded list of latest patch versions, and then tests which verify that those versions are correct so that we remember to keep them up-to-date. I'd recommend doing the same thing for the ASP.NET latest patch versions. dotnet/cli#9374 puts the default versions in the CLI, I would also add the latest versions and tests to make sure they're up-to-date.
<PropertyGroup Condition="'$(DefaultPatchVersionForAspNetCoreAll2_2)' == ''"> | ||
<DefaultPatchVersionForAspNetCoreAll2_2>2.2.0</DefaultPatchVersionForAspNetCoreAll2_2> | ||
<!-- Check the bundled version. It will start with 2.2.0 until create the final version of the 2.2.0 runtimes.--> | ||
<DefaultPatchVersionForAspNetCoreAll2_2 Condition="$(BundledAspNetCoreAllPackageVersion.StartsWith('2.2.0'))">$(BundledAspNetCoreAllPackageVersion)</DefaultPatchVersionForAspNetCoreAll2_2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest using a dash here (and in the below condition for AspNetCoreApp), as I did for dotnet/cli#9374. It's not needed here for correctness since we won't release an "01" patch version, but ensures that if we update the number or copy-paste it somewhere else it's more likely to be correct.
This will already taken care of by dotnet/cli#9374 This reverts commit 9e595f7.
9b88011
to
10f9768
Compare
10f9768
to
81fa6e5
Compare
Ok, thanks for the feedback @dsplaisted. I've updated this PR. |
CI is passing now, thanks @vijayrkn. Are you okay with this change? |
Changes:
cc @dsplaisted @vijayrkn