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

Auto-generate AssemblyInfo + refactor version into common props. #393

Merged
merged 10 commits into from
May 9, 2024

Conversation

davidmrdavid
Copy link
Member

@davidmrdavid davidmrdavid commented May 3, 2024

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 .csprojs.

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.

@davidmrdavid davidmrdavid requested review from jviau, nytian and cgillum May 3, 2024 21:18
@cgillum
Copy link
Member

cgillum commented May 3, 2024

common.props works, but even simpler would be to use Directory.Build.props, because all project files in the same directory hierarchy get the definitions get imported automatically (you don't need to do an explicit import in each .csproj file). More info here: https://learn.microsoft.com/visualstudio/msbuild/customize-by-directory?view=vs-2022. This is the approach I've been using for all new projects. Here's a recent example.

@davidmrdavid
Copy link
Member Author

davidmrdavid commented May 3, 2024

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).

@jviau
Copy link
Member

jviau commented May 3, 2024

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 dotnet build -pp:pp.xml , which will output pp.xml. It is thousands (tens even?) of lines long. Directory.Build.props is just a drop in the bucket. Plus, once you know how it works, is it magic anymore?

<PatchVersion>1</PatchVersion>
<VersionPrefix>$(MajorVersion).$(MinorVersion).$(PatchVersion)</VersionPrefix>
<VersionSuffix></VersionSuffix>
<AssemblyVersion>$(MajorVersion).0.0.0</AssemblyVersion>
Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member

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.

@davidmrdavid
Copy link
Member Author

Plus, once you know how it works, is it magic anymore?

I personally have no strong feelings on Build.directory.props vs common.props, I just think that since there's still an unresolved conversation about it in another thread, I shouldn't bypass that discussion in this PR. So I'm opting for a different, more "low tech" approach. Alternatively, I can just remove this part of the PR, as my main goal was to automate the AssemblyInfo metadata.

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.

Copy link
Member

@cgillum cgillum left a 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. :)

@davidmrdavid davidmrdavid merged commit 51c6b95 into main May 9, 2024
2 checks passed
@davidmrdavid davidmrdavid deleted the dajusto/remove-version-duplicate branch May 9, 2024 22:19
@davidmrdavid
Copy link
Member Author

FYI @sebastianburckhardt of this change. Hoping it will minimize release mishaps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants