-
Notifications
You must be signed in to change notification settings - Fork 26
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
Auto-generate AssemblyInfo + refactor version into common props. #393
Conversation
|
Hmm, I know Sebastian had some reservations about the "magic" of directory.build.props, from here: #224 (comment) . So although I'd prefer to "stick to convention" and use "directory.build.props", I think using the more manual approach of this PR may be less controversial (and obvious in how it works). |
But there is also so much more to msbuild 'magic' than Directory.Build.props - so why single that out? You can view the stitched together import graph by running |
<PatchVersion>1</PatchVersion> | ||
<VersionPrefix>$(MajorVersion).$(MinorVersion).$(PatchVersion)</VersionPrefix> | ||
<VersionSuffix></VersionSuffix> | ||
<AssemblyVersion>$(MajorVersion).0.0.0</AssemblyVersion> |
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 does pinning assembly version to only major offer?
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 a convention I think @cgillum first introduced to the DF assemblies. I recall it being related to hotswapping dlls. Can't say, I'll let him explain.
Note - I did not add this pinning to the repo, this already existed
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.
@jviau The original Azure Functions engineering manager who was previously on the ASP.NET team strongly recommended this to avoid problems related to compatibility issues with GAC'd assemblies. I can't remember exactly what all the issues were, but it made sense at the time and been trying to follow it.
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.
Ah, back in ASP.NET days yeah the GAC was relevant. I don't think it is anymore. This is safe to remove nowadays. I don't see this pattern present anymore - not in func host, or dotnet builds itself.
I personally have no strong feelings on No strong feelings here. Happy to remove this part of the PR if we'd rather "do things right" from the beginning instead of taking this partial step. |
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.
Given Sebastian's strong feelings about Build.Directory.props, I'm inclined to say let's just go with what you have (common.props). It's largely a matter of preference so I'd like to respect the wishes of the primary maintainer. :)
FYI @sebastianburckhardt of this change. Hoping it will minimize release mishaps. |
The Netherite version of this PR: microsoft/durabletask-mssql#213
Our most recent Netherite release accidentaly missed updating the AssemblyInfo.cs file, leading to the following issue: #390
This PR does two things in response:
(1) Auto-generates the AssemblyInfo file, as in microsoft/durabletask-mssql#213
(2) Refactor the version properties in a
common.props
file that is referenced by all.csproj
s.Open question: On VS, I struggled to add the common.props video to the Solution view, which would make editing it inside the IDE difficult. I'd like to fix that before merging this.