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

Git commit ID included in assembly ProductVersion field when building with sdk 8 #34568

Closed
PurplestViper opened this issue Aug 10, 2023 · 31 comments
Assignees
Labels
Area-NetSDK untriaged Request triage from a team member
Milestone

Comments

@PurplestViper
Copy link

PurplestViper commented Aug 10, 2023

Describe the bug

Starting from dotnet sdk 8 preview 4, building an assembly with dotnet will include the git commit id in the ProductVersion field by default, is there any chance it will be removed?

To Reproduce

Exceptions (if any)

Further technical details

  • Include the output of dotnet --info
  • The IDE (VS / VS Code/ VS4Mac) you're running on, and its version
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-NetSDK untriaged Request triage from a team member labels Aug 10, 2023
@PurplestViper PurplestViper changed the title Git commit id in Assembly ProductionVersion field Git commit ID included in assembly ProductionVersion field when building with sdk 8 Aug 21, 2023
@PurplestViper PurplestViper changed the title Git commit ID included in assembly ProductionVersion field when building with sdk 8 Git commit ID included in assembly ProductVersion field when building with sdk 8 Aug 22, 2023
@marcpopMSFT
Copy link
Member

I believe this is related to some pieces of source link being enabled by default. @tmat to point to any documentation or announcement about this.

I believe if you set EnableSourceLink to false, it'll restore the old behavior.

@marcpopMSFT marcpopMSFT added this to the Discussion milestone Aug 25, 2023
@KalleOlaviNiemitalo
Copy link
Contributor

There's also the IncludeSourceRevisionInInformationalVersion property, mentioned in https://learn.microsoft.com/dotnet/core/project-sdk/msbuild-props.

@PurplestViper
Copy link
Author

It is necessary to enable source link support when developing, but this behavior change will only appear when updating the SDK, and if it is intentional, it should be pointed out in the change log. But I don't think this behavior should be enabled by default.

@KalleOlaviNiemitalo
Copy link
Contributor

The implicit Source Link is listed in https://learn.microsoft.com/en-us/dotnet/core/whats-new/dotnet-8#source-link but that doesn't say it will affect ProductVersion.

@KalleOlaviNiemitalo
Copy link
Contributor

Related issue for ProductVersion of NuGet.Common.dll and similar assemblies: NuGet/Home#11974

@TimKras
Copy link

TimKras commented Nov 15, 2023

@PurplestViper @KalleOlaviNiemitalo
Why is this item closed. It also impacting 'Hot Reload' in VS 2022 #36666

@PurplestViper
Copy link
Author

PurplestViper commented Nov 15, 2023

@PurplestViper @KalleOlaviNiemitalo Why is this item closed. It also impacting 'Hot Reload' in VS 2022 #36666

Simply upgrading the sdk will cause some programs that detect InformationalVersion to fail. Although setting IncludeSourceRevisionInInformationalVersion=false avoids the problem, I consider this a breaking behavior change.

  • Maybe source link should not be enabled by default
  • Or IncludeSourceRevisionInInformationalVersion should be false by default
  • Or at least possible problems should be described in the documentation.

This issue has not received an effective reply three months since it was opened. I hope to observe the butterfly effect by changing the issue status. Now I found this issue dotnet/docs#37674 .

@PurplestViper PurplestViper reopened this Nov 15, 2023
@tmat
Copy link
Member

tmat commented Nov 15, 2023

@PurplestViper Source Link is intentionally enabled by default. The breaking change in version attribute is documented in https://learn.microsoft.com/en-us/dotnet/core/compatibility/sdk/8.0/source-link.

@ghost
Copy link

ghost commented Nov 29, 2023

Really poor decision to do this, and not even mention this. I just wasted 8 hours combing build plans trying to work out where tis was coming from..

I now have to edit 400 project files and add a new property to disable the daft behavior.

@ChristoWolf
Copy link

Really poor decision to do this, and not even mention this. I just wasted 8 hours combing build plans trying to work out where tis was coming from..

I now have to edit 400 project files and add a new property to disable the daft behavior.

You could do it in a single file, but I agree, making this the default behavior is a breaking change for many.

@WeihanLi
Copy link
Contributor

I now have to edit 400 project files and add a new property to disable the daft behavior.

Maybe you could use Directory.Build.props to define it globally

@andrensairr
Copy link

andrensairr commented Nov 29, 2023

https://developercommunity.visualstudio.com/t/Build-adds-string-to-assembly-Informatio/10515014

I logged an issue here noting this change affected my .NET 4.7.1 project, where the SDK documentation clearly indicates this should affect .NET 8+ only. My reading of the discussion thus far suggests this is not what this present issue is intended to address. Am I right, and I should raise another issue to address that particular issue?

@tmat
Copy link
Member

tmat commented Nov 29, 2023

Any .NET SDK project (i.e. project that specifies <Project Sdk="Microsoft.NET.Sdk">) built with .NET SDK 8 will include the commit SHA in the informational version, regardless of the target framework.

To disable this new behavior you can include Directory.Build.props file in the repository root that contains:

<Project>
  <PropertyGroup>
    <IncludeSourceRevisionInInformationalVersion>false</IncludeSourceRevisionInInformationalVersion>
  </PropertyGroup>
</Project>

@andrensairr
Copy link

Thank you @tmat, that was my fix. That was not how I read the documentation, however, it says:

Starting in .NET 8, the default InformationalVersion of a library or application is the Version property and the SourceRevisionId property.

It reads like it's the target framework that should introduce the difference in behaviour. I can understand why the change in behaviour is not desired. Using .NET SDK 8 was not a conscious decision, it's something that happened for me merely by virtue of updating Visual Studio to 17.8. Surely such breaking changes should only occur when making a deliberate decision to update the target framework version? I'm sure I'm not alone in this: why look for documentation about breaking changes when I have not actually made any changes?

@adurmus
Copy link

adurmus commented Dec 20, 2023

Is there a way to disable this behaviour in .NET SDK? I see some suggested to use a single file to disable this but what if you have over 100 repositories? Do we need to add this single file to every single repository? There must be a better way of disabling this.

@ChristoWolf
Copy link

Is there a way to disable this behaviour in .NET SDK? I see some suggested to use a single file to disable this but what if you have over 100 repositories? Do we need to add this single file to every single repository? There must be a better way of disabling this.

I would use exactly that approach with the .props/.targets file as I have suggested above and simply generate or copy it to each repo during CI runs.
Or you could do the same by running such a generation script locally and committing all of them in their respective repos.

@KalleOlaviNiemitalo
Copy link
Contributor

Or you could set IncludeSourceRevisionInInformationalVersion as an environment variable; but this could be even harder to deploy.

@PurplestViper
Copy link
Author

PurplestViper commented Dec 26, 2023

Is there a way to disable this behaviour in .NET SDK? I see some suggested to use a single file to disable this but what if you have over 100 repositories? Do we need to add this single file to every single repository? There must be a better way of disabling this.

@marcpopMSFT @tmat
I think we need to gracefully switch this feature on and off so that it reduces work for most people with minimal changes, for example: dotnet bulid/publish --excludeCommitHash

@tmat
Copy link
Member

tmat commented Dec 27, 2023

Is there a way to disable this behaviour in .NET SDK? I see some suggested to use a single file to disable this but what if you have over 100 repositories? Do we need to add this single file to every single repository? There must be a better way of disabling this.

@marcpopMSFT @tmat I think we need to gracefully switch this feature on and off so that it reduces work for most people with minimal changes, for example: dotnet bulid/publish --excludeCommitHash

You can use /p command line switch to do so, e.g.

dotnet build /p:IncludeSourceRevisionInInformationalVersion=false

@adurmus
Copy link

adurmus commented Dec 27, 2023

Upgrading .net sdk shouldn't force users to change their build scripts. This behaviour should have been default to false. Anyone interested could enable this. Bad decision.

@tvert
Copy link

tvert commented Feb 9, 2024

Hi, I agreed with many, this is a poor decision to have this new feature enabled by default as a) it changes the a public value (i.e. the file version in 'InformationalVersion') and b) it affects existing projects targeting previous version of .NET.
I am glad other people started to document this, so this "breaking behavior" starts to have more documentated soltuon and people spend less time researching solution for this breaking change, but still a VERY POOR DECISION.

