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.

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

Fix Microsoft.VisualBasic.Core file version #62848

Merged
merged 2 commits into from
Dec 16, 2021

Conversation

safern
Copy link
Member

@safern safern commented Dec 15, 2021

@ghost
Copy link

ghost commented Dec 15, 2021

Tagging subscribers to this area: @cston
See info in area-owners.md if you want to be subscribed.

Issue Details

contributes to: #62218

This was regressed on: a505c46#diff-804ceb4a1df769aca14d1bd4043455cc1cce391376dcb02798cbf716cc6150eeL4

Author: safern
Assignees: safern
Labels:

area-Microsoft.VisualBasic

Milestone: -

@@ -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>
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member

@ericstj ericstj Dec 15, 2021

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

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.

Small comment but other than that this looks good.

@safern safern merged commit 6b45b53 into dotnet:main Dec 16, 2021
@safern safern deleted the VisualBasicFileVersion branch December 16, 2021 05:03
safern added a commit to safern/runtime that referenced this pull request Dec 16, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jan 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants