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

Reorganize CoreCLR native build to reduce CMake reconfigures when the build system is untouched #49906

Merged
merged 21 commits into from
Apr 5, 2021

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Mar 19, 2021

Part of April's batched infrastructure rollout: #49889

Move the CoreCLR native subsets support to use custom CMake targets instead of additional subset properties. Now, cmake will always configure the whole CoreCLR native project, but will only build the required subsets. This enables the generated CMake build system to be reused between build invocations even if for example the developer first builds the clr.runtime subset and then builds the clr.alljits subset.

Since CMake already supports rerunning itself during the build when it detects changes in CMake files, this PR also adds caching of the CMake command line options that can change between build invocations for a given arch/config so that we don't do a CMake reconfigure if the results will be the same as the last invocation or if CMake can regenerate the build system itself with its up-to-date check. This is only done on builds that are not "configure only" and do not use the VS generator (CMake-generated VS projects lack the auto-regeneration feature that makes this work).

This PR also adds a clr.native subset that builds all of the CoreCLR native code since apparently we didn't have a subset that did that after clr.runtime and clr.alljits was introduced.

Changes to the build-runtime.cmd/sh arguments

The -skipruntime, -skipjit, -skipalljits, -paltests, and -skipiltools arguments have been removed. Instead of using an opt-out system, the command line uses an opt-in system with a new -component <componentName> argument which can be specified multiple times. The following components are supported:

  • -component runtime
  • -component jit
  • -component alljits
  • -component paltests
  • -component iltools

If no -component argument is provided, then all native components are built.

@ghost
Copy link

ghost commented Mar 19, 2021

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

Issue Details

Move the CoreCLR native subsets support to use custom CMake targets instead of additional subset properties. Now, cmake will always configure the whole CoreCLR native project, but will only build the required subsets. This enables the generated CMake build system to be reused between build invocations even if for example the developer first builds the clr.runtime subset and then builds the clr.alljits subset.

Since CMake already supports rerunning itself during the build when it detects changes in CMake files, this PR also adds caching of the CMake command line options that can change between build invocations for a given arch/config so that we don't do a CMake reconfigure if the results will be the same as the last invocation or if CMake can regenerate the build system itself with its up-to-date check. This is only done on builds that are not "configure only" and do not use the VS generator (CMake-generated VS projects lack the auto-regeneration feature that makes this work).

This PR also adds a clr.native subset that builds all of the CoreCLR native code since apparently we didn't have a subset that did that after clr.runtime and clr.alljits was introduced.

Author: jkoritzinsky
Assignees: -
Labels:

area-Infrastructure-coreclr

Milestone: -

Add a -component flag to the native build scripts to control which components are built. Remove the old -skipX flags and use the component flags instead.

Move symbol stripping for CoreCLR out of the install step and into the add_library/executable step to ensure we only strip once when a file can be installed for multiple components.

Fix specifying all subsets.

mark corerun and coreshim as runtime components.

Move cross-component build targets onto the componentized plan.

Remove _install function since the check it does is no longer needed.

Implicitly add dependencies for runtime/all_jits metatargets.

Move paltests to the component plan.

Fixes for unix platform builds

Add clr.native subset to build the full clr project.

Don't build pal tests as part of the default build.

Make targets without a specific component automatically be built by any non-crosscomponent subcomponent.

Move libraries CMake build to stop using environment variables.

Don't rerun Cmake configure if the command line hasn't changed since the last run. Saves about 15 seconds on a no-op incremental build. Non-Windows impl still tbd

Add iltools subset.

Since we're at a point where combinatorial explosion makes it untenable to support combinatorial targets, use the multiple-targets feature of CMake when available (thankfully Windows is included in this set) and otherwise use the underlying build tool.

Disable command line up-to-date check for VS.

Implement command-line check for cmake reconfigure for Unix platforms as well.
@jkoritzinsky jkoritzinsky force-pushed the single-configure-coreclr-cmake branch from f41baf5 to 35d617f Compare March 19, 2021 21:15
@jkoritzinsky
Copy link
Member Author

Test failure is rare intermittent failure. Tracked by #50139

add_subdirectory(${CLR_SRC_NATIVE_DIR}/corehost/apphost/static Corehost.Static)
add_dependencies(runtime singlefilehost)
Copy link
Member

Choose a reason for hiding this comment

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

This dependency seems reverse.

Copy link
Member Author

Choose a reason for hiding this comment

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

We want to build the "singlefilehost" as part of the "runtime" component. Otherwise we'd build it either as its own component (which no one will remember to specify) or as part of the fallback component (which would make all components build it, which is undesirable).

Copy link
Member

Choose a reason for hiding this comment

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

This seems fine for now. Eventually I'd prefer we specify dependencies on the top level projects and people use subsets less. That's something we can address when we're ready.

src/coreclr/CMakeLists.txt Outdated Show resolved Hide resolved
endif(CLR_CMAKE_HOST_UNIX)

_install (PROGRAMS $<TARGET_FILE:superpmi-shim-collector> DESTINATION .)
install_clr(TARGETS superpmi-shim-collector DESTINATIONS .)
Copy link
Member

Choose a reason for hiding this comment

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

These changes from PROGRAMS to TARGETS are a problem. I've recently needed to switch from TARGETS to PROGRAMS to fix macOS arm64 signature corruption issues. Please see #46452 description for the reasons.

Copy link
Member Author

Choose a reason for hiding this comment

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

install_clr uses PROGRAMS internally, so we should be good.

Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

LGTM, a few minor comments, mostly around the build-runtime scripts

@@ -206,6 +206,27 @@ function(compile_asm)
set(${COMPILE_ASM_OUTPUT_OBJECTS} ${ASSEMBLED_OBJECTS} PARENT_SCOPE)
endfunction()

# add_component(componentName [targetName] [EXCLUDE_FROM_ALL])
function(add_component componentName)
if (${ARGC} GREATER 2 OR ${ARGC} EQUAL 2)
Copy link
Member

Choose a reason for hiding this comment

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

lol there's no >= operator?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not in CMake 3.6, which we still need to support for Linux source build.

add_subdirectory(${CLR_SRC_NATIVE_DIR}/corehost/apphost/static Corehost.Static)
add_dependencies(runtime singlefilehost)
Copy link
Member

Choose a reason for hiding this comment

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

This seems fine for now. Eventually I'd prefer we specify dependencies on the top level projects and people use subsets less. That's something we can address when we're ready.

src/coreclr/build-runtime.sh Outdated Show resolved Hide resolved
usage_list+=("-skipiltools: skip building IL tools.")
usage_list+=("-paltests: build the pal tests.")
usage_list+=("-staticanalyzer: use scan_build static analyzer.")
usage_list+=("-component: Build individual components instead of the full project. Available options are 'jit', 'runtime', 'paltests', 'alljits', and 'iltools'. Can be specified multiple times.")
Copy link
Member

Choose a reason for hiding this comment

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

Do we still consider the build scripts as dev entry points?

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 know the JIT team still uses these scripts directly sometimes, so I wanted to keep them up to date.

add_library(corguids INTERFACE)
target_sources(corguids INTERFACE $<TARGET_OBJECTS:corguids_obj>)

# Binplace the inc files for packaging later.

_install (FILES cfi.h
Copy link
Member

Choose a reason for hiding this comment

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

Do file installs not need a component? Is this under unknown?

Copy link
Member Author

Choose a reason for hiding this comment

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

They'll fall under the coreclr_misc subset and be installed when any component (other that iltools or jit) is built.

@ghost
Copy link

ghost commented Apr 5, 2021

Hello @jkoritzinsky!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@jkoritzinsky
Copy link
Member Author

Mono CrossAOT build failure tracked by #50753

@jkoritzinsky jkoritzinsky merged commit 8d6cd81 into dotnet:main Apr 5, 2021
@jkoritzinsky jkoritzinsky deleted the single-configure-coreclr-cmake branch April 5, 2021 22:05
thaystg added a commit to thaystg/runtime that referenced this pull request Apr 6, 2021
…shim_mono

# By Aaron Robinson (10) and others
# Via GitHub
* upstream/main: (108 commits)
  [mbr] Add Apple sample (dotnet#50740)
  make EstablishProxyTunnelAsync throw on failure status code from proxy (dotnet#50763)
  Improve RGB Min Max evaluation performance by using 2 or 3 comparison… (dotnet#50622)
  [mono] More domain cleanups (dotnet#50479)
  Fix Crossgen2 of PlatformDefaultMemberFunction methods and calls. (dotnet#50754)
  Disable EventSource generator in design-time builds (dotnet#50741)
  Fix X509 test failures on Android (dotnet#50301)
  Do not confuse fgDispBasicBlocks in fgMorphBlocks (dotnet#50703)
  Enforce 64KB event payload size limit on EventPipe  (dotnet#50600)
  Reorganize CoreCLR native build to reduce CMake reconfigures when the build system is untouched (dotnet#49906)
  [mbr] Turn on hot reload for iOS, tvOS and MacCatalyst (dotnet#50458)
  improve connection scavenge logic by doing zero-byte read (dotnet#50545)
  Resolve call mdtokens when making tier 1 inline observations (dotnet#50675)
  Annotate APIs in System.Private.Xml (dotnet#49682)
  Support compiling against OpenSSL 3 headers
  Change Configuration.Json to use a regular Dictionary. (dotnet#50611)
  Remove unused BigNumFromBinary P/Invoke (dotnet#50670)
  Make Ninja the default CMake generator on Windows for the repo (dotnet#49715)
  [AppleAppBuilder] Entitlements to run tests on catalyst using the JIT (dotnet#50637)
  [mono] Fix delegate invokes to dynamic methods in mixed mode. (dotnet#50547)
  ...

# Conflicts:
#	src/mono/dlls/mscordbi/CMakeLists.txt
BruceForstall added a commit to BruceForstall/runtime that referenced this pull request Apr 7, 2021
@ghost ghost locked as resolved and limited conversation to collaborators May 9, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
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.

6 participants