Skip to content
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

[release/6.0] Add packaging changes for System.Text.Json

Merged
merged 4 commits into from Nov 17, 2021

Conversation

safern
Copy link
Member

@safern safern commented Nov 16, 2021

It seems like we missed packaging changes for JSON on: fd61aef

Also fixes: #61694

@msftbot
Copy link
Contributor

msftbot bot commented Nov 16, 2021

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

Issue Details

It seems like we missed packaging changes for JSON on: fd61aef

Author: safern
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

Anipik
Anipik approved these changes Nov 16, 2021
@safern
Copy link
Member Author

safern commented Nov 17, 2021

@Anipik @ericstj could you please review: 28b425b

By setting these properties on packaging.targets this was happening too late, after Version.BeforeCommonTargets.targets was imported which is where the Version property is set. So if we want to use arcade version calculation we need to import using the before common targets hooks. The reason why we were seeing package versions 6.0.1 was because the PatchVersion is set to 1.

Also, instead of setting VersionPrefix I had to instead set PatchVersion to ServicingVersion because arcade relies on that and overrides the VersionPrefix when these properties are set:
https://github.com/dotnet/arcade/blob/53cc1bc2e555aa7aea95884575d22e21d63708cf/src/Microsoft.DotNet.Arcade.Sdk/tools/Version.BeforeCommonTargets.targets#L76

We could avoid this complexity by just setting <Version>Major.Minor.ServicingVersion</Version> and then appending the VersionSuffix if not empty, basically copying this logic: https://github.com/dotnet/arcade/blob/53cc1bc2e555aa7aea95884575d22e21d63708cf/src/Microsoft.DotNet.Arcade.Sdk/tools/Version.BeforeCommonTargets.targets#L165

But it felt more right to not duplicate arcade logic on our side.

For .NET 7+ we could fix the VersionSuffix calculation in arcade and make it overridable, rather than always setting it here: https://github.com/dotnet/arcade/blob/53cc1bc2e555aa7aea95884575d22e21d63708cf/src/Microsoft.DotNet.Arcade.Sdk/tools/Version.BeforeCommonTargets.targets#L76

@safern
Copy link
Member Author

safern commented Nov 17, 2021

After giving it a thought, I decided to go the simple route which is setting Version on our side. The other change was too complicated for what we wanted to achieve and felt hacky.

@@ -267,9 +273,15 @@
</Target>

<Target Name="ValidateAssemblyVersionsInRefPack"
Condition="$(_AssemblyInTargetingPack) == 'true' and '$(PreReleaseVersionLabel)' == 'servicing'"
Condition="'$(SkipValidateAssemblyVersion)' != 'true' and '$(_AssemblyInTargetingPack)' == 'true' and '$(PreReleaseVersionLabel)' == 'servicing'"
AfterTargets="CoreCompile" >
<Error Condition="'$(AssemblyVersion)' != '$(LastReleasedStableAssemblyVersion)'" Text="AssemblyVersion should match last released assembly version $(LastReleasedStableAssemblyVersion)" />
Copy link
Member

@ericstj ericstj Nov 17, 2021

Choose a reason for hiding this comment

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

Is this all the validation we're doing @Anipik? I thought we were going to run a reverse check here:

<Exec Command="$(_ApiCompatCommand) &quot;$(PreviousNetCoreAppRefPath.TrimEnd('\/'))&quot; @&quot;$(ApiCompatResponseFile)&quot; $(_previousNetCoreAppBaselineParam)"
CustomErrorRegularExpression="^[a-zA-Z]+ :"
StandardOutputImportance="Low"
IgnoreExitCode="true">
<Output TaskParameter="ExitCode" PropertyName="ApiCompatExitCode" />
</Exec>

To ensure folks don't add API in servicing.

Copy link
Member Author

@safern safern Nov 17, 2021

Choose a reason for hiding this comment

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

We have an open issue, we haven't enabled it yet.

@ericstj
Copy link
Member

ericstj commented Nov 17, 2021

Don't treat my comment as blocking. Merge when ready.

@safern
Copy link
Member Author

safern commented Nov 17, 2021

Failure is: #50748

@safern safern merged commit e9036b0 into dotnet:release/6.0 Nov 17, 2021
168 of 173 checks passed
@safern safern deleted the AddPackagingJson60 branch Nov 17, 2021
@msftbot msftbot bot locked as resolved and limited conversation to collaborators Dec 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants