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

[Bug] Assembly Informational Version custumization ignored #2199

Closed
fedebn opened this issue Mar 23, 2020 · 26 comments
Closed

[Bug] Assembly Informational Version custumization ignored #2199

fedebn opened this issue Mar 23, 2020 · 26 comments

Comments

@fedebn
Copy link

fedebn commented Mar 23, 2020

Using GitVersion.yml to set custumized Assembly Informational Version, any charater used to separate version number from ShortSha is ignored and substitued with -.

Expected Behavior

verified with version 5.1.3

assembly-informational-format: '{Major}.{Minor}.{Patch}_{ShortSha}'
1.2.3_856daf9

Actual Behavior

verified with version 5.2.4

assembly-informational-format: '{Major}.{Minor}.{Patch}_{ShortSha}'
1.2.3-856daf9
@fedebn fedebn added the bug label Mar 23, 2020
@fedebn fedebn changed the title [Bug] [Bug] Assembly Informational Version custumization ignored Mar 23, 2020
@asbjornu
Copy link
Member

This change might have been introduced in #2014. Perhaps we need a more fine-grained replacement strategy here. 🤔

@fedebn
Copy link
Author

fedebn commented Mar 23, 2020

I don't know which charaters are not allowed in SemVer, but i think alphanumeric charaters are not allowed, but some symbols like '-' '_' '.' '|' are allowed, right?

@asbjornu
Copy link
Member

The regex of characters allowed in a SemVer 2.0 compatible pre-release label is [0-9A-Za-z-]. So I suppose the question here is: Should we allow InformationalVersion to be incompatible with SemVer?

@fedebn
Copy link
Author

fedebn commented Mar 23, 2020

If the scope of GitVersion is to use InformationalVersion as SemVer, if think this bug can be rejected

@asbjornu
Copy link
Member

Well, no, it's the other way around. GitVersion generates a SemVer 2.0 compatible version number, then provides many different serializations and formats of this version numbers targeting different environments.

Since AssemblyInformationalVersionAttribute allows for a larger set of characters than SemVer 2.0, I think it's natural to think that assembly-informational-format should only affect AssemblyInformationalVersionAttribute and thus allow characters not allowed in SemVer 2.0. I'm just not quite sure yet how to implement it.

@fedebn
Copy link
Author

fedebn commented Mar 23, 2020

So, i'm not sure, but we can think about a warning at compile time if AssemblyInformationalVersionAttribute isn't comply with SemVer 2.0.

@asbjornu
Copy link
Member

Providing the warning at compile time is going to be difficult, but we could log a warning statement during execution.

@NeilMacMullen
Copy link

NeilMacMullen commented May 19, 2020

Speaking only as a user, but we wish to use AssemblyInformationVersionAttribute as a way of holding more detailed build information than can be obtained from the SemVer format. (For example, the date of the commit, name of build-machine/user, what the user ate for breakfast etc...) By all means, have a way of stuffing a SemVer-compatible string into this field but, please also allow for those of us who want to include arbitrary information. I'm aware we could create a custom attribute ourselves but since gitversion already does 95% of what we want in setting up the attributes, it seems a shame not to also use it for customised info.

The 'flattening' of characters into hyphens is particularly annoying when trying to treat the information field as structured data since dates and branch-names also regularly contain hyphens, a custom format such as
assembly-informational-format: '{MajorMinorPatch} {CommitDate} {ShortSha} {BranchName} {env:USERNAME} {env:BREAKFAST}'
can turn into

"1.7.6-2020-05-18-e1bbd7e-bug-fix-345-neilm-fried-eggs"
which means extraction of fields is much more awkward.

@asbjornu
Copy link
Member

Sure, if someone provides a pull request that doesn't restrict the format of AssemblyInformationVersionAttribute with tests, it will be merged. As none of the maintainers of GitVersion has a need for this feature, it is very unlikely any of us are going to spend our free time implementing this, sorry.

@NeilMacMullen
Copy link

Seems reasonable - just wanted to check there is no underlying design-philosophy that means that such a PR would be rejected?

@asbjornu
Copy link
Member

No, as I wrote in #2199 (comment), the current behavior is just a conservative default. We might want to allow the current behavior to continue as is and figure out a backwards compatible way to implement this, though. One way is to introduce a configuration value, but we may also look at how this can be solved better with a breaking change in the upcoming version 6 of GitVersion.

Ideas are welcome to be discussed here before a pull request is submitted.

@LazaroOnline
Copy link
Contributor

I created a pull request for this as I also need this feature:
#2341

@asbjornu
Copy link
Member

Thanks @LazaroOnline. As I wrote in #2341 (comment) we should be careful to effectively revert #2014. Since our format code already supports an extended syntax through {env:VAR ?? "fallback-value-if-env-var-does-not-exist"} (see #1385 and #2179), one possible solution may be to extend this syntax to apply (or not apply) character replacement. I'm not sure what syntax we should provide this functionality with, though. Here's a wild idea:

{CommitsSinceVersionSource ? $.Raw }

Where the full extent of the possible syntax would look like this:

{env:VAR ? $.Raw ?? "fallback-value-if-env-var-does-not-exist"}

The idea is that ? acts almost like a ternary operator, falling through to $.Raw if env:VAR exists. Then $ acts like an implicit this and .Raw is a property exposed on the class that represents the value of env:VAR that will give you its Raw, unreplaced value.

This means that the following:

{CommitsSinceVersionSource}

Would become equivalent to:

{CommitsSinceVersionSource ? $.ToString() }

Where the class wrapping the value of CommitsSinceVersionSource would have a .ToString() method that does character replacement.

Thoughts?

@LazaroOnline
Copy link
Contributor

I think people that want InformationalVersion to use special char replacement, they need it in the whole value altogether, so I would prefer a new config parameter like:

assembly-informational-is-semver: true

It would be true by default to keep retro compatibility. If set to false then it would allow any special character.

The special syntax could be a more fine grained feature, but I prefer something simple like a boolean variable that is easier to understand than learning the special meaning of a custom template syntax, that I think would be harder to implement.
One of my goals is to keep the template simple so I can do something like:
assembly-informational-format: '{BranchName}-{ShortSha}-{CommitDate} - Asbjørn Team'
instead of something like:
assembly-informational-format: '{BranchName}-{ShortSha}-{CommitDate} - {Asbjørn Team ? $.Raw}'

Would this solution by configuration be acceptable?

@NeilMacMullen
Copy link

As an interested observer I prefer the simplicity of the configuration flag (regardless of whether it defaults to true or false). I think the design intent that all the main version fields should be SemVer is clear and understandable so introducing a special syntax that would allow this to be overridden for any of them seems over-complicated when "information" is arguably a special-case.

@LazaroOnline
Copy link
Contributor

I updated the pull request to be backwards-compatible and with the new config parameter assembly-informational-is-semve as described above.

@asbjornu
Copy link
Member

I agree my suggested syntax is a bit convoluted, I just want to think through all alternatives before adding configuration. We already have too much configuration, imho. If we are going to solve this with a configuration property, we should try to approach it from a high level perspective and consider whether what's being asked for here and in #2065 may be related, for instance.

In #2065, it has been made clear that being able for GitVersion to serialize version numbers in various different schemes could be useful. Some environments, such as Python, PHP Composer, Ruby Gems, Docker and possibly others only support a subset of SemVer 2.0. Being able to reflect these variations with some sort of scheme configuration property would be useful.

Could the same scheme (not just affecting the pre-release label) be used to say that the serialization of InformationalVersion, pre-release-label (and possibly more) could be made either more strict or more relaxed than SemVer 2.0?

Thoughts?

@LazaroOnline
Copy link
Contributor

I see the use-case of other languages for some sort of scheme for the GitVersion generated SemVer to adhere to their language/platform requirements.

However I feel that what we are trying to achieve with this bug/PR is something different.
We just want to have a free text field to fill with whatever information (hence the informational name) using any character, so for this use-case, we are not trying to follow SemVer nor any scheme at all, and that should be a valid goal independently of the language or scheme used, as even the Python community might want to follow their schema but also have a free text template.

I feel that microsoft's dotnet field AssemblyInformationalVersion was meant for this, just to give total free text, like a description, that's why the field doesn't even seem to have a maximum char limit.

Also the scheme feature mentioned in the Python bug, I feel it is far from being implemented because the problem of adding a "scheme" feature with different platform support requires deep knowledge of many other platforms and testing becomes harder.
Even if it is a commodity for users, it also becomes a burden for this project that I think it would be better solved by using extension mechanism following the Open-Close principle, maybe python can have a library that wraps GitVersion and assigns the configuration that python expects, just like dotnet has the GitVersion.Task NuGet.
It could be developed under the same GitTools group in GitHub, but it should not need changes in the main GitVersion project other than allowing proper extensibility if it is not already there.
That way there would be no need for more configurations and the new parameter proposed in the PR would still make sense.
I fear that the python/docker/PHP scheme issue would be blocking this PR unnecessarily for a long time, when this bug is in reality a simple one that could be solved today.

Maybe to accept this PR it would help to rename the new configuration parameter from:
assembly-informational-is-semver: true
to assembly-informational-allow-special-chars: false
or assembly-informational-allow-any-character: false
or assembly-informational-allow-free-text: false
so it makes more sense in the case the scheme config gets added.

Would that help?

@arturcic
Copy link
Member

I agree my suggested syntax is a bit convoluted, I just want to think through all alternatives before adding configuration. We already have too much configuration, imho.

