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

Allow TargetOS and TargetRid to be overridden in DotNetBuild.props #113765

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jkoritzinsky
Copy link
Member

This is needed to fix the linux-bionic builds in the VMR.

Contributes to dotnet/source-build#4955

This is needed to fix the linux-bionic builds in the VMR
@Copilot Copilot bot review requested due to automatic review settings March 21, 2025 16:10

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.

Files not reviewed (1)
  • eng/DotNetBuild.props: Language not supported
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

@jkoritzinsky jkoritzinsky requested a review from a team March 21, 2025 16:11
Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

Doesn't this imply that due to L18, the passed in TargetOS value will never be used?

@jkoritzinsky
Copy link
Member Author

Yes. The problem is that the runtime's build scripts process and change the passed in TargetOS when determining the RID. Because of our "nested inner build" logic, we effectively were stripping the "-bionic" part of "linux-bionic" from the inner build invocation and building the product in a weird blend of linux, linux-bionic, and android.

By letting this calculation happen again and overwriting the TargetOS based on the RID, we can restore the calculation of the TargetOS to the correct value by re-extracting it from the RID (which we calculate based on the originally-passed-in TargetOS).

@tmds
Copy link
Member

tmds commented Mar 24, 2025

My 2 cents: it may be interesting to globally introduce a property that is the "portable rid OS corresponding to the target". That one would be linux-bionic. dotnet/sdk#44800 is also about having such a property.

@am11
Copy link
Member

am11 commented Mar 24, 2025

Because of our "nested inner build" logic, we effectively were stripping the "-bionic" part of "linux-bionic" from the inner build invocation and building the product in a weird blend of linux, linux-bionic, and android.

It sounds like we are missing handling of linux-bionic somewhere next to the existing handling of linux-musl which should have the same behavior but doesn't (e.g. alpine distro packages use source-build without issues). If we can find and fix that problem, I think that would be better targeted fix rather than exposing these props.

Ideally, eng/OSArch.props + eng/RuntimeIdentifier.props type of platform spec resolution should be shared via arcade/eng/common so the handling of supported platforms and effective platforms (build/host/target) is unified across the board; single source of truth.

@jkoritzinsky
Copy link
Member Author

Alpine doesn't have a problem because musl and glibc builds are more similar.

@jkoritzinsky
Copy link
Member Author

Coming back to this, here's the flow of what's happening in main and why musl doesn't have a problem here:

  1. When the VMR orchestrator passes -os linux-bionic to the runtime outer build script, we hit this block:

    runtime/eng/build.sh

    Lines 290 to 292 in 7efe7f7

    linux-bionic)
    os="linux"
    __PortableTargetOS=linux-bionic
  2. We pass the os variable as the TargetOS property to the build:
    arguments="$arguments /p:TargetOS=$os"
  3. We initialize the RID based on the os variable:

    runtime/eng/build.sh

    Lines 133 to 147 in 7efe7f7

    initDistroRid()
    {
    source "$scriptroot"/common/native/init-distro-rid.sh
    local passedRootfsDir=""
    local targetOs="$1"
    local targetArch="$2"
    local isCrossBuild="$3"
    # Only pass ROOTFS_DIR if __DoCrossArchBuild is specified and the current platform is not an Apple platform (that doesn't use rootfs)
    if [[ $isCrossBuild == 1 && "$targetOs" != "osx" && "$targetOs" != "android" && "$targetOs" != "ios" && "$targetOs" != "iossimulator" && "$targetOs" != "tvos" && "$targetOs" != "tvossimulator" && "$targetOs" != "maccatalyst" ]]; then
    passedRootfsDir=${ROOTFS_DIR}
    fi
    initDistroRidGlobal "${targetOs}" "${targetArch}" "${passedRootfsDir}"
    }
  4. The outer build tries to evaluate the TargetOS property based on the RID, but this is ignored:
    <TargetOS>$(TargetRid.Substring(0, $(_targetRidPlatformIndex)))</TargetOS>
  5. We pass the TargetOS property to the inner build:
    <InnerBuildArgs Condition="'$(DotNetBuildSourceOnly)' != 'true'">$(InnerBuildArgs) $(FlagParameterPrefix)os $(TargetOS)</InnerBuildArgs>
  6. The inner build sees the TargetOS property as linux instead of linux-bionic:

    runtime/eng/build.sh

    Lines 264 to 265 in 7efe7f7

    linux)
    os="linux" ;;
  7. As linux-bionic is not a cross build, but linux is, we end up not having the -cross flag and build fails.
  • linux-musl is a cross-build like linux, so -cross will be present. Since we have the correct RID passed in, we end up building everything correctly even though we pass -os linux today.
  • If we pass the -cross flag for linux-bionic. We must pass a rootfs. If we pass a rootfs, we end up building the Android crypto stack instead of the OpenSSL one because we miss setting the "Force use OpenSSL Crypto" due to the rootfs being provided.

This change lets the logic at step 4 correct the adjustment in step 2 using the RID that's passed in by the orchestrator.

I'd like to get in this change as-is. After we switch to flat code-flow to the VMR, we can revisit how we handle RIDs and platform specification and simplify it from the VMR where we can do all the changes in one PR.

@tmds
Copy link
Member

tmds commented Mar 26, 2025

Note that in the first step, build.sh sets __PortableTargetOS=linux-bionic. dotnet/runtime then uses that to initialize _portableOS_. __PortableTargetOS/_portableOS_ gets lost when it goes from the vmr outer build to the runtime inner build (TargetOS=linux). For linux-musl the __PortableTargetOS gets "recovered" here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants