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

Source Link should have more detailed information as it has unexpected consequences #38404

Closed
ScarletKuro opened this issue Nov 22, 2023 · 7 comments · Fixed by #38777
Closed
Assignees
Labels
dotnet-fundamentals/svc Pri1 High priority, do before Pri2 and Pri3 📌 seQUESTered Identifies that an issue has been imported into Quest.

Comments

@ScarletKuro
Copy link

ScarletKuro commented Nov 22, 2023

Type of issue

Missing information

Description

I want to mention this issues: dotnet/runtime#95134, dotnet/sdk#36885, dotnet/sdk#34568, dotnet/sdk#37158, dotnet/sdk#37142, stackoverflow
As the issues show, the SourceLink adds commit hash to the product version. This might be unexpected because this wasn't included in .net 6 & 7.
For example my application extracts the product version and the commit hash wasn't expected, also we use product version for SCCM and the hash is also not desired.

My problem with the current documentation is that the SourceLink description is too vague, it doesn't say anything about the consequences, for example i did not know it includes commit hash to the product version. It also doesn't link to the sourcelink build properties, in my opinion <IncludeSourceRevisionInInformationalVersion>false</IncludeSourceRevisionInInformationalVersion> is hard to discover if you want everything to work as before.
Maybe it would make sense to add this information to the https://learn.microsoft.com/en-us/dotnet/core/compatibility/8.0 instead of the "What's new".

Page URL

https://learn.microsoft.com/en-us/dotnet/core/whats-new/dotnet-8

Content source URL

https://github.com/dotnet/docs/blob/main/docs/core/whats-new/dotnet-8.md

Document Version Independent Id

b22cffcc-898f-a3cf-83f9-a3c7349a1b14

Article author

gewarren

Metadata

  • ID: 9d8a154a-d129-7e5a-e2aa-8d1a720252c5
  • Product: dotnet-fundamentals

Associated WorkItem - 189675

@ScarletKuro
Copy link
Author

ScarletKuro commented Nov 22, 2023

Apparently there is this documentation https://learn.microsoft.com/en-us/dotnet/core/compatibility/sdk/8.0/source-link
But I don't understand why it's not listed in the overview https://learn.microsoft.com/en-us/dotnet/core/compatibility/8.0 or why its not linked here https://learn.microsoft.com/en-us/dotnet/core/whats-new/dotnet-8#source-link
In my opinion this information should be duplicated everywhere

@tpressley
Copy link

tpressley commented Nov 27, 2023

I feel like this should have been disabled by default. It had a lot of negative consequences in our system, including disabling our entire logging system because the version # was too long to insert into logs.

Edit: I want to clarify our project files still target .NET 7.

@ScarletKuro
Copy link
Author

Edit: I want to clarify our project files still target .NET 7.

Yes, if you target .net7 in TFM but build against net8 SDK, you still will be affected.

@radj307
Copy link

radj307 commented Nov 27, 2023

Agreed about having it disabled by default. Even with extensive documentation, having to disable it in every project that could be built on a system with the .NET 8 SDK is quite frustrating. Especially frustrating since it affects projects that don't even target .NET 8.

I encountered this issue when the github actions runners upgraded to the .NET 8 SDK & previously-working project files targeting .NET 6 started having the commit hash appended to their product versions unexpectedly.

Also, while IncludeSourceRevisionInInformationalVersion is fairly descriptive, it's very long for a property that must be included to maintain the default behaviour of previous .NET SDK versions. Microsoft's explanation for this being the default behaviour is also very lacking:

Source Link enables rich editor tooling, like go-to-definition support for non-local source files. This benefit is worth including by default for all artifacts.

This benefit is not worth including by default for executables that will be publicly released. The product version field is the only place that you can (sensibly) put an actual semver2 version number without it being mangled, and having to take extra steps to prevent that should never be the default behaviour. Additionally, most Windows users access version information for an executable through the Details tab in the properties window, which doesn't allow copying the values. Making those version numbers even harder to accurately type is not good.

Overall, this makes it much more confusing for people that are coming to C# from other languages like C++. I certainly wouldn't have expected this to be the default behaviour, and I'm fairly confident that most developers would agree with me on that. If this is going to continue being the default behaviour of the .NET SDK going forward, it should be shouted from the rooftops & made exceedingly clear in both the build log and the SDK documentation for all versions (since it affects all versions).

@gewarren
Copy link
Contributor

@baronfel FYI

@WeihanLi
Copy link
Contributor

I feel like this should have been disabled by default

What about adding <IncludeSourceRevisionInInformationalVersion>false</IncludeSourceRevisionInInformationalVersion> for the target frameworks below .NET 8, then we would not break the project already using it for .NET 8 projects, though it's still a breaking-change

@gewarren gewarren added the 🗺️ reQUEST Triggers an issue to be imported into Quest. label Dec 4, 2023
@github-actions github-actions bot added 📌 seQUESTered Identifies that an issue has been imported into Quest. and removed 🗺️ reQUEST Triggers an issue to be imported into Quest. labels Dec 4, 2023
@gewarren gewarren moved this from 🔖 Ready to 🏗 In progress in dotnet/docs December 2023 sprint Dec 14, 2023
@ghost ghost added the in-pr This issue will be closed (fixed) by an active pull request. label Dec 14, 2023
@gewarren gewarren moved this from 🏗 In progress to 👀 In review in dotnet/docs December 2023 sprint Dec 14, 2023
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in dotnet/docs December 2023 sprint Dec 14, 2023
@dotnet-bot dotnet-bot added ⌚ Not Triaged Not triaged and removed ⌚ Not Triaged Not triaged labels Dec 14, 2023
@ghost ghost removed the in-pr This issue will be closed (fixed) by an active pull request. label Dec 14, 2023
@thedarkfalcon
Copy link

thedarkfalcon commented Oct 16, 2024

Echoing everyone's comments here and in related threads, this is terrible behaviour to enable by default, especially when most .net8 documentation pages fail to mention it.
One of our newer projects is running .net8 which required .net8 to be installed on our Azure build agents, now to maintain functionality we now have to add <IncludeSourceRevisionInInformationalVersion>false</IncludeSourceRevisionInInformationalVersion>to hundreds (maybe even thousands) of csproj files.
Having this long version string breaks a lot of integrations with installer creators, deployment services, and artefact services.
User experience for looking at these long version strings is also really bad, not that I think users should even be getting commit IDs etc told to them about the software they have. If MS really wanted to force this behaviour, it really shouldn't be in the version string property IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dotnet-fundamentals/svc Pri1 High priority, do before Pri2 and Pri3 📌 seQUESTered Identifies that an issue has been imported into Quest.
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

7 participants