-
Notifications
You must be signed in to change notification settings - Fork 5k
Fix Microsoft.VisualBasic.Core file version #62848
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
Conversation
Tagging subscribers to this area: @cston Issue Detailscontributes to: #62218 This was regressed on: a505c46#diff-804ceb4a1df769aca14d1bd4043455cc1cce391376dcb02798cbf716cc6150eeL4
|
@@ -2,6 +2,7 @@ | |||
<Import Project="..\Directory.Build.props" /> | |||
<PropertyGroup> | |||
<AssemblyVersion>$([MSBuild]::Add($(MajorVersion), 5)).$(MinorVersion).0.0</AssemblyVersion> | |||
<MajorVersion>$([MSBuild]::Add($(MajorVersion), 6))</MajorVersion> |
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: Should we add a comment or something here to a) explain why we are changing these two so it doesn't get removed and b) say that the order of these two properties is important since if you switch them now MajorVersion used on assembly version will be wrong?
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.
Sure I'll add a comment.
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.
I think this should only be +5. AssemblyVersion and FileVersion should match if possible. We need to make sure the following:
Once this change is ported to 6.0 both AssemblyVersion and FileVersion are higher than 5.0, AssemblyVersion doesn't change from GA, and ideally Major versions of Assembly and File match.
7.0 has higher major version that 6.0.
Also, can we get away with just changing MajorVersion and then let other targets construct the correct Assembly and File versions?
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.
Ok, so for reference here are the versions we previously shipped
Release | File Version | Assembly Version |
---|---|---|
3.1 | 4.7.x.y | 10.0.5.0 |
5.0 | 11.0.x.y | 10.0.6.0 |
6.0 | 6.0.x.y | 11.0.0.0 |
7.0 | 7.0.x.y | 12.0.0.0 |
What about making 7.0 simpler (by having consistent file and assembly version), and just special casing 6.0 servicing?
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.
Also, can we get away with just changing MajorVersion and then let other targets construct the correct Assembly and File versions?
Unfortunately we can't as we set AssemblyVersion
on Versions.props
for all projects to Major.Minor.0.0
and at that point Major.Minor
have whatever values are set on Versions.props
in the case of main, 7.0, so we have to override AssemblyVersion
on the project directly.
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.
Done, I added a comment and also made FileVersion Major and Minor match the assembly version, then on 6.0 we can just bump the minor version and that way going forward FileVersion will match the assembly version.
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.
Small comment but other than that this looks good.
contributes to: #62218
This was regressed on: a505c46#diff-804ceb4a1df769aca14d1bd4043455cc1cce391376dcb02798cbf716cc6150eeL4