Skip to content

Prevent preview packages to ship as RC #6195

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 2 commits into from
Oct 9, 2024
Merged

Prevent preview packages to ship as RC #6195

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e058b55
Prevent preview packages to ship as RC
joperezr Oct 8, 2024
67d7b83
PR Feedback
joperezr Oct 8, 2024
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
3 changes: 2 additions & 1 deletion eng/Versions.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
<MinorVersion>0</MinorVersion>
<PatchVersion>0</PatchVersion>
<VersionPrefix>$(MajorVersion).$(MinorVersion).$(PatchVersion)</VersionPrefix>
<PreReleaseVersionLabel>rc.1</PreReleaseVersionLabel>
<PreReleaseVersionLabel Condition="'$(SuppressFinalPackageVersion)' != 'true'">rc.1</PreReleaseVersionLabel>
<PreReleaseVersionLabel Condition="'$(PreReleaseVersionLabel)' == ''">preview.4</PreReleaseVersionLabel>
Copy link
Member

@eerhardt eerhardt Oct 8, 2024

Choose a reason for hiding this comment

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

Can we instead do this in a .targets so we don't have to split properties out of the .csprojs?

Copy link
Member Author

@joperezr joperezr Oct 8, 2024

Choose a reason for hiding this comment

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

Unfortunately no, this needs to happen in a props file as it's values are read in the Common props from the SDK, so setting it later would be too late. I am adding a test like @radical suggested, so at least we'll know if this is broken in a project because they aren't using a d.b.props file.

Copy link
Member

@eerhardt eerhardt Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Have 2 properties here - named whatever.

And then in the Directory.Build.targets set the PreReleaseVersionLabel to one of those properties based on SuppressFinalPackageVersion.

This is better ordering in MSBuild IMO

Copy link
Member

@eerhardt eerhardt Oct 8, 2024

Choose a reason for hiding this comment

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

This is how arcade consumes this property - in a .targets.

https://github.com/dotnet/arcade/blob/382c993e9ab742d2292c9330e1b952c5b57aaa2c/src/Microsoft.DotNet.Arcade.Sdk/tools/Version.BeforeCommonTargets.targets#L131-L137

Copy link
Member Author

@joperezr joperezr Oct 8, 2024

Choose a reason for hiding this comment

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

That is technically possible, but I would be worried about the potential impact of those changes, especially when making these in a release branch as we don't know what changing the ordering of those properties could do. What would you say about doing this less risky change here, and then in main (which will have more bake time and validation) do the change you suggest?

Copy link
Member

@eerhardt eerhardt Oct 8, 2024

Choose a reason for hiding this comment

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

this needs to happen in a props file as it's values are read in the Common props from the SDK, so setting it later would be too late

I don't think this is true. Doing a dotnet build /bl and looking at the preprocess, the only places I see that use this property are the .csproj setting it, and the arcade line above.

but I would be worried about the potential impact of those changes

If you are really concerned about the risk, we can clutter the 8 projects here like we are doing. But I don't think this is ideal. If you want to do it the correct way in main, I'm fine with that.

Copy link
Member Author

@joperezr joperezr Oct 9, 2024

Choose a reason for hiding this comment

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

Yeah, at this point my top priority is to de-risk RC1, so unless you feel strongly about it, I'd opt for keeping this as-is and then getting this refactored in main

<!--
When running package validation as part of the build, we want to ensure we didn't break the API against the previous
version of each package. The following property points to the package version that should be used as baseline.
Expand Down
2 changes: 0 additions & 2 deletions src/Aspire.Hosting.Azure.WebPubSub/Aspire.Hosting.Azure.WebPubSub.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
<PackageTags>aspire integration hosting azure webpubsub pubsub websocket</PackageTags>
<Description>Azure WebPubSub resource types for .NET Aspire.</Description>
<PackageIconFullPath>$(SharedDir)AzureWebPubSub_256x.png</PackageIconFullPath>
<!-- This library can't ship stable until Azure.Provisioning.WebPubSub ships stable. -->
<SuppressFinalPackageVersion>true</SuppressFinalPackageVersion>
<!-- Disable package validation as this package hasn't shipped yet. -->
<EnablePackageValidation>false</EnablePackageValidation>
</PropertyGroup>
Expand Down
9 changes: 9 additions & 0 deletions src/Aspire.Hosting.Azure.WebPubSub/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<Project>
<PropertyGroup>
<!-- This needs to be set to true before importing parent Directory.Build.props -->
<!-- This library can't ship stable until Azure.Provisioning.WebPubSub ships stable. -->
<SuppressFinalPackageVersion>true</SuppressFinalPackageVersion>
</PropertyGroup>

<Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props', '$(MSBuildThisFileDirectory)../'))" />
</Project>
1 change: 0 additions & 1 deletion src/Aspire.Hosting.Elasticsearch/Aspire.Hosting.Elasticsearch.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
<PropertyGroup>
<TargetFramework>$(DefaultTargetFramework)</TargetFramework>
<IsPackable>true</IsPackable>
<SuppressFinalPackageVersion>true</SuppressFinalPackageVersion>
<PackageTags>aspire integration hosting elasticsearch</PackageTags>
<Description>Elasticsearch support for .NET Aspire.</Description>
<PackageIconFullPath>$(SharedDir)Elastic_logo.png</PackageIconFullPath>
Expand Down
8 changes: 8 additions & 0 deletions src/Aspire.Hosting.Elasticsearch/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<Project>
<PropertyGroup>
<!-- This needs to be set to true before importing parent Directory.Build.props -->
<SuppressFinalPackageVersion>true</SuppressFinalPackageVersion>
</PropertyGroup>

<Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props', '$(MSBuildThisFileDirectory)../'))" />
</Project>
1 change: 0 additions & 1 deletion src/Aspire.Hosting.Keycloak/Aspire.Hosting.Keycloak.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
<PropertyGroup>
<TargetFramework>$(DefaultTargetFramework)</TargetFramework>
<IsPackable>true</IsPackable>
<SuppressFinalPackageVersion>true</SuppressFinalPackageVersion>
<PackageTags>aspire integration hosting keycloak</PackageTags>
<Description>Keycloak support for .NET Aspire.</Description>
</PropertyGroup>
Expand Down
8 changes: 8 additions & 0 deletions src/Aspire.Hosting.Keycloak/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<Project>
<PropertyGroup>
<!-- This needs to be set to true before importing parent Directory.Build.props -->
<SuppressFinalPackageVersion>true</SuppressFinalPackageVersion>
</PropertyGroup>

<Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props', '$(MSBuildThisFileDirectory)../'))" />
</Project>
2 changes: 0 additions & 2 deletions src/Components/Aspire.Azure.AI.OpenAI/Aspire.Azure.AI.OpenAI.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
<PropertyGroup>
<TargetFramework>$(DefaultTargetFramework)</TargetFramework>
<IsPackable>true</IsPackable>
<!-- This library can't ship stable until Azure.AI.OpenAI ships stable. -->
<SuppressFinalPackageVersion>true</SuppressFinalPackageVersion>
<PackageTags>$(ComponentAzurePackageTags) ai openai</PackageTags>
<Description>A client for Azure OpenAI that integrates with Aspire, including logging and telemetry.</Description>
<PackageIconFullPath>$(SharedDir)AzureOpenAI_256x.png</PackageIconFullPath>
Expand Down
9 changes: 9 additions & 0 deletions src/Components/Aspire.Azure.AI.OpenAI/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<Project>
<PropertyGroup>
<!-- This needs to be set to true before importing parent Directory.Build.props -->
<!-- This library can't ship stable until Azure.AI.OpenAI ships stable. -->
<SuppressFinalPackageVersion>true</SuppressFinalPackageVersion>
</PropertyGroup>

<Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props', '$(MSBuildThisFileDirectory)../'))" />
</Project>
1 change: 0 additions & 1 deletion ...mponents/Aspire.Elastic.Clients.Elasticsearch/Aspire.Elastic.Clients.Elasticsearch.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
<PropertyGroup>
<TargetFramework>$(DefaultTargetFramework)</TargetFramework>
<IsPackable>true</IsPackable>
<SuppressFinalPackageVersion>true</SuppressFinalPackageVersion>
<PackageTags>$(ComponentCommonPackageTags) Elasticsearch</PackageTags>
<Description>A Elasticsearch client that integrates with Aspire, including health checks, logging, and telemetry.</Description>
<PackageIconFullPath>$(SharedDir)Elastic_logo.png</PackageIconFullPath>
Expand Down
8 changes: 8 additions & 0 deletions src/Components/Aspire.Elastic.Clients.Elasticsearch/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<Project>
<PropertyGroup>
<!-- This needs to be set to true before importing parent Directory.Build.props -->
<SuppressFinalPackageVersion>true</SuppressFinalPackageVersion>
</PropertyGroup>

<Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props', '$(MSBuildThisFileDirectory)../'))" />
</Project>
1 change: 0 additions & 1 deletion src/Components/Aspire.Keycloak.Authentication/Aspire.Keycloak.Authentication.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
<PropertyGroup>
<TargetFramework>$(DefaultTargetFramework)</TargetFramework>
<IsPackable>true</IsPackable>
<SuppressFinalPackageVersion>true</SuppressFinalPackageVersion>
<PackageTags>$(ComponentCommonPackageTags) Keycloak</PackageTags>
<Description>Configures Keycloak as the authentication provider for ASP.NET Core applications.</Description>
<!-- Disable package validation as this package hasn't shipped yet. -->
Expand Down
8 changes: 8 additions & 0 deletions src/Components/Aspire.Keycloak.Authentication/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<Project>
<PropertyGroup>
<!-- This needs to be set to true before importing parent Directory.Build.props -->
<SuppressFinalPackageVersion>true</SuppressFinalPackageVersion>
</PropertyGroup>

<Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props', '$(MSBuildThisFileDirectory)../'))" />
</Project>
1 change: 0 additions & 1 deletion src/Components/Aspire.Milvus.Client/Aspire.Milvus.Client.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
<PackageTags>$(ComponentCommonPackageTags) milvus database vector search</PackageTags>
<Description>A Milvus client that integrates with Aspire, including logging.</Description>
<PackageIconFullPath>$(SharedDir)Milvus_256x.png</PackageIconFullPath>
<SuppressFinalPackageVersion>true</SuppressFinalPackageVersion>
<NoWarn>$(NoWarn);CS8002</NoWarn> <!-- Milvus.Client packages are not signed -->
<!-- Disable package validation as this package hasn't shipped yet. -->
<EnablePackageValidation>false</EnablePackageValidation>
Expand Down
8 changes: 8 additions & 0 deletions src/Components/Aspire.Milvus.Client/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<Project>
<PropertyGroup>
<!-- This needs to be set to true before importing parent Directory.Build.props -->
<SuppressFinalPackageVersion>true</SuppressFinalPackageVersion>
</PropertyGroup>

<Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props', '$(MSBuildThisFileDirectory)../'))" />
</Project>
2 changes: 0 additions & 2 deletions src/Components/Aspire.OpenAI/Aspire.OpenAI.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
<PropertyGroup>
<TargetFramework>$(DefaultTargetFramework)</TargetFramework>
<IsPackable>true</IsPackable>
<!-- This library can't ship stable until OpenAI ships stable. -->
<SuppressFinalPackageVersion>true</SuppressFinalPackageVersion>
<PackageTags>$(ComponentCommonPackageTags) ai openai</PackageTags>
<Description>A client for OpenAI that integrates with Aspire, including metrics and telemetry.</Description>
<NoWarn>$(NoWarn);SYSLIB1100;SYSLIB1101</NoWarn>
Expand Down
9 changes: 9 additions & 0 deletions src/Components/Aspire.OpenAI/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<Project>
<PropertyGroup>
<!-- This needs to be set to true before importing parent Directory.Build.props -->
<!-- This library can't ship stable until OpenAI ships stable. -->
<SuppressFinalPackageVersion>true</SuppressFinalPackageVersion>
</PropertyGroup>

<Import Project="$([MSBuild]::GetPathOfFileAbove('Directory.Build.props', '$(MSBuildThisFileDirectory)../'))" />
</Project>
6 changes: 6 additions & 0 deletions src/Components/Directory.Build.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@
</ItemGroup>
</Target>

<Target Name="ValidatePreReleaseLabelIsCorrectForPreviewPackages"
AfterTargets="Pack"
Condition="'$(OfficialBuildId)' != '' and '$(SuppressFinalPackageVersion)' == 'true' and !$(PreReleaseVersionLabel.StartsWith('preview'))">
Copy link
Member

@eerhardt eerhardt Oct 8, 2024

Choose a reason for hiding this comment

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

Would it be better to actually look at the outputted nupkg and verify its version?

Copy link
Member Author

@joperezr joperezr Oct 9, 2024

Choose a reason for hiding this comment

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

This would be possible I think, yes, via a Regex that extracts the label. Unless you feel strongly about it, I can make those changes in main along with your refactoring suggestion in favor of getting rc1 build ready.

<Error Text="The PreReleaseLabel for package '$(MSBuildProjectName)' must start with 'preview' as it has the SuppressFinalPackageVersion flag set. Current label is '$(PreReleaseVersionLabel)'" />
</Target>

<!--
Logic for generating and comparing the ConfigurationSchema.json file
-->
Expand Down