-
Notifications
You must be signed in to change notification settings - Fork 757
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
Generalise how package version isn't stabilized for dev/preview stages #5640
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for the help with this @RussKie! this is great. Given these properties could be impacted by race condition, can we also add a target that runs after GenerateNuspec that validates that version contains a prerelease label?
Also, I'm assuming you did, but in case not you should be able to validate if this works by running |
--> | ||
<SuppressFinalPackageVersion /> | ||
<SuppressFinalPackageVersion Condition="'$(Stage)' == 'dev' or '$(Stage)' == 'preview'">true</SuppressFinalPackageVersion> | ||
</PropertyGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version suffix calculations are performed in Version.BeforeCommonTargets.targets, which too late for the functionality placed in eng/MSBuild/ProjectStaging.targets
Arguably, we could add eng/MSBuild/ProjectStaging.props, but it feels more logical to have versioning-related configurations in one place (which is here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will Stage be defined this early though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see it will because you are creating a Directory.Build.props everywhere to make sure you control the order of imports. While that would work, I'd argue that we are probably defeating the point of the benefit of the change. The initial motivation was to simplify things, and reducing 2 lines in projects (setting the stage, and setting suppressFinalVersion) to a single one, but this is instead now making us having to add new files per project, and it also means that now you need to know to look into this file to see if a package is stable or not and also to know what is the stage.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably an even more meta question (sorry for having this discussion in your PR) is: do we even really need package stages? What benefit are we getting out of them? if it is literally just that the description has a few other words as a prefix, then perhaps we should just have stable and unstable packages, and adjust the descriptions of packages accordingly. That way, we can go back to having a single property in the package that marks it's "stableness"
Makes it such that the package version won't be stabilized even when the rest of the repo is going stable. | ||
https://github.com/dotnet/arcade/blob/main/Documentation/CorePackages/Versioning.md#package-version | ||
--> | ||
<SuppressFinalPackageVersion /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: No need to unset it here as that is the default already.
<SuppressFinalPackageVersion /> |
Follow up on #5632
Microsoft Reviewers: Open in CodeFlow