ModelsBuilder generated files use scientific notation for version number

tormnator commented 8 months ago

Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)

12.3.6

Bug summary

The release part of the version parameter on the GeneratedCodeAttribute is written in scientific notation, e.g. "12.3.6+e8a81e3" when "12.3.6" would be sufficient.

Specifics

No response

Steps to reproduce

I use the following settings in appSettings.json:

      "ModelsBuilder": {
        "ModelsMode": "SourceCodeManual",
        "FlagOutOfDateModels": true,
        "ModelsDirectory": "~/App_Code/Models.Generated"
      },

Build, run and open the site's back office. Then go to the Models Builder tab under Settings and click "Generate Models".

Expected result / actual result

The expected version number would be something like "12.3.6", but in my case the generated files actually look like this:

github-actions[bot] commented 8 months ago

Hi there @tormnator!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

We'll assess whether this issue relates to something that has already been fixed in a later version of the release that it has been raised for.
If it's a bug, is it related to a release that we are actively supporting or is it related to a release that's in the end-of-life or security-only phase?
We'll replicate the issue to ensure that the problem is as described.
We'll decide whether the behavior is an issue or if the behavior is intended.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

Thanks, from your friendly Umbraco GitHub bot :robot: :slightly_smiling_face:

tormnator commented 8 months ago

Another question is whether that attribute needs to be there, or if it does, if the version parameter needs to be there. Having it there causes the generated files to frequently be modified (i.e. whenever Umbraco is updated), requiring a new check-in into the version control repository. One might say that the generated files shouldn't be in the repository in the first place, but for Umbraco Cloud I don't think there's a way around including them. Or?

elit0451 commented 7 months ago

Hi @tormnator 👋 Thank you for reaching out!

You are right, you need to commit them into the git repository for each Umbraco Cloud environment. So, changing the version number to just "12.3.6" for example, seems like a good idea. I will mark this as up for grabs if anyone is up for it 🙂

github-actions[bot] commented 7 months ago

Hi @tormnator,

We're writing to let you know that we would love some help with this issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.

For more information about issues and states, have a look at this blog post.

Thanks muchly, from your friendly Umbraco GitHub bot :-)

skttl commented 7 months ago

@elit0451 do we need the version number at all? They are quite annoying, and as @tormnator mentions, cumbersome to work with, as every upgrade changes all model files.

tormnator commented 7 months ago

Looks like it should be an easy fix to remove the scientific notation part. The method to change is https://github.com/umbraco/Umbraco-CMS/blob/contrib/src/Umbraco.Infrastructure/ModelsBuilder/Building/TextBuilder.cs#L146.

There are also source code xml comments with references (url's) which could be helpful in determining if it's possible to reduce or remove the version number.

tormnator commented 7 months ago

Upon further review, that suffix is not a number in scientific notation, but the build number, i.e. major.minor.patch.build. It looks like it's taken from the current executing assembly, which must be Umbraco.Infrastructure.dll.

Also, after googling around and asking ChatGPT, I can't find any references to tools or other consumers actually using the Version property, and assume it's only there for informational purposes. The documentation states this property is the version of the tool which generated the code. Since the tool name is hardcoded as "Umbraco.ModelsBuilder.Embedded", maybe we could solve this by also hardcoding the version number. At least it seems slightly inaccurate to use the assembly's version number.

To summarize, for me the grievance is that the presence of the full version number in the attribute causes the generated files to too often show up as changed and in need of commit in my local repo for Umbraco cloud. If the version number was more stable, the generated files would only show up as changed when the actual generated code is changed.

I assume this is core enough for the masters of Umbraco to decide before making any code changes/PR's.

abjerner commented 7 months ago

I think is somewhat normal/standard to include the assembly name and version in auto-generated. Microsoft does something similar for some of their tools. But I can see why it might not make so much sense in this context, so could make sense to make a away to disable that. On the other hand, one could argue that it's nice to have the version included - eg. to better debug any issues that might occur.

Anyways, the part after the + is the first seven characters of the SHA1 hash of the commit the Umbraco DLL is based on. I think Microsoft introduced this behavior as part of the .NET 8 release, although it's probably more tied to the compiler and not so much the target framework, as the informational version will still have the SHA1 hash even if building with older target frameworks.

nul800sebastiaan commented 7 months ago

Just to chip in: indeed 12.3.6+e8a81e3 and 12.3.6 are the same (one just has part of the commit hash in it, it's the same version and it only ever changes when your Umbraco version changes).

I asked for some insights as to what the version was for from the original developer Stephane as well: https://hachyderm.io/@zpqrtbnk/111844477726539958

What problem are we really trying to solve here? What about the SHA bothers you so that it needs to be removed?

skttl commented 7 months ago

Every time Umbraco upgrades (both minors and patches), all model files changes, but they only change the version number. This is quite annoying, as you then have to decide if you want to include all those changes in your Pull Request, even if they don't actually do any changes.

Example

You have a working site.
Umbraco upgrades to 13.1.1.
You are tasked to do some kind of feature that changes a doctype
When generating the new models files (SourceCodeManual), it changes ALL models files
You then either need to include a bunch of irrelevant files in your PR, or reset changes to a lot of files.

If the number is not important, or anything, I would love if it was removed.

I have the same issue with Deploys .uda files.

nul800sebastiaan commented 7 months ago

Okay, original issue was about the SHA though!

I understand the version number is slightly annoying. Looks like GeneratedCodeAttribute asks for a version number but it looks like it's nullable so it could be removed. Since the standalone version of MB is no longer a thing I think that's ok.

Make sure to create an issue on the Deploy issue tracker if you want them to consider removing the version number.

umbraco / Umbraco-CMS