I do agree there are too many configurations there. We need to take this in consideration for version 6.0 and limit the configuration properties only needed for calculation purposes. All the others should be removed or moved to the output plugins (either as separate configuration file or as cli arguments) @asbjornu would you agree?

@NeilMacMullen
Copy link

NeilMacMullen commented Jun 26, 2020

I rather like @asbjornu's scheme idea. You could regard all fields as having an implicit scheme of SemVer2. @LazaroOnline's new configuration could then be rewritten as assembly-informational-scheme : raw.

What's less clear is whether the scheme is simply a constraint on allowable output characters or is in some way enforcing a deeper correctness. I can currently write

assembly-informational-format: '-abd012.{MajorMinorPatch}.45def-'

The hyphens may be removed to make it "SemVer2 compliant" but it certainly won't be a "SemVer2" version number so the constraint here is a fairly shallow one and not massively helpful.

And what would you if there was a hypothetical scheme where versions had to be expressed as string representations of numbers? Would setting the scheme to "stringified" cause {MajorMinorPatch} to be rendered as "one_point_two_point_three" ?

I raise these points not to be awkward but show that it's probably impossible to find the perfect solution to this. I'll say again that my primary use-case for GitVersion is as a convenient way of getting version information into AssemblyInfo files. The fact that it (currently) has some canned variables that correspond to the way that SemVer expects to interpret stuff is great but if I had to write {Major}.{Minor}.{Patch} or {MajorMinorPatch_SemVer2} or even {SEMVER2(MajorMinorPatch)} I wouldn't be too unhappy. What is frustrating is having the tool override my specified formatting - by all means emit a warning but please allow me to shoot myself in the foot! ;-)

@LazaroOnline
Copy link
Contributor

Let me explain why I really need this feature.
GitVersion is good for producing SemVer numbers for assemblies, but also for adding any git version related information to them, and GitVersionTask is also a way for us to automatically embed that info into dll/exe.
Having a free text field that embeds GitVersion information is a key point for extensibility.
Projects can take advantage of it for example by running a script that updates the GitVersion.yml assembly-file-versioning-format property appending whatever extra information that GitVersion can't get, but it becomes messed up and hard to work with if the text gets replaced by dashes.

Not implementing this because of having too many configuration keys sounds a bit arbitrary. If the feature makes sense and adds a desired feature then why not?
We can always change them, group them or whatever in the upcoming versions.

There could be many valid use cases to have free text, I will state one.
In our company we use GitVersion to check the version of dlls in the servers so we know what was deployed (looking at the sha commit id).
One of the problems we have at our company for example is that we need to know if the dll was built with local changes (isDirty) or not, since we have seen many cases of dlls deployed from developer's local machines.
Even if deploying a dirty compiled dll sounds bad, in practice it does happen and it is out of my reach to prevent, for example in small companies or when there is a deadline to meet and they don't have time to follow proper deploying procedures, or when employees just disregard them.
In the free text field we can add the isDirty flag and even list the count of pending changes and filenames, machinename used to compile, anything we come up with limited only by our imagination.

Even if the feature to add an IsDirty flag gets implemented, I would like to be able to extend it to list the dirty filenames/paths and the count of lines added/deleted, because in the past we even had to do reverse engineering over DLLs handed over by other dev teams that left the company, in order to know what was the version of the source code running in production and what parts were actually missing in git because someone deployed a dirty version and never pushed it to git!
In that scenario the dirty filenames are a great help.

Also for CI/CD automation it would be very interesting to be able to add free text to assemblies that could be parsed by custom tools.

It is all about extensibility!

@asbjornu
Copy link
Member

@arturcic:

I do agree there are too many configurations there. We need to take this in consideration for version 6.0 and limit the configuration properties only needed for calculation purposes. All the others should be removed or moved to the output plugins (either as separate configuration file or as cli arguments) @asbjornu would you agree?

💯

@LazaroOnline:

GitVersion is good for producing SemVer numbers for assemblies, but also for adding any git version related information to them, and GitVersionTask is also a way for us to automatically embed that info into dll/exe.
Having a free text field that embeds GitVersion information is a key point for extensibility.

Yes, I understand why the feature is valuable, we just disagree on the implementation.

Not implementing this because of having too many configuration keys sounds a bit arbitrary.

Adding another configuration option to the core is not cost-free. Seeing how this feature only is about the serialization of version numbers, the feature belongs at the edge of GitVersion, not in the Core. With GitVersion 6, this entire feature could be delegated to its own GitVersion extension and we wouldn't have to add yet another configuration option to the Core, adding more complexity to the already over-complex codebase.

Configuration options are already a major source of problems, because they have been added without thinking thoroughly through how option A affects option B. This leads to a lot of support issues and makes maintaining this repository much harder than it should. Not only the code, but all the support and help these conflicting configuration options necessitates.

Not having the best documentation around any of the configuration options available doesn't exactly help either.

If the feature makes sense and adds a desired feature then why not?

The feature makes sense, but I'm very hesitant to add more configuration options, especially when they don't deal with core functionality such as how version numbers are calculated, but instead interface and edge-level concerns like serialization and formatting.

Usability decreases as flexibility increases.

Usability decreases as flexibility increases

We can always change them, group them or whatever in the upcoming versions.

Sure, but wouldn't it better if we managed to implement this in a way that was forwards-compatible with future versions of GitVersion, instead of implementing something now that we know is going to be changed in v6?

One of the problems we have at our company for example is that we need to know if the dll was built with local changes (isDirty) or not, since we have seen many cases of dlls deployed from developer's local machines.

This use-case is much better solved with signed tags or signed commits, imho. Instead of embedding metadata (that can also be screwed with), you can cryptographically verify that the build comes from the correct source by including the signature of the tag or commit in the build artifact itself, with signed NuGet packages, signed JAR files (JAR are just fancy ZIP files, so could be used to store any kind of content) or something similar.

Even if deploying a dirty compiled dll sounds bad, in practice it does happen and it is out of my reach to prevent, for example in small companies or when there is a deadline to meet and they don't have time to follow proper deploying procedures, or when employees just disregard them.

Sounds like better solution would be to not give developers direct access to the production environment, but instead require them to go through a CI/CD procedure with a release manager such as Octopus Deploy.

Also for CI/CD automation it would be very interesting to be able to add free text to assemblies that could be parsed by custom tools.

Is AssemblyInformationalVersion the right place to add this information, though? I understand that the field is just a string and as such can support an entire SOAP request with XSD, WSDL and whatnot if you want to. But its name contains the word Version, which indicates that the information provided should be tightly related to the version number.

Adding attributes to assemblies that can contain arbitrary metadata is a nice feature, but I don't think it belongs in GitVersion.

Now, don't get me wrong. I may sound negative – I'm not. I think the proposed feature is important and that we need it in GitVersion. I'm just a bit sad we're having this discussion now before the extension system of v6 is in place and that we have to debate short-term solutions such as adding a configuration option.

@LazaroOnline
Copy link
Contributor

As far as it ends up implemented it is good for me.
If the v6 is planned to be released in less than half a year or so, then I understand not making a change in the v5, so feel free to reject the PR.
I wonder how is the new extension system going to work and in what way this feature will be solved, maybe I can help with some ideas, is there any place/url to discuss about it?

About the company deployment issues, sure there are proper procedures and we do have a CI/CD pipeline, but I don't get to decide when and if it is ok to bypass it, and the company is happy to allow anything as far as things are done in time, at the end what gives them profit is gaining new customers that want to see a certain new feature.
The developers are not doing it in bad faith, they just follow the path of least resistance, meaning that they will deploy the dll with the metadata indicating their local changes without trying to hide it, some of them may don't even notice it is there. I just need to configure the project in a way so that if it happens I will be able to notice by checking the server files, maybe even creating a custom tool that checks regularly if any dll/exe has certain keyword like "isDirty", at the end this is also info related to the git version used to build.

Thank you for your time and for understanding the value of this request.

@asbjornu
Copy link
Member

If the v6 is planned to be released in less than half a year or so, then I understand not making a change in the v5, so feel free to reject the PR.

I don't think any amount of planning is going to help the progress of v6, but it is being worked on and will be ready when it's ready. Sorry for not being able to be more specific, but we are all working on this on our spare time.

I wonder how is the new extension system going to work and in what way this feature will be solved, maybe I can help with some ideas, is there any place/url to discuss about it?

You can take a look at the GitVersionV6 prototype repository for a glimpse of some of our ideas. Still early days, but it should be enough to give you an idea.

The developers are not doing it in bad faith, they just follow the path of least resistance, meaning that they will deploy the dll with the metadata indicating their local changes without trying to hide it, some of them may don't even notice it is there.

Exactly. The solution to this is to make the path of least resistance the right one to take.

I just need to configure the project in a way so that if it happens I will be able to notice by checking the server files, maybe even creating a custom tool that checks regularly if any dll/exe has certain keyword like "isDirty", at the end this is also info related to the git version used to build.

I believe extensions like these may be possible to create in v6. Perhaps not in 6.0.0, but at least during its lifetime.

Thank you for your time and for understanding the value of this request.

Likewise! :)

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity. After 30 days from now, it will be closed if no further activity occurs.

@github-actions github-actions bot added the stale label Apr 13, 2023
@github-actions
Copy link

This issue was closed because it has been stalled for 30 days with no activity. Thank you for your contributions

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants