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

Retry publish crossgen as AOT (#65948) #67636

Merged
merged 43 commits into from
Apr 26, 2022
Merged

Conversation

agocke
Copy link
Member

@agocke agocke commented Apr 6, 2022

Trying to work through the problems with source build

…5948)

Publishes crossgen as an AOT binary on Windows+Linux x64+ARM64, otherwise publishes as an R2R single file.

Closes dotnet#60016

(cherry picked from commit 0d1e04b)
@agocke
Copy link
Member Author

agocke commented Apr 6, 2022

With much help from @hoyosjs, @carlossanlop, and @krwq I think I have figured this out: the problem is that sometimes the crossgen2 package was restored as net6.0 and sometimes as net7.0. I can't identify from the logs where the net7.0 was coming from, but I think I can pass it down from the package build, which should hopefully ensure that it's always 6.0 at the time it matters.

@agocke
Copy link
Member Author

agocke commented Apr 10, 2022

So, digging in more, I'm more confused about what's going on here. I wonder if there's something I don't understand about MSBuild static graph restore.

@rainersigwald Any chance you can provide any insight here? The problem is that the project.assets.json does not seem to consistently get a RID included. It has net6.0 instead of net6.0/centos.7-x64.

The build I'm having trouble with is at https://dev.azure.com/dnceng/public/_build/results?buildId=1709594&view=logs&j=000ba8e3-34f8-51d2-8d06-9b61f10256d0&t=ca9b6e26-8c16-57f9-673e-2bc0dc655f25

The important part of the log is

Microsoft.NETCore.App.Crossgen2 -> 
    Deleting project.assets.json
    Continuing with RID centos.7-x64
    Determining projects to restore...
    Restored /__w/1/s/artifacts/source-build/self/src/src/coreclr/tools/aot/ILCompiler.Diagnostics/ILCompiler.Diagnostics.csproj (in 270 ms).
    Restored /__w/1/s/artifacts/source-build/self/src/src/coreclr/tools/aot/crossgen2/crossgen2.csproj (in 269 ms).
    Restored /__w/1/s/artifacts/source-build/self/src/src/coreclr/tools/aot/ILCompiler.TypeSystem/ILCompiler.TypeSystem.csproj (in 269 ms).
    Restored /__w/1/s/artifacts/source-build/self/src/src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj (in 269 ms).
    Restored /__w/1/s/artifacts/source-build/self/src/src/coreclr/tools/aot/ILCompiler.DependencyAnalysisFramework/ILCompiler.DependencyAnalysisFramework.csproj (in 29 ms).
    FileContents: /__w/1/s/artifacts/source-build/self/src/artifacts/obj/coreclr/crossgen2/project.assets.json
    {
    "version": 3,
    "targets": {
    "net6.0": {
    "Microsoft.Build.Tasks.Git/1.2.0-beta-22174-02": {
    "type": "package",
...

The important bits are

    Deleting project.assets.json
    Continuing with RID centos.7-x64

and

   "targets": {
    "net6.0": {

I don't understand why that is not "targets": { "net6.0/centos.7-x64". The code here is basically a straightforward MSBuild invocation of the target project:
https://github.com/dotnet/runtime/pull/67636/files#diff-115c40d5f6cdfb8ebea1490c13581e76b60c3430cbf13f45c270fc5ec383daffR36-R45

The RuntimeIdentifier should be a global property, and I don't understand why it's not factoring into the restore.

@agocke
Copy link
Member Author

agocke commented Apr 11, 2022

also + @dsplaisted in case you have any ideas or can point me in the right direction

@dsplaisted
Copy link
Member

Even if a RuntimeIdentifier is specified, the assets file will still have a RID-less "targets" entry in it, and it will be the first one. Later on in the file there should also be a target for "net6.0/centos.7-x64".

   "targets": {
    "net6.0": {

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.

Just a few remaining questions 👍

<_TargetsToRun>Restore;Publish;PublishItemsOutputGroup</_TargetsToRun>
</PropertyGroup>

<MSBuild Projects="$(RepoRoot)src/coreclr/tools/aot/crossgen2/crossgen2.csproj"
Copy link
Member

@ViktorHofer ViktorHofer Apr 22, 2022

Choose a reason for hiding this comment

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

Why can't this be a ProjectReference instead? ProjectReferences allow to specify additional properties and target(s) to invoke. They also allow to capture their output.

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 had no idea. I'd rather do that in a follow-up PR, since this is a very important deliverable for .NET 7 and I'd like to get dogfooding feedback as soon as possible.

agocke and others added 3 commits April 22, 2022 13:29
…App.Crossgen2.sfxproj

Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
@agocke
Copy link
Member Author

agocke commented Apr 25, 2022

@ViktorHofer Anything else high-priority? I've opened an issue for follow-up work.

@agocke
Copy link
Member Author

agocke commented Apr 25, 2022

@jkoritzinsky Does this new strategy also look OK to you? I don't think much has changed except including the RID in the list of RuntimeIdentifiers in the first restore, so even if the assets.json is cached, it will still have compatible content.

@agocke agocke requested review from a team and ViktorHofer April 26, 2022 05:20
…App.Crossgen2.sfxproj

Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
@agocke
Copy link
Member Author

agocke commented Apr 26, 2022

@ViktorHofer Done, anything else?

@agocke agocke requested a review from a team April 26, 2022 19:07
@ViktorHofer
Copy link
Member

I do have a lot more questions on why we need to restore that project separately outside of the upfront static graph repo restore but I don't think they should block this PR to going in.

Just wanted to make sure that you understand that a lazy just-in-time restore is the thing that eventually causes a build to fail in the middle. I saw such cases with the libraries linker tests which do something similar. But we can discuss that in your follow-up issue, if you want to :)

@agocke agocke merged commit 7ff3ac6 into dotnet:main Apr 26, 2022
@agocke agocke deleted the crossgen2-publish branch April 26, 2022 23:56
@agocke
Copy link
Member Author

agocke commented Apr 26, 2022

Makes sense. First thing in the follow-up can be to remove the restore. It may no longer be necessary.

@ghost ghost locked as resolved and limited conversation to collaborators May 31, 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.

5 participants