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

Fix for #60532 #60572

Merged
merged 1 commit into from
Oct 18, 2021
Merged

Fix for #60532 #60572

merged 1 commit into from
Oct 18, 2021

Conversation

trylek
Copy link
Member

@trylek trylek commented Oct 18, 2021

In stable package mode we should be setting CoreLib informational
version to the ProductVersion according to the discussion on the
issue thread. I have verified locally that this fixes the processinfo2
test for me that was previously failing in the StabilizePackageVersion
mode. Please let me know how to proceed with the fix, whether you
want me to just merge it into dotnet/runtime main, whether I should
pursue its backport into 6.0 release and / or whether Matt considers
cherry-picking my change to his stabilization PR.

Thanks

Tomas

Fixes #60532

/cc @dotnet/runtime-infrastructure

In stable package mode we should be setting CoreLib informational
version to the ProductVersion according to the discussion on the
issue thread. I have verified locally that this fixes the processinfo2
test for me that was previously failing in the StabilizePackageVersion
mode. Please let me know how to proceed with the fix, whether you
want me to just merge it into dotnet/runtime main, whether I should
pursue its backport into 6.0 release and / or whether Matt considers
cherry-picking my change to his stabilization PR.

Thanks

Tomas
@hoyosjs
Copy link
Member

hoyosjs commented Oct 18, 2021

For main we might also want to make sure that IsPrivateAssembly and SuppressFinalPackageVersion are false and that IsShippingPackage is true. Basically, the DBprops looks at Private in the name and thinks it's not meant to be a stable assembly.

@trylek
Copy link
Member Author

trylek commented Oct 18, 2021

OK, merging this in with the understanding that the related scripts merit additional cleanup but that's not blocked on "day D".

@trylek trylek merged commit 1c949ac into dotnet:main Oct 18, 2021
@trylek trylek deleted the CoreLibInformationalVersionFix branch October 18, 2021 20:51
akoeplinger added a commit to akoeplinger/runtime that referenced this pull request Oct 19, 2021
@hoyosjs
Copy link
Member

hoyosjs commented Oct 20, 2021

@mmitche @Anipik isn't this needed in 6.0?

akoeplinger added a commit that referenced this pull request Oct 20, 2021
@Anipik
Copy link
Contributor

Anipik commented Oct 21, 2021

@mmitche @Anipik isn't this needed in 6.0?

@jeffschwMSFT @danmoseley will be the right person to make a decision on this.

@jeffschwMSFT
Copy link
Member

@hoyosjs / @trylek can you elaborate on the impact and if we need this in .NET 6? We are about to produce our first candidate build.

@trylek
Copy link
Member Author

trylek commented Oct 21, 2021

The impact is that, without the fix, the shipping System.Private.CoreLib would be marked with an internal informational version "6.0.0-dev" instead of the expected "6.0.0". According to MSIL documentation runtime itself should never look at this value, it's just an informational text as the name suggests, we only hit this issue in a test that happens to compare it to coreclr.dll version information (which is correctly set to "6.0.0"). It was most likely the same in .NET 5 and frankly speaking I don't think anyone noticed in the last year or so.

@jeffschwMSFT
Copy link
Member

I think we should take this. @trylek can you create a backport?

@trylek
Copy link
Member Author

trylek commented Nov 5, 2021

/backport to release/6.0

@github-actions
Copy link
Contributor

github-actions bot commented Nov 5, 2021

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1423935628

@github-actions
Copy link
Contributor

github-actions bot commented Nov 5, 2021

@trylek an error occurred while backporting to release/6.0, please check the run log for details!

Error: @trylek is not a repo collaborator, backporting is not allowed.

@jkotas
Copy link
Member

jkotas commented Nov 5, 2021

/backport to release/6.0

@github-actions
Copy link
Contributor

github-actions bot commented Nov 5, 2021

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1423971588

jkotas pushed a commit that referenced this pull request Nov 9, 2021
Anipik pushed a commit that referenced this pull request Nov 9, 2021
* Fix for #60532

In stable package mode we should be setting CoreLib informational
version to the ProductVersion according to the discussion on the
issue thread. I have verified locally that this fixes the processinfo2
test for me that was previously failing in the StabilizePackageVersion
mode. Please let me know how to proceed with the fix, whether you
want me to just merge it into dotnet/runtime main, whether I should
pursue its backport into 6.0 release and / or whether Matt considers
cherry-picking my change to his stabilization PR.

Thanks

Tomas

* Port InformationVersion fix to Mono corelib (#60614)

#60572 for Mono's corelib.

Co-authored-by: Tomas Rylek <trylek@microsoft.com>
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLRInformationVersion contains the buildNumber or version suffix
6 participants