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

Move optdata and version file generation up to MSBuild from build-runtime #58674

Merged
merged 47 commits into from
Oct 27, 2021

Conversation

jkoritzinsky
Copy link
Member

Move optdata and version file generation from the native build scripts up into the managed scripts. The native build scripts will now by default copy a fallback version file in place if the version files do not exist and will disable PGO if no pgo file path is passed in to the build-runtime scripts.

This removes all cases of build-runtime calling into MSBuild, which removes the last MSBuild-in-Batch/Bash-in-MSBuild process nesting we have in the product build (there's still some in the test build).

This PR also renames the clr.dactools subset to clr.nativeprereqs to reflect its expanded role.

…s up into the managed scripts. The native build scripts will now by default copy a fallback version file in place if the version files do not exist and will disable PGO if no pgo file path is passed in to the build-runtime scripts.

This removes all cases of build-runtime calling into MSBuild.
…uild-runtime no longer calls into any MSBuild processes.
@ghost
Copy link

ghost commented Sep 4, 2021

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

Issue Details

Move optdata and version file generation from the native build scripts up into the managed scripts. The native build scripts will now by default copy a fallback version file in place if the version files do not exist and will disable PGO if no pgo file path is passed in to the build-runtime scripts.

This removes all cases of build-runtime calling into MSBuild, which removes the last MSBuild-in-Batch/Bash-in-MSBuild process nesting we have in the product build (there's still some in the test build).

This PR also renames the clr.dactools subset to clr.nativeprereqs to reflect its expanded role.

Author: jkoritzinsky
Assignees: -
Labels:

area-Infrastructure-coreclr

Milestone: -

@jkoritzinsky
Copy link
Member Author

cc: @jkotas this is the build change we talked about earlier today.

@agocke This PR should have an impact on no-op incremental build times. Once I get it green and have CI working correctly, I'll get some numbers for you.

Comment on lines +184 to +186
# Build/Generate native prerequisites
- script: $(Build.SourcesDirectory)$(dir)build$(scriptExt) -subset clr.nativeprereqs $(crossArg) -arch $(archType) $(osArg) -c $(buildConfig) $(officialBuildIdArg) -ci /bl:$(Build.SourcesDirectory)artifacts/log/$(buildConfig)/CoreCLRNativePrereqs.binlog
displayName: Build and generate native prerequisites
Copy link
Member

Choose a reason for hiding this comment

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

Why not use the common msbuild entry point to build the CoreCLR runtime and by that avoid the msbuild invocation here and the extra AzDO specific OutputPgoPathForCI target?

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 spoke with @jkotas before doing this work and he said that he still doesn't want us to call into CMake/Ninja from MSBuild if we can avoid it.

Copy link
Member

Choose a reason for hiding this comment

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

Sharing my thoughts from my conversation with Jeremy:

  • Designs where one build driver (msbuild) is wrapping another build driver (ninja) are very hard to debug.
  • I think it is important to have a reasonable way to build the unmanaged runtime without a working managed runtime for bootstrapping on new platforms.
  • Things like version files are non-essential and it is easy to replace them with placeholder files. I do not see a problem with using C# to generate them. I would actually prefer to use C# to generate them. It would be nice to get rid of the python dependency.

Copy link
Member

Choose a reason for hiding this comment

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

* It would be nice to get rid of the python dependency.

For eventpipe generator as well (can be replaced with C++ shim/macros as done for numa and openssl shims). We could also try to get rid of perl dependency, we have some headers generators still written in perl (which not many folks use in this day and age).. 🙂

jkoritzinsky and others added 4 commits September 8, 2021 09:55
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
src/coreclr/CMakeLists.txt Outdated Show resolved Hide resolved
src/coreclr/build-runtime.cmd Outdated Show resolved Hide resolved
src/coreclr/dlls/mscordac/CMakeLists.txt Outdated Show resolved Hide resolved
eng/nativepgo.targets Outdated Show resolved Hide resolved
src/coreclr/runtime.proj Show resolved Hide resolved
@jkoritzinsky
Copy link
Member Author

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

src/mono/CMakeLists.txt Show resolved Hide resolved
eng/nativepgo.targets Show resolved Hide resolved
jkoritzinsky and others added 2 commits October 19, 2021 15:38
Co-authored-by: Juan Hoyos <19413848+hoyosjs@users.noreply.github.com>
@hoyosjs hoyosjs merged commit 4682098 into dotnet:main Oct 27, 2021
@hoyosjs
Copy link
Member

hoyosjs commented Oct 27, 2021

Thanks for this @jkoritzinsky

@jkoritzinsky jkoritzinsky deleted the no-msbuild-in-build-runtime branch October 27, 2021 17:05
@BruceForstall
Copy link
Member

@jkoritzinsky It looks like this breaks the JIT rolling build, e.g.:

C:\gh\runtime4\src\coreclr\utilcode\utilmessagebox.cpp(205): error C2220: the following warning is treated as an error
C:\gh\runtime4\src\coreclr\utilcode\utilmessagebox.cpp(205): warning C4003: not enough arguments for function-like macro invocation 'QUOTE_MACRO_L_HELPER'
C:\gh\runtime4\src\coreclr\utilcode\utilmessagebox.cpp(205): error C2065: 'L': undeclared identifier

https://dev.azure.com/dnceng/internal/_build?definitionId=902

which is built in a clean enlistment with:

src\coreclr\build-runtime.cmd x64 checked -component alljits

cc @dotnet/jit-contrib

@jkoritzinsky
Copy link
Member Author

@BruceForstall I'll put out a PR to generate the version files for that pipeline to unblock it. Once that's out, I'll investigate what part of the version fallback doesn't work.

@BruceForstall
Copy link
Member

Thanks. btw, it's trivial to repro this on a clean enlistment (no artifacts folder) by running that build command.

@jkoritzinsky
Copy link
Member Author

jkoritzinsky commented Oct 28, 2021

#60981 should have a fix. Is there a public pipeline I can run to validate the fix or are all of the affected pipelines internal-only?

@BruceForstall
Copy link
Member

You can trigger the "runtime-coreclr superpmi-replay" pipeline, which also uses build-jit-job.yml

@BruceForstall
Copy link
Member

That's a 2+ hour pipeline, but the build is a pre-req that happens quickly, and first.

@BruceForstall
Copy link
Member

So is the model that you can't call build-runtime.cmd on a "raw" enlistment anymore?

Should the pipeline invoke the root build.cmd, e.g.,

build.cmd -c Checked -arch x64 -subset clr.alljits

instead?

@BruceForstall
Copy link
Member

Doing that does a few things (dotnet CLI install, wasm restores?) before invoking "C:\gh\runtime4\src\coreclr\build-runtime.cmd" -x64 -checked -component alljits. It notably doesn't output anything about building version files (and I don't think we generate any log files anymore, so you can't actually see individual steps that are performed), but it works.

@hoyosjs
Copy link
Member

hoyosjs commented Oct 28, 2021

build-runtime.cmd won't work on a raw enlistment if you're trying to do a full native build. It should work for the jits though. @jkoritzinsky shouldn't this pass in that case? Just copy the phony files and skip PGO?

@jkoritzinsky
Copy link
Member Author

Yes, but it looks like there's a bit with the fallback files that doesn't work quite right that I missed in my testing.

@hoyosjs
Copy link
Member

hoyosjs commented Oct 28, 2021

I see https://github.com/dotnet/runtime/blob/main/eng/native/version/runtime_version.h

RuntimeProductVersion is blank @jkoritzinsky was this intentional? It's supposed to be version. We could just have it be the 42.42.42... thing that arcade uses

@ghost ghost locked as resolved and limited conversation to collaborators Nov 27, 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