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 versions for Libs subset's native artifacts #52799

Merged
merged 1 commit into from
May 18, 2021

Conversation

am11
Copy link
Member

@am11 am11 commented May 15, 2021

During the repo consolidation, we missed passing OfficialBuildId
argument to common script from all subsets, which resulted in default
version emitted in the binaries produced by Libs subset in the official
builds.

During the repo consolidation, we missed passing `OfficialBuildId`
argument to common script from all subsets, which resulted in default
version emitted in the binaries produced by Libs subset in the official
builds.
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@am11
Copy link
Member Author

am11 commented May 15, 2021

cc @janvorli

before

file sccid
libSystem.Globalization.Native.so @(#)Version 42.42.42.42424
libSystem.Native.so @(#)Version 42.42.42.42424
libmscordbi.so @(#)Version 6.0.21.26599 @Commit: e01ffb9bb49c66d5d0cad430b3079d0b7bd7ca09
libcoreclr.so @(#)Version 6.0.21.26599 @Commit: e01ffb9bb49c66d5d0cad430b3079d0b7bd7ca09

...

after

file sccid
libSystem.Globalization.Native.so @(#)Version 6.0.21.26599 @Commit: e01ffb9bb49c66d5d0cad430b3079d0b7bd7ca09
libSystem.Native.so @(#)Version 6.0.21.26599 @Commit: e01ffb9bb49c66d5d0cad430b3079d0b7bd7ca09
libmscordbi.so @(#)Version 6.0.21.26599 @Commit: e01ffb9bb49c66d5d0cad430b3079d0b7bd7ca09
libcoreclr.so @(#)Version 6.0.21.26599 @Commit: e01ffb9bb49c66d5d0cad430b3079d0b7bd7ca09

...

@am11
Copy link
Member Author

am11 commented May 18, 2021

CI failures are unrelated to PR changes.

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for noticing the issue and fixing it!

@janvorli
Copy link
Member

@am11 do we have the same issue in 5.0 too?

@am11
Copy link
Member Author

am11 commented May 18, 2021

@janvorli, yes, 5.0 has this problem as well and it was a regression from 3.1 where we have correct version in all ELF files produced by official build; shared object and executable. ebdcb5e fixed it in executable files, this PR fixes the remaining shared objects.
If backporting to 5.x is in the realm of possibilities, then IMO we should consider both patches for consistency.

@janvorli
Copy link
Member

@am11 thank you for the details, I'll create a porting PR after this is merged in.

@janvorli janvorli merged commit 4267d56 into dotnet:main May 18, 2021
@janvorli
Copy link
Member

/backport to release/5.0

@github-actions
Copy link
Contributor

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/853353504

@am11 am11 deleted the feature/product-versioning branch May 18, 2021 13:06
@akoeplinger
Copy link
Member

I think this causes an issue on the runtime-staging pipeline in Build Browser wasm release Browser_wasm_Windows

EXEC : CMake error : The source directory "D:/Unix" does not exist. [D:\workspace\_work\1\s\src\libraries\Native\build-native.proj]

I've asked @radekdoulik to help investigate.

@ViktorHofer
Copy link
Member

Seeing this in my PR as well: #52897. To unblock CI, should we condition to only pass the property in if it set?

@am11 it's unclear to me how this code path actually works. Where is OfficialBuildId parsed and then honored in the cmd and sh scripts?

@akoeplinger
Copy link
Member

should be fixed by #52917

@github-actions
Copy link
Contributor

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/853353504

@akoeplinger
Copy link
Member

Hmm for some reason it thinks you're not a contributor: Error: @janvorli is not a repo collaborator, backporting is not allowed. which is weird, probably something wrong with the token being used for the GitHub API.

@akoeplinger
Copy link
Member

Fixed the backport bot issue with #52938 but I think we'll need to do a manual backport anyway since there were a few follow-ups.

janvorli added a commit to janvorli/runtime that referenced this pull request May 19, 2021
Port of dotnet#52799 and dotnet#52917

During the repo consolidation, we missed passing OfficialBuildId
argument to common script from all subsets, which resulted in default
version emitted in the binaries produced by Libs subset in the official
builds.
@janvorli
Copy link
Member

@am11 I've created a port to release/5.0. I've actually found that only the libraries change was needed, the coreclr and installer binaries including the singlefilehost had a correct version. I can see that there was some change in the installer repo in 6.0 related to the versions, so that probably broke the installer.

@am11
Copy link
Member Author

am11 commented May 20, 2021

@janvorli, thank you. I think the singlefilehost patch would be good to add (the file was moved by the same change applies). Currently, on Ubuntu commit info is there but version is the placeholder (42.42.42..) in 5.0 installed with apt:

$ strings /usr/share/dotnet/packs/Microsoft.NETCore.App.Host.linux-x64/5.0.5/runtimes/linux-x64/native/singlefilehost | grep '(#)'
@(#)Version 42.42.42.42424 @Commit: 2f740adc1457e8a28c1c072993b66f515977eb51

@janvorli
Copy link
Member

@am11 - that is strange, the version is correct in my local build that I've just produced using ./build.sh installer -c Release -rc Release /p:OfficialBuildId="20210520.1"

strings /home/janvorli/git/runtime4/artifacts/bin/linux-x64.Release/corehost/singlefilehost | grep "@(#)"
@(#)Version 5.0.721.27001 @Commit: d9951ed03172641d98d02e06a4f8ad8da5b56f15

@am11
Copy link
Member Author

am11 commented May 20, 2021

I ran sudo apt --only-upgrade install dotnet-sdk-5.0 to upgrade to 5.0.6 and it apparently still shows the same version

$ strings /usr/share/dotnet/packs/Microsoft.NETCore.App.Host.linux-x64/5.0.6/runtimes/linux-x64/native/singlefilehost | grep '(#)'
@(#)Version 42.42.42.42424 @Commit: 478b2f8c0e480665f6c52c95cd57830784dc9560

Perhaps it is fixed for 5.0.7, I will double check once it is released.

@am11
Copy link
Member Author

am11 commented May 20, 2021

@ViktorHofer, it gets ultimately passed to empty.csproj here:

"$__RepoRootDir/eng/common/msbuild.sh" /clp:nosummary "$__ArcadeScriptArgs" "$__RepoRootDir"/eng/empty.csproj \

which generates version.c file under obj directory (via arcade versioning targets).

Currently, we emit sccid only in Unix binaries, and RC resource on windows based on the info derived from OfficialBuildId and version control ID (commit hash).

Vitek's idea was to also emit sccid in Windows binary (as it simply improves diadnostibility in headless environments), which would requires some changes in the arcade target.

@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Anipik pushed a commit that referenced this pull request Jun 3, 2021
* [Release/5.0] - Fix versions for Libs native artifacts

Port of #52799 and #52917

During the repo consolidation, we missed passing OfficialBuildId
argument to common script from all subsets, which resulted in default
version emitted in the binaries produced by Libs subset in the official
builds.

* Fix empty build version case
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants