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

Make the cross-component build just another invocation of the build-runtime script #67108

Merged
merged 17 commits into from
Apr 1, 2022

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Mar 24, 2022

Simplify the CoreCLR build scripts by making the "cross component" builds be separate invocations of the script instead of having the builds included in the build-runtime script. With this change, each build-runtime.cmd/sh runs one build (excluding the all command on the Windows script).

This PR also updates our native build scripts to use the same terminology as the CMake scripts for architecture/OS (switch to Build/Host/Target OS/Arch terminology) for an easier understanding of what's going on.

This is a UX change for people who directly use the build-runtime script, so it should wait for an infra rollout to be merged.

@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.

@ghost ghost assigned jkoritzinsky Mar 24, 2022
@ghost
Copy link

ghost commented Mar 24, 2022

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

Simplify the CoreCLR build scripts by making the "cross component" builds be separate invocations of the script instead of having the builds included in the build-runtime script. With this change, each build-runtime.cmd/sh runs one build (excluding the all command on the Windows script).

This PR also updates our native build scripts to use the same terminology as the CMake scripts for architecture/OS (switch to Build/Host/Target OS/Arch terminology) for an easier understanding of what's going on.

This is a UX change for people who directly use the build-runtime script, so it should wait for an infra rollout to be merged.

Author: jkoritzinsky
Assignees: jkoritzinsky
Labels:

area-Infrastructure-coreclr

Milestone: -

When building cross-dac builds, we still end up running restore on runtime-prereqs.proj and runtime.proj, so we end up overridding the NuGet-generated MSBuild files, causing weird failures in CI.
@jkoritzinsky jkoritzinsky marked this pull request as ready for review March 29, 2022 22:50
@jkoritzinsky
Copy link
Member Author

@hoyosjs for review

cc: @dotnet/dotnet-diag for visibility on changes to how the cross-OS DAC builds invoke the build-runtime script.
cc: @dotnet/jit-contrib for visibility on changes to how the cross-targetting tools (not clr.alljits, but for example the runs-on-x64 targets-ARM JIT that's built when you build the runtime targetting ARM to enable running crossgen2 on CoreLib)

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

LGTM.

Seems like there should be a section in https://github.com/dotnet/runtime/blob/main/docs/workflow/building/coreclr/README.md (or https://github.com/dotnet/runtime/blob/main/docs/workflow/building/coreclr/cross-building.md, or somewhere similar) that talks about how to build the cross components. It looks like before it "just happened" (although I normally disabled it in my inner loop work), but now the commands to do it look non-obvious and "hidden" in YML file.

src/coreclr/build-runtime.cmd Outdated Show resolved Hide resolved
src/coreclr/build-runtime.cmd Outdated Show resolved Hide resolved
@@ -106,7 +106,7 @@ jobs:
- script: $(Build.SourcesDirectory)/src/coreclr/build-runtime$(scriptExt) $(buildConfig) $(archType) $(crossArg) -ci $(compilerArg) -component alljits -component spmi
displayName: Build CoreCLR JIT
- ${{ if eq(parameters.osGroup, 'windows') }}:
- script: set __TestIntermediateDir=int&&$(Build.SourcesDirectory)/src/coreclr/build-runtime$(scriptExt) $(buildConfig) $(archType) -ci -component alljits -component spmi
- script: $(Build.SourcesDirectory)/src/coreclr/build-runtime$(scriptExt) $(buildConfig) $(archType) -ci -component alljits -component spmi
Copy link
Member

Choose a reason for hiding this comment

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

I'm glad you did this, but I thought we only did it because we ended up with larger-than-MAX_PATH paths if we didn't, in the CI. Maybe that's fixed?

Copy link
Member Author

Choose a reason for hiding this comment

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

When we separated out the test build script from the regular CoreCLR runtime build script, we removed the last usages of this environment variable.

I think we require long-paths to be enabled now, so we don't hit any issues in CI with larger-than-MAX_PATH paths.

@jkoritzinsky
Copy link
Member Author

I'll make sure to add some docs about this for people who want to run the build-runtime scripts manually before I merge.

@jkoritzinsky
Copy link
Member Author

I've added some docs to the cross-building document about building the cross-targeting tools.

Copy link
Member

@hoyosjs hoyosjs left a comment

Choose a reason for hiding this comment

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

Other than that it LGTM

Just for whoever lands here, the crossdac basically now builds via:

src/coreclr/build-runtime.cmd -<arch> -os linux -hostarch x64 -component crosscomponents -cmakeargs "-DCLR_CROSS_COMPONENTS_BUILD=1"

PgoInstrument=false;
NoPgoOptimize=true;
CrossBuild=false;
CMakeArgs=$(CMakeArgs) -DCLR_CROSS_COMPONENTS_BUILD=1"
Copy link
Member

Choose a reason for hiding this comment

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

Is CLR_CROSS_COMPONENTS_BUILD cmake def something that's better to add in the cmd or in runtime.proj? I don't see it being a thing for the subset infrastructure. I think it's also missing in the cross-building part of the docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think adding it out here is okay (I'd like to get rid of it, but I don't want to fight CMake for that). I'll update the docs.

@jkoritzinsky
Copy link
Member Author

Windows test failure is #48798.

iOS and tvOS failures are due to a networking issue pulling down the provisioning profile. I'd link the issue number, but I can't seem to find it.

@jkoritzinsky jkoritzinsky merged commit 7d562f9 into dotnet:main Apr 1, 2022
@jkoritzinsky jkoritzinsky deleted the cross-comp-build-independence branch April 1, 2022 17:08
@ghost ghost locked as resolved and limited conversation to collaborators May 1, 2022
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.

3 participants