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

Allow custom fields to be added to the generated ThisAssembly class #565

Merged
merged 1 commit into from Mar 18, 2021

Conversation

tg73
Copy link
Contributor

@tg73 tg73 commented Mar 10, 2021

We're looking to retire our own in-house equivalent to NBGV which we rolled many years ago when there was no alternative. We have a few extra fields in our ThisAssembly class which are specific to our build process that we need to keep, so I've added this ability to NBGV.

Custom fields can be specified using MSBuild items, for example:

<ItemGroup>
  <AdditionalThisAssemblyFields Include="CustomString1" String="Hello, World!"/>
  <!-- Fields with empty values are not emitted by default. Set EmitIfEmpty if required: -->
  <AdditionalThisAssemblyFields Include="CustomString2" String="$(SomeProperty)" EmitIfEmpty="true"/>
  <AdditionalThisAssemblyFields Include="CustomBool1" Boolean="true"/>
  <AdditionalThisAssemblyFields Include="CustomDateTime1" Ticks="637505461230000000"/>
</ItemGroup>

At present, string, boolean and DateTime fields are supported. Unit tests have been updated with basic coverage in line with what was already provided.

@dnfadmin
Copy link

dnfadmin commented Mar 10, 2021

CLA assistant check
All CLA requirements met.

@tg73
Copy link
Contributor Author

tg73 commented Mar 10, 2021

As far as I can tell, this failure seems to have something to do with how the checks pipeline is configured, rather than because of a problem with the pull request. But please do disabuse me if I am wrong.

@AArnott
Copy link
Collaborator

AArnott commented Mar 11, 2021

How do you feel about ThisAssembly being a partial class and you continuing to generate your part of it? I'd rather not NB.GV become a dumping ground for arbitrary codegen into the ThisAssembly class, but have no objection to you adding to it via partial classes.

@tg73
Copy link
Contributor Author

tg73 commented Mar 11, 2021

Thanks for your comment, which I've given some careful thought.

If one defines "dumping ground" as being metadata that is not directly related to NB.GV's core concerns, then one could argue that ThisAssembly has already become a dumping ground: notably the fields PublicKey, PublicKeyToken and RootNamespace appear to be well outside NB.GV's concern; and taking a purist position, one could argue that only the Assembly*Verision, GitCommitId and GitCommitDate fields are truly NB.GV's concern. I think this existing "creep" highlights that ThisAssembly is a desirable feature in and of itself that is missing from the OOTB .NET/.NET SDK build system. So one approach would be to make ThisAssembly generation an entirely separate (and extensible) concern outside of NB.GV. This would I think arguably be a more correct solution in general. But that's quite a big change.

In terms of extensibility, I took inspiration from the OOTB targets that build AssembyInfo.cs (et al), to which NB.GV is in some respects analogous. Those targets provide at least two extension points: @(AssemblyMetadata), to which projects can add, causing System.Reflection.AssemblyMetadataAttribute instances to be added to AssemblyInfo.cs; and @(AssemblyAttribute), which it would appear allows any referenced assembly attribute to be added to AssemblyInfo.cs with arbitrary parameters. So here the pattern of allowing easy and arbitrary extension to the set of assembly-scope metadata emitted by code generation is demonstrated. My proposed change to NB.GV extends this pattern into ThisAssembly, which is just another form of assembly-scope metadata. Note that my change only allows additional immutable fields to be added (ie, assembly-scope metadata), it does not create or encourage more general "arbitrary codegen".

As regards making ThisAssembly partial: this would of course work, but it creates the burden of developing and maintaining a custom-made code generation task/targets implementation. While not difficult for someone familiar with MSBuild development, incremental build/clean etc, it's a significant barrier to anyone without that experience. From a performance perspective, it's also a frustrating duplication of temporary file generation/compare/replace. Another partial solution is to just rely upon @(AssemblyMetadata) as described above, but this would not expose compile time constants which are the only expression that can be used in certain circumstances, such as attribute parameters.

@AArnott AArnott changed the title Allow custom fields to be added to the generated ThisAssembly class. Allow custom fields to be added to the generated ThisAssembly class Mar 18, 2021
@AArnott AArnott merged commit e3cd90f into dotnet:master Mar 18, 2021
@AArnott
Copy link
Collaborator

AArnott commented Mar 18, 2021

Thank you for your contribution.

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

Successfully merging this pull request may close these issues.

None yet

3 participants