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

Add support for additional assembly metadata in AssemblyInfo.cs #895

Closed
johncrim opened this issue Jun 8, 2016 · 6 comments

Comments

@johncrim
Copy link

commented Jun 8, 2016

My use-case is that I'd like to include additional git metadata in the assembly on top of AssemblyVersion, AssemblyFileVersion, and AssemblyInformationalVersion. While I can figure out a way to do this outside of GitVersion, this seems like a feature that belongs in GitVersion.

For example, here are some lines that I would like to append to my AssemblyInfo.cs or GlobalAssemblyInfo.cs:

[assembly: AssemblyMetadata("Git.BranchName", "develop")]
[assembly: AssemblyMetadata("Git.Sha", "aafdf7b9087d386666cb8a3d2910447ce765be66")]
[assembly: AssemblyMetadata("BuildHost", "builder2")]
[assembly: AssemblyMetadata("BuildTimestamp", "20160607-18:53:00Z")]

I like these assembly attributes because they're easy to examine across multiple assemblies.

Obviously the git values are already available, and it would be nice to be able to use the existing logic to add/replace these attributes in the *AssemblyInfo.cs files.

There's a case to be made that BuildHost and BuildTimestamp don't belong in gitversion, but given that all the other *AssemblyInfo.cs modification logic is in GitVersion, it would be nice to be able to pass these in too.

I would imagine these names + value formats could be configured using command-line parameters or using the GitVersion.yml file.

I'm willing and interested in implementing this if you think it's something you would accept if well implemented.

@asbjornu

This comment has been minimized.

Copy link
Member

commented Jun 13, 2016

@johncrim Seems like a lot of the stuff you're after is already embedded in the assembly through the autogenerated static class GitVersionInformation. If anything's missing in that class, please feel free to submit a PR, but as you can see here, all properties of the VersionVariables class are injected into the static class, so nothing should be missing.

@johncrim

This comment has been minimized.

Copy link
Author

commented Jun 14, 2016

@asbjornu - thank you for the response.

The static class is suboptimal for a couple reasons:

  1. It only exists when the msbuild step is used (we're using the command-line app)
  2. Assembly attributes can be examined in a way that isn't coupled to gitversion, and doesn't require reflecting and calling an internal class.
@asbjornu

This comment has been minimized.

Copy link
Member

commented Jun 15, 2016

@johncrim Those are good points. I'd like to fix them in another way, though. There's some relevant discussions on the reasons behind using a static class in OctopusDeploy/OctoPack#69 and #531. @SimonCropp's example there, illustrates it quite well:

var assemblyName = assembly.GetName().Name;

var gitVersionInformationType = assembly.GetType(assemblyName + ".GitVersionInformation");
var versionField = gitVersionInformationType.GetField("NuGetVersion");
Trace.WriteLine(versionField.GetValue(null));

var gitVersionInformationAttributeType = assembly.GetType(assemblyName + ".GitVersionInformationAttribute");
var attribute = assembly.GetCustomAttribute(gitVersionInformationAttributeType);
var attributeVersionField = gitVersionInformationAttributeType.GetProperty("NuGetVersion");
Trace.WriteLine(attributeVersionField.GetValue(attribute));

Since the attribute is an instance, it requires one extra line of reflection code to extract. Therefore, my suggestion is to consolidate the assemblyinfo update logic as per #883, such that any discrepancy between GitVersionTask and GitVersion.exe (as well as the different supported languages; C#, Visual Basic and F#) that now exists, go away.

It's obvious that the MSBuild task and the EXE should behave the same way across all languages, but that's currently sadly not the case. If you're up for tackling that, I'd be more than happy to merge your pull request.

We can do this in baby steps, where the first PR just injects the GitVersionInformation class somewhere closer to GitVersionCore, so GitVersionTask and GitVersionExe both performs it in the same, consistent way.

What do you think?

@johncrim

This comment has been minimized.

Copy link
Author

commented Jun 15, 2016

I'm willing to give it a go using whatever approach provides the best utility for the community.

I agree 100% that GitVersionExe and GitVersionTask should have consistent behavior and would be great to make GitVersionInformation a shared feature.

I've reviewed the issues you referenced, and I don't see an explanation for why a static class is preferable to assembly attributes - can you shed some more light on that?

Obviously assembly attributes are a key part of the assembly design and are used for binding and assembly versioning already. They're exposed by disassembly tools, etc.

For my purposes, I already have some tools that expose assembly version info and AssemblyMetadataAttribute values for assemblies in a web service. This attribute appears to be intended for additional key-value assembly metadata, and using it doesn't require a new dependency (which IMO would be the strongest reason for not using an attribute - and AssemblyMetadataAttribute appears to be in coreclr mscorlib as well).

If this feature is only implemented using a static internal class, then anyone consuming gitversion-created assembly metadata will have to consume it differently than they consume other assembly metadata. Since gitversion is a build tool it seems reasonable to me to support emitting the info in whatever form works best for the consumer.

How do you feel about me attempting support for both use-cases?

@asbjornu

This comment has been minimized.

Copy link
Member

commented Jun 21, 2016

@johncrim Since I haven't heard from anyone else regarding this, I'd say: Go for it. If you need any help submitting the PR, please let me know!

@asbjornu

This comment has been minimized.

Copy link
Member

commented Mar 11, 2018

Closing due to inactivity. We're still open for pull requests, though. 😃

@asbjornu asbjornu closed this Mar 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.