Why not have intoduced another attribute(s) related to "commit information" if that was the intent, i.e. provide info about the commit.
That way we could shared more info about the commit, e.g.: repo url, commit hash, date of the commit, author(s) ... Those fields would a) be new (no breaking changes to existing tools and process) , b) optional so people could choose which information they want to share about commit information, and c) not mix with the product's version or break existing fields that already have a meaning and a well established format used by people/tools.

@Nils-Berghs
Copy link

While you can debate on whether this change was necessary or not, the way it was implemented was definitely "not good"

If such a breaking change is made it would have made sense that the IncludeSourceRevisionInInformationalVersion was explicitly set to true en the default project templates (even if that was the default)

It should also not have been released without the IncludeSourceRevisionInInformationalVersion showing up in the intellisense when editing the project file. I know that the absolute first thing to do is to disable this in the project properties but I always have to copy it from another project / github as VS won't suggest it.

@bryanfarrell
Copy link

bryanfarrell commented Mar 27, 2024

@tmat @marcpopMSFT This issue should definitely not be closed. You guys have added a breaking change with the inclusion of Source Link and unfortunately the "fix" you have provided does not work correctly.

I have included <IncludeSourceRevisionInInformationalVersion>false</IncludeSourceRevisionInInformationalVersion> in my project file as suggested but this does not work correctly.

For example, we are compiling towards multiple target frameworks in our project file like so:
<TargetFrameworks>net472;net7.0;net8.0;netstandard2.0</TargetFrameworks>

When the project is built without the <IncludeSourceRevisionInInformationalVersion> property, all compiled target framework versions include the git commit hash, which is the functionality you added that is the breaking change.

However, when we set that property to false and rebuild the projects, the property does not behave as you have indicated.
The product version is correct (no git commit) for the net472 and netstandard2.0 versions of the assembly but the net7.0 and net8.0 version of the assembly ignore that property and continue to include the git commit in the product version.

As others have mentioned previously, this change should have been configured to support backwards compatibility first, especially for existing projects. It also seems to have not been carefully planned as the setting you are suggesting to use does not even work correctly for all target frameworks.

Can you please re-open this issue and look into why the suggested property is not working in the scenario I have described?

EDIT:

The previous analysis was done with VS 2022 v17.9.3. I posted this as I was updating VS 2022 to v17.9.5. After the latest update, it does seem the behavior has changed but still seems something is not working correctly yet.

With the property commented out, net8.0. net472 and netstandard2.0 include the git commit value as you intend, but net7.0 does not include it. This is inconsistent and I'm not sure what your default intention is (i.e. include git commit for all target frameworks in product version by default) or to just have it effect net8.0 exclusively. Regardless of the intention, this being inconsistent certainly suggests something is still not correct.

The good news is that with the property explicitly set to false, under VS 2022 v17.9.5, all four target frameworks are correctly removing the git commit from the product version, so that is definitely a step in the right direction.

I have not tested this with all possible target framework monikers that VS 2022 can compile, but it would see you guys should do some internal testing for all target framework monikers both as individual targets and in a collection as I have used, to verify the intended functionality is working correctly when you expect the git commit to be included as well as when it should not be included.

I do see the benefit of this addition but again think it should not have been forced upon the development community as a breaking change. A much better approach would have been to include it in a new features summary page when loading Visual Studio after the update, highlighting the benefits of Source Link so we can see it's benefits and decide on our own if and when we want to implement it in our code.

For a long time now Visual Studio updates have been pretty stable and changing to a new framework like NET 7 to NET 8 could be expected to have breaking changes. Just updating the IDE has typically not been one of those concerns.

How this was done is a bit concerning for my team as this broke things that should not have been broken with a VS update. I really hope in the future the Microsoft team takes more care in making changes like this and does so with backwards compatibility being the first preference. I suspect if you continue to make changes like this in the future, that the community may start being weary of your regular Visual Studio updates, which I don't think anyone wants. I hope you keep these thoughts in mind going forward because based on this thread it's clear that most of us are not happy with how this was handled.

@tmat
Copy link
Member

tmat commented Mar 27, 2024

@bryanfarrell Can you please share a repro project or a binlog showing that IncludeSourceRevisionInInformationalVersion does not work?

@bryanfarrell
Copy link

@bryanfarrell Can you please share a repro project or a binlog showing that IncludeSourceRevisionInInformationalVersion does not work?

@tmat please see the edit to my original post. The setting when on seems to now be working, at least for the target frameworks we are using. As I've already updated Visual Studio and rebuilt the project, I have no binlog that I can share with you.

As I mentioned in the edit, I would think a change like this should have a set of tests around it to make sure the behavior works correctly in all target framework configuration that VS 2022 supports. That at a minimum would make it easy for your team to verify that the expected behavior works as intended.

@tmat
Copy link
Member

tmat commented Mar 27, 2024

@bryanfarrell I tried a simple library project with the following settings:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFrameworks>net472;net7.0;net8.0;netstandard2.0</TargetFrameworks>
    <IncludeSourceRevisionInInformationalVersion>false</IncludeSourceRevisionInInformationalVersion>
  </PropertyGroup>
</Project>

As expected, git sha is not included in the versions of any of the output binaries.

If it doesn't work for you your build must be doing something different. I can't tell what it is unless you provide a repro project or a binlog (dotnet build /bl outputs msbuild.binlog).

@bryanfarrell
Copy link

@bryanfarrell I tried a simple library project with the following settings:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFrameworks>net472;net7.0;net8.0;netstandard2.0</TargetFrameworks>
    <IncludeSourceRevisionInInformationalVersion>false</IncludeSourceRevisionInInformationalVersion>
  </PropertyGroup>
</Project>

As expected, git sha is not included in the versions of any of the output binaries.

If it doesn't work for you your build must be doing something different. I can't tell what it is unless you provide a repro project or a binlog (dotnet build /bl outputs msbuild.binlog).

As I mentioned, with latest version of Visual Studio it is working when off but is not consistently working when the setting is ON. This is a private repo so I can't share that with you, and I am also not triggering this build directly with dotnet build, I am building from VS IDE. If you comment out <IncludeSourceRevisionInInformationalVersion>false</IncludeSourceRevisionInInformationalVersion>, you may see the behavior I am seeing (that net7.0 was not adding the git commit data). Just mentioning that it seems to be working inconsistently, at least from within the VS 2022 IDE when rebuilding the solution.

@tmat
Copy link
Member

tmat commented Mar 27, 2024

Does it always work correctly when building form command line (dotnet build)?

@tmat
Copy link
Member

tmat commented Mar 27, 2024

This is inconsistent and I'm not sure what your default intention is (i.e. include git commit for all target frameworks in product version by default) or to just have it effect net8.0 exclusively

The intention is that it is set for all TFMs. I can't reproduce behavior that you're describing. The generated *AssemblyInfo.cs file in obj directory (and subsequently the binary) contains the expected version string for all TFMs. This file is generated when the project is opened in VS even without building. Building from VS also works for me, as does building from command line.

@bryanfarrell
Copy link

This is inconsistent and I'm not sure what your default intention is (i.e. include git commit for all target frameworks in product version by default) or to just have it effect net8.0 exclusively

The intention is that it is set for all TFMs. I can't reproduce behavior that you're describing. The generated *AssemblyInfo.cs file in obj directory (and subsequently the binary) contains the expected version string for all TFMs. This file is generated when the project is opened in VS even without building. Building from VS also works for me, as does building from command line.

Perhaps it was caching the generated AssemblyInfo.cs file in the obj directory even though I cleaned and rebuilt the project. The good news is when I say to turn it off, it turns it off. So perhaps it's just a glitch with the obj folder not being cleared out. When I have time, I will see if I can clear everything completely and see if I get the same results.

@Mgamerz
Copy link

Mgamerz commented Jul 1, 2024

Ah, so this is what has been poisoning my telemetry data. Real smooth, really appreciate it Microsoft.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-NetSDK untriaged Request triage from a team member
Projects
None yet
Development

No branches or pull requests