Consolidate external repo versions into Versions.props #286
Conversation
repo-projects/cssparser.proj
Outdated
| @@ -13,7 +13,7 @@ | |||
| <BuildCommandArgs>$(BuildCommandArgs) /p:Configuration=$(Configuration)</BuildCommandArgs> | |||
| <BuildCommandArgs>$(BuildCommandArgs) /p:DelaySign=$(DelaySign)</BuildCommandArgs> | |||
| <BuildCommandArgs>$(BuildCommandArgs) /p:PublicSign=$(PublicSign)</BuildCommandArgs> | |||
| <BuildCommandArgs>$(BuildCommandArgs) /p:VersionSuffix=20230414.1</BuildCommandArgs> | |||
| <BuildCommandArgs>$(BuildCommandArgs) /p:VersionSuffix=$(CssParserVersionSuffix)</BuildCommandArgs> | |||
There was a problem hiding this comment.
Does it make sense to consolidate a suffix too?
eng/Versions.props
Outdated
| @@ -8,4 +8,16 @@ | |||
| <PreReleaseVersionLabel>alpha</PreReleaseVersionLabel> | |||
| <PreReleaseVersionIteration>1</PreReleaseVersionIteration> | |||
| </PropertyGroup> | |||
|
|
|||
| <PropertyGroup> | |||
| <AzureActiveDirectoryIdentityModelExtensionsVersion>7.1.2</AzureActiveDirectoryIdentityModelExtensionsVersion> | |||
There was a problem hiding this comment.
The original issue that prompted the logging of dotnet/source-build#3897 was https://github.com/dotnet/source-build-externals/pull/248/files/e22d3d81f27c6b8389d97c80819b86b296ed1df4. When I made that comment, the intent was to encapsulate the reference package versions in a single location to help ensure the repo's only referenced a single version and to make maintenance easier. This resulting issue pivoted and suggested encapsulating the external package versions as you did.
Using a similar/same naming convention as the reference packages introduces a potential issue. If the names match the generated artifact package version property names then anything defined here in the Versions.props file will get overridden during a product source-build with the N-1 version. This can therefore cause source-build to product an older external package version than intended.
Additionally names like DockerCredsProvider220PackageVersion are actually adding more maintenance in the upgrade scenario because not only does the value have to change, the property name does as well.
I see a few options:
- Abandon these changes
- Change the package naming convention to ensure a collisions with the N-1 packages never occur. e.g. introduce a new suffix - ExternalPackageVersion. We accept that some property names will need to change with each package upgrade.
With either approach, the versions of the package references owned by this repo should be encapsulated in the versions.props file e.g. https://github.com/dotnet/source-build-externals/blob/main/eng/tasks/Microsoft.DotNet.SourceBuild.Tasks.XPlat/Microsoft.DotNet.SourceBuild.Tasks.XPlat.csproj#L10
There was a problem hiding this comment.
Thank you for that context.
Which option would you prefer?
I tried implementing option 2, except I left the docker-creds-provider version hardcoded outside of eng/Versions.props.
There was a problem hiding this comment.
I personally have never experienced a need for these. From a conversation with @mthalman, he sees value in them. I have no objection to them as long as the aforementioned n-1 version issue is addressed.
omajid
force-pushed
the
consolidate-versions
branch
from
dbbd227 to
7808780
Compare
eng/Versions.props
Outdated
| @@ -8,4 +8,21 @@ | |||
| <PreReleaseVersionLabel>alpha</PreReleaseVersionLabel> | |||
| <PreReleaseVersionIteration>1</PreReleaseVersionIteration> | |||
| </PropertyGroup> | |||
|
|
|||
| <PropertyGroup> | |||
| <AzureActiveDirectoryIdentityModelExtensionsExternalPackageVersion>7.1.2</AzureActiveDirectoryIdentityModelExtensionsExternalPackageVersion> | |||
There was a problem hiding this comment.
An alternative suggestion on the name that is a bit shorter and decouples the version from just the packages - {repoName}ReleaseVersion
omajid
force-pushed
the
consolidate-versions
branch
from
7808780 to
c763a70
Compare
|
I don't have write access to this repo. If the PR looks good, please merge it for me. Thanks! |
Fixes: dotnet/source-build#3897