-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Try to untangle the rid calculation. #82832
Conversation
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. |
796337e
to
ebf8d01
Compare
02c0f79
to
e420bf5
Compare
CI is looking good for these changes, with the exception of the coreclr linux_musl_arm64 job. This is a cross build job, and not I'm familiar with those. From the error message, I assume the wrong linker gets used for the crossgen2 project? @wfurt @am11 any thoughts on what changes in this PR are causing this failure?
|
Just a heads up: we spent days of testing in previous consolidation of RID calculation from various places in the repo. There are many scenarios not obvious and not covered by the CI. Then some folks only run their build flavors near the releases. Some edge-cases from memory:
Then we had issue with what goes into the official nuget packages and we went back and forth on fixing those. |
bf9c686
to
ed9c6b4
Compare
CI is looking good. |
@@ -253,11 +250,13 @@ | |||
<OutputRID Condition="'$(OutputRID)' == ''">$(_outputOS)-$(TargetArchitecture)</OutputRID> | |||
</PropertyGroup> | |||
|
|||
<PropertyGroup Label="CalculateTargetOSName" Condition="'$(SkipInferTargetOSName)' != 'true'"> | |||
<PropertyGroup Label="CalculateTargetOSName"> |
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.
Nit: Would be good to move this up in a follow-up and add TargetsMobile into the group as well.
@@ -73,7 +73,7 @@ | |||
<TestContextVariable Include="NUGET_PACKAGES=$(TestRestorePackagesPath)" /> | |||
<TestContextVariable Include="TEST_ARTIFACTS=$(SystemPathTestsOutputDir)" /> | |||
<TestContextVariable Include="TEST_TARGETRID=$(TestTargetRid)" /> | |||
<TestContextVariable Include="BUILDRID=$(OutputRid)" /> | |||
<TestContextVariable Include="BUILDRID=$(OutputRID)" /> |
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.
Nit: Should the test context variable be changed in the future to OutputRID?
<InnerBuildArgs>$(InnerBuildArgs) /p:RuntimeOS=$(RuntimeOS)</InnerBuildArgs> | ||
<!-- PackageOS and ToolsOS control the rids of prebuilts consumed by the build. | ||
They are set to RuntimeOS so they match with the build SDK rid. --> | ||
<InnerBuildArgs Condition="'$(RuntimeOS)' != ''">$(InnerBuildArgs) /p:PackageOS=$(RuntimeOS) /p:ToolsOS=$(RuntimeOS)</InnerBuildArgs> |
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.
Who provides RuntimeOS in source build?
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.
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.
Is that still the right property to set or do we want to change that?
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.
Approving with the caveat that I didn't understand all the changes in init-distro-rid.sh but if our builds are happy with the changes then so am I. Great work. Thanks a lot 💯
Test failures are unrelated. I will merge and if something starts to fail, we can revert the change anytime. |
@ViktorHofer and @akoeplinger thanks for your support! |
@tmat the
I will allow us 24 hours to investigate the issue, otherwise we will need to revert the change, unfortunately. |
I submitted a fix for it to unblock official builds: #85215 The behavior change was unexpected. Previously, we didn't build the runtime.$(OutputRID).*.proj projects (only System.IO.Ports) in that leg as the glob pattern didn't match anything as OutputRID was set to something different: runtime/src/libraries/oob-src.proj Lines 18 to 21 in 46101c4
Now that |
I assume this issue is specific to this type of 'cross-build': on Windows we build for Linux. Once we figure out what the rids were, we should be able to match the previous behavior.
What are these projects?
If you can unblock the official builds, we can fix this in the coming days. |
Previously, when we were building on Windows and set <_runtimeOS Condition="'$(_runtimeOS)' == ''">$(_parseDistroRid.SubString(0, $(_distroRidIndex)))</_runtimeOS>
...
<_runtimeOS Condition="'$(TargetsMobile)' == 'true'">$(TargetOS.ToLowerInvariant())</_runtimeOS>
...
<_portableOS Condition="'$(_runtimeOS)' == 'win' or '$(TargetOS)' == 'windows'">win</_portableOS> To go back to the previous behavior, we can set <_packageOS Condition="'$(_hostOS)' == 'windows' and '$(TargetsMobile)' != 'true'">win</_packageOS> I assume the goal of this configuration is being able to build managed sources on Windows for non-Windows targets. @ViktorHofer can you verify on Windows if the build works as before by adding |
Correct. The "source-index-stage" job which runs in our official builds is what powers https://source.dot.net. It just needs to build the managed libraries under src/libraries. We need to skip the native packages under https://github.com/dotnet/runtime/tree/main/src/libraries/System.IO.Ports/pkg as they currently require the native assets of the TargetOS to be built. Cross-building linux assets on a Windows OS isn't a supported scenario today AFAIK. I submitted a workaround to fix the failing official builds via #85239. My previous attempt was flawed as I missed that we can't build the libs.native subset targeting Linux, on Windows.
let me check. FWIW here's the build command:
|
Interesting to know, I was wondering why the job was building sources like that.
Thanks for looking into the fix. |
Unfortunately, appending
|
Yes. I didn't understand well enough what these projects were and should have taken a closer look. Somehow I had assumed they'd be generated using Thanks for getting the issue fixed. |
Sure, but my fix was just meant as a workaround. Would it make sense to additionally add the following back?
|
@tmds - This change broke the stage2 build of source-build. The PackageRID is set to portable e.g.
The first PackageRID assignment will always be overridden by the second. |
Yes, this is an error. This should have been similar to: Lines 226 to 228 in 0fc8978
|
@MichaelSimons #85350 adds the missing |
This is an attempt at simplifying how the rids get calculated.
Instead of providing a single rid,
init-distro-rid.sh
also provides a__PortableOS
value which is the portable os that matches the target platform. It gets initialized also for non-portable builds, which enables detecting musl in the native script and passing that information along. The existing__DistroRid
is used solely for naming the non-portable build output.Instead of calculating the
_portableOS
of the old__DistroRid
,Directory.Build.props
can now directly use__PortableOS
(and fall back to usingTargetOS
)._runtimeOS
is merged into_packageOS
.Let's see how well this works against the CI configurations. 🤞
cc @ViktorHofer @am11 @jkotas