Skip to content

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 cgillum, jviau and nytian 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
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.

4 participants