-
Notifications
You must be signed in to change notification settings - Fork 440
Fix Alpine SB legs #19623
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 Alpine SB legs #19623
Conversation
| since the tests require the ability to execute the built SDK. An example of where this would be disabled is | ||
| cross-build of using Mariner to build for Alpine (linux vs linux-musl). --> | ||
| <_RunScenarioTests Condition="'$(BuildOS)' != 'windows' and '$(__PortableTargetOS.ToLowerInvariant())' != '$(TargetOS.ToLowerInvariant())'">false</_RunScenarioTests> | ||
| <_RunScenarioTests Condition="'$(BuildOS)' != 'windows' and '$(DotNetBuildSourceOnly)' == 'false' and '$(__PortableTargetOS.ToLowerInvariant())' != '$(TargetOS.ToLowerInvariant())'">false</_RunScenarioTests> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can someone help me understand why __PortableTargetOS is initialized to linux-musl on alpine while targetOS is linux? I'm trying to understand if the PortableTargetOS comparison against TargetOS is correct.
Given this is blocking. We should merge and circle back if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you asking in the context of SB?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you asking in the context of SB?
No, in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's look at an example. In UB cross build scenarios of building Alpine from Mariner, __PortableTargetOS is set to linux here: https://github.com/dotnet/arcade/blob/be933308b9024d798a9a22c0b8f3c8e3616ffbd8/eng/common/native/init-distro-rid.sh#L111
And TargetOS is set to linux-musl as a build property here:
| extraBuildProperties="$extraBuildProperties /p:TargetOS=${{ parameters.targetOS }}" |
This build property doesn't impact the value of targetOS in the init script that __PortableTargetOS is derived from.
/cc @ViktorHofer
| since the tests require the ability to execute the built SDK. An example of where this would be disabled is | ||
| cross-build of using Mariner to build for Alpine (linux vs linux-musl). --> | ||
| <_RunScenarioTests Condition="'$(BuildOS)' != 'windows' and '$(__PortableTargetOS.ToLowerInvariant())' != '$(TargetOS.ToLowerInvariant())'">false</_RunScenarioTests> | ||
| <_RunScenarioTests Condition="'$(BuildOS)' != 'windows' and '$(DotNetBuildSourceOnly)' == 'false' and '$(__PortableTargetOS.ToLowerInvariant())' != '$(TargetOS.ToLowerInvariant())'">false</_RunScenarioTests> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we ever set DotNetBuildSourceOnly to false. This should probably be changed to '$(DotNetBuildSourceOnly)' != 'true'".
The changes from #19222 caused a regression for source build Alpine build legs, causing the following error:
This is due to the change made in dotnet/dotnet@546a6bb#diff-e98dbe2aa8231e9d6264210a63f02e97b5e26de2cca16db7ee471a2257f32c8f to define
targetOSfor the Alpine build legs. That change was intended to make this check succeed: https://github.com/dotnet/dotnet/blob/00d6710677d75715dedef053b70a34b09af8bc47/test/tests.proj#L14.The fix is to remove the setting of
targetOSand update the test condition to not apply for source build since source build doesn't use portable RIDs.