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.

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

Jump to bottom

Fix load order of ProjectStaging and restrict package version validation to only official builds #6041

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Fix load order of ProjectStaging and restrict package version validation to only official builds #6041

wants to merge 9 commits into from

Conversation

RussKie
Copy link
Member

@RussKie RussKie commented Mar 5, 2025 •
edited
Loading

Reinstate #5864.

@RussKie RussKie requested a review from a team as a code owner March 5, 2025 22:48
@RussKie RussKie changed the title Fix load order of ProjectStaging and restrict package version validat… Fix load order of ProjectStaging and restrict package version validation to only official builds Mar 5, 2025
@RussKie RussKie requested a review from joperezr March 5, 2025 22:48
@RussKie
Copy link
Member Author

RussKie commented Mar 5, 2025

Tested in in the internal CI leg as a normal and a release branches.

@RussKie RussKie requested review from a team as code owners March 11, 2025 23:42
@RussKie
Copy link
Member Author

RussKie commented Mar 11, 2025

Tested in https://dev.azure.com/dnceng/internal/_build/results?buildId=2660769 and compared artifacts to the latest release (9.3).

@RussKie RussKie force-pushed the fix_validation branch 3 times, most recently from a37fef3 to 0804d78 Compare March 12, 2025 23:02
joperezr
joperezr reviewed Mar 12, 2025
View reviewed changes
Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

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

Other than the above comments, the rest of the changes look good to me. I'm assuming you've ran a stable build with this and validated that the right packages are still marked as prerelease?

@RussKie
Copy link
Member Author

RussKie commented Mar 12, 2025

Other than the above comments, the rest of the changes look good to me. I'm assuming you've ran a stable build with this and validated that the right packages are still marked as prerelease?

I'm running tests now.

@RussKie RussKie mentioned this pull request Mar 13, 2025
Merged
@RussKie RussKie requested a review from a team as a code owner March 18, 2025 00:58
RussKie added 7 commits March 18, 2025 12:16
@RussKie
Add "validation" branch which mirrors "release" but can be deleted
19614ae
@RussKie
Revert "Merged PR 48132: Flowing dependencies and getting ready for 9…
d9ab6c1 
….3 release"

This reverts commit 092d941.
@RussKie
fixup! Fix load order of ProjectStaging and restrict package version …
682aed4 
…validation to only official builds
@RussKie
fixup! Fix load order of ProjectStaging and restrict package version …
4de3c68 
…validation to only official builds
@RussKie
fixup! Fix load order of ProjectStaging and restrict package version …
25ef112 
…validation to only official builds
@RussKie
fixup! Fix load order of ProjectStaging and restrict package version …
6f470af 
…validation to only official builds
@RussKie
Revert "fixup! Fix load order of ProjectStaging and restrict package …
5c0f5c2 
…version validation to only official builds"

This reverts commit d953d0a.
RussKie
RussKie commented Mar 18, 2025
View reviewed changes
eng/Versions.props
-->
<DotNetFinalVersionKind />
<StabilizePackageVersion Condition="'$(StabilizePackageVersion)' == ''">false</StabilizePackageVersion>
Copy link
Member Author

Choose a reason for hiding this comment

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

@joperezr this is what we'll set to true in the release branch, and this flag controls whether DotNetFinalVersionKind=true.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, thanks!

joperezr
joperezr reviewed Mar 26, 2025
View reviewed changes
eng/MSBuild/ProjectStaging.targets
Note, that Arcade resets $(_PreReleaseLabel) to "ci" for non-official builds, which breaks the validation.
See Arcade SDK/Version.BeforeCommonTargets.targets for more details.
-->
<Target Name="_ValidateVersion" AfterTargets="GenerateNuspec" Condition=" '$(OfficialBuild)' == 'true' and '$(_IsStable)' != 'true' ">
Copy link
Member

Choose a reason for hiding this comment

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

Where is OfficialBuild set? And what is the difference between that and OfficialBuildId which is the common one that gets set?

joperezr
joperezr reviewed Mar 26, 2025
View reviewed changes
Directory.Build.props
@@ -105,6 +105,19 @@
<!-- Enable the Evaluation report builds on all CI builds -->
<PropertyGroup>
<EnableEvaluationReportBuild Condition="'$(ContinuousIntegrationBuild)'=='true'">true</EnableEvaluationReportBuild>

<!--
ProjectStaging.props depends on the $(Stage) property, which is defined in the project file.
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the detailed comment 😃!

joperezr
joperezr reviewed Mar 26, 2025
View reviewed changes
azure-pipelines.yml
@@ -31,6 +31,7 @@ pr:
- main
- dev
- release/*
- validation/*
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want this to run for PRs against validation too? Or simply with manually queued builds?

joperezr
joperezr approved these changes Mar 26, 2025
View reviewed changes
Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

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

Left a couple of super minor comments, but this looks good to go otherwise. Thanks a lot for the hard work on this and on getting it back and enabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evgenyfedorov2 evgenyfedorov2 evgenyfedorov2 approved these changes

@joperezr joperezr joperezr approved these changes

At least 1 approving review is required to merge this pull request.

Assignees

@RussKie RussKie

Labels
area-infrastructure
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RussKie @joperezr @evgenyfedorov2