-
Notifications
You must be signed in to change notification settings - Fork 741
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
Have dev packages produce non-stable package versions. #4662
Conversation
FYI @Tratcher who originally reported this. |
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=458928&view=codecoverage-tab |
@@ -0,0 +1,9 @@ | |||
<Project> | |||
<!-- In order to get the right package versions for projects that shouldn't stabilize, we need to set this property before |
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.
This is unfortunate, it defines the stage property in an inconsistent place and makes it harder to use correctly. Can we do something else like have these projects override the DotNetFinalVersionKind property?
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.
Using Directory.Build.props on library-specific projects to work around things like this is not that uncommon, in fact dotnet/runtime use this all the time. One more issue is that the reason why this property (DotnetFinalVersionKind) needs to be defined so early is because it needs to be defined before the import of `Version.BeforeCommonProps.props, which gets imported before the project, meaning that even if we want to override that property in the .csprojs it would still be too late for some other things that are derived from it.
@@ -12,7 +12,7 @@ | |||
<!-- | |||
When DotNetFinalVersionKind is set to 'release', this branch will produce stable outputs for 'Shipping' packages | |||
--> | |||
<DotNetFinalVersionKind Condition="'$(DotNetFinalVersionKind)' == ''">release</DotNetFinalVersionKind> | |||
<DotNetFinalVersionKind Condition="'$(DotNetFinalVersionKind)' == '' And '$(Stage)' != 'dev'">release</DotNetFinalVersionKind> |
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.
What if we moved this line to the targets file? That is, leave the original line in this file, and then copy the new line to the targets file.
Then, we'd not have to create individual props files... 🤔
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.
The problem is not this file, this file needs to be here because there are some props files imported from Arcade infrastructure that depend on this being set, so if we move this to targets files, arcade props that depend on this won't work.
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.
It is possible that we could hook this up differently but I think that would be too risky to do this late in the release. I'm fine if we want to explore a different way of doing this for main branch, but for the release branch I want to have the least amount of moving pieces possible, and moving the way we set a property used by the whole rest of arcade is much more risk that what I think we should take.
Fixes #4649
Microsoft Reviewers: Open in CodeFlow