-
Notifications
You must be signed in to change notification settings - Fork 196
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
In VMR builds only build the target RID and don't rebuild #10017
Conversation
…sh (dotnet#9925)" (dotnet#10014) This reverts commit c5c96a2.
…ding on a vertical build (where we won't have the other AppHost RIDs anyway). Also re-enable building the language servers in VMR non-sourcebuild builds.
ccb20e7
to
85f105d
Compare
…tifier doesn't seem like it wants to work nicely.
cc: @mmitche any ideas why this PR would be hitting a source-build error with what looks like the inner clone? |
Odd...it's recursively cloning and going on forever. @dotnet/source-build-internal |
@@ -12,6 +12,22 @@ | |||
<RemoveDevicePlatformSupport>true</RemoveDevicePlatformSupport> | |||
</PropertyGroup> | |||
|
|||
<!-- | |||
In a vertical build, we'll only publish for the RID of the vertical. | |||
In a non-vertical build, we'll publish for alll RIDs of the OS we are building on (to reduce the number of CI jobs). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a non-vertical build, we'll publish for alll RIDs of the OS we are building on (to reduce the number of CI jobs). | |
In a non-vertical build, we'll publish for all RIDs of the OS we are building on (to reduce the number of CI jobs). |
|
||
<PropertyGroup Condition="'$(DotNetBuild)' == 'true'"> | ||
<RuntimeIdentifiers>$(TargetRid)</RuntimeIdentifiers> | ||
<RuntimeIdentifier>$(TargetRid)</RuntimeIdentifier> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to set both RuntimeIdentifiers (plural) and RuntimeIdentifier (singular)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use RuntimeIdentifiers
below to create the "publish" job (for both VMR and non-VMR builds) and the SDK uses RuntimeIdentifier
to determine which RID to create the apphost for (which we need for the correct apphost architecture in the VMR build).
Looking at sequence of events:
It looks like that last action re-runs |
@ellahathaway - Where are you getting that sequence from? I don't see any binlog attached to these builds. |
I had to build locally in order to get the outer binlog (unable to get inner binlog due to infinite clone). This is the binlog from my local build. |
Confirmed that removing this change fixed the infinite clone issue. Working to determine why this change that would cause the infinite looping. |
@jkoritzinsky Is the intention to pass this only to the inner build, or just generally to the repo build? If it's the latter, then this should just go into the VMR's build args for razor. |
I wanted to make sure it's propagated to the inner build from the outer build (as the inner build now needs it). If we can get the property value to the inner build without adding that line, we can do that instead. |
The inner build takes the outer build command line and adds additional arguments. So if it doesn't adversely affect the outer build, then you can pass it through at the outer build level. |
@mmitche just to make sure I understand, does that mean that I can skip passing it here and only pass it in dotnet/installer#18843? I just want to make sure I get it right with all of the layers. |
Yes. It will need to be passed at that layer no matter what because TargetRid properties aren't ambiently visible to the repos. They need to be passed explicitly. So if it gets passed at the top level it should appear in the inner build args. |
I don't have permission to merge here, can someone else merge this in? |
@ryzngard as the resident expert, can you test the build output from https://dev.azure.com/dnceng/internal/_build/results?buildId=2394804&view=results and verify this hasn't regressed VS Code again. |
Validated the osx-arm64 build runs on osx with an m1 chip |
Reapply #9925 but only do it in VMR builds. Additionally, only build the vertical's RID in a vertical build as we won't be able to restore the other apphost packs anyway.