Include versions and checksums in VMR publishing and remove prepare-artifacts.proj #100004
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Tagging subscribers to this area: @dotnet/area-infrastructure-libraries |
ViktorHofer
force-pushed
the
ViktorHofer-patch-1
branch
2 times, most recently
from
d769d5e
to
3533944
Compare
ViktorHofer
force-pushed
the
ViktorHofer-patch-1
branch
from
3533944
to
3e78558
Compare
ViktorHofer
commented
Mar 25, 2024
| @@ -8,7 +8,7 @@ | |||
|
|
|||
| <PropertyGroup> | |||
| <IncrementalNativeBuild Condition="'$(IncrementalNativeBuild)' == ''">true</IncrementalNativeBuild> | |||
| <BuildCoreHostDependsOn>GetProductVersions;GenerateRuntimeVersionFile</BuildCoreHostDependsOn> | |||
| <BuildCoreHostDependsOn>GenerateRuntimeVersionFile</BuildCoreHostDependsOn> | |||
There was a problem hiding this comment.
I checked and I couldn't find a place under src/native that still depended on the GetProductVersions target.
ViktorHofer
commented
Mar 25, 2024
Comment on lines
-27
to
-31
| <Target Name="_SetProductVersion" DependsOnTargets="GetProductVersions" BeforeTargets="Pack"> | ||
| <PropertyGroup> | ||
| <PackageVersion>$(ProductVersion)</PackageVersion> | ||
| </PropertyGroup> | ||
| </Target> |
There was a problem hiding this comment.
From what I could see, this setting shouldn't be necessary as PackageVersion is already automatically set by Arcade.
ViktorHofer
commented
Mar 25, 2024
| <ItemGroup> | ||
| <_MonoRollupEnvironmentVariable Include="Configuration:$(Configuration)" /> | ||
| <_MonoRollupEnvironmentVariable Include="NativeBinDir:$(NativeBinDir)" /> | ||
| <_MonoRollupEnvironmentVariable Include="WasmObjDir:$(WasmObjDir)" /> | ||
| <_MonoRollupEnvironmentVariable Include="ProductVersion:$(ProductVersion)" /> | ||
| <_MonoRollupEnvironmentVariable Include="ProductVersion:$(RuntimePackProductVersion)" /> |
There was a problem hiding this comment.
Calling into sfxproj to retrieve the version felt safer than just relying on PackageVersion here in case this project sets IsShipping or any other properties already or in the future.
ViktorHofer
commented
Mar 25, 2024
| @@ -37,7 +37,6 @@ | |||
| <Target Name="SetupTestContextVariables" | |||
| Condition="'$(IsTestProject)' == 'true'" | |||
| DependsOnTargets=" | |||
| GetProductVersions; | |||
There was a problem hiding this comment.
I couldn't find any place that still depends on this target or its properties.
ViktorHofer
commented
Mar 25, 2024
Comment on lines
-19
to
-32
| <Target Name="SetTargetBasedPackageVersion" | ||
| BeforeTargets="GenerateNuSpec" | ||
| DependsOnTargets="GetProductVersions"> | ||
| <PropertyGroup> | ||
| <Version>$(ProductVersion)</Version> | ||
| <!-- | ||
| PackageVersion is normally calculated using Version during static property evaluation, but | ||
| we need some info from GetProductVersions, so it's too late to rely on that. We need to set | ||
| both in target evaluation, here. | ||
| --> | ||
| <PackageVersion>$(ProductVersion)</PackageVersion> | ||
| </PropertyGroup> | ||
| </Target> | ||
|
|
There was a problem hiding this comment.
This might have been necessary in the past but AFAIK, NuGet calculates PackageVersion correctly, even for pkgprojs.
This was referenced Mar 25, 2024
… into ViktorHofer-patch-1
ViktorHofer
added
the
NO-MERGE
ViktorHofer
changed the title
jkoritzinsky
reviewed
Apr 1, 2024
jkoritzinsky
reviewed
Apr 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes dotnet/source-build#4239
Follows the pattern that we established in windowsdesktop: https://github.com/dotnet/windowsdesktop/blob/main/eng/Publishing.props
Important: No Merge until I verified the changes in a runtime official build and a VMR build.
Runtime official build: https://dnceng.visualstudio.com/internal/_build/results?buildId=2414748&view=results