Skip to content

Conversation

@trylek
Copy link
Member

@trylek trylek commented Mar 2, 2021

Fixes: #48252
/cc @dotnet/crossgen-contrib

@ViktorHofer
Copy link
Member

Do we need to wait until the minimum required version of the SDK is P2? When using P1 (which is the current min version), would the sfxproj then fail to build?

@trylek
Copy link
Member Author

trylek commented Mar 2, 2021

@ViktorHofer - yes, the sfxproj would fail to build now as P1 SDK doesn't support DotNetHostPath argument, that was the main reason for the workaround in the first place. What is the scenario that requires building runtime using P1 SDK after you bumped up its version to P2 in the March infra rollout?

@ViktorHofer
Copy link
Member

What is the scenario that requires building runtime using P1 SDK after you bumped up its version to P2 in the March infra rollout?

The March infra rollout bumped the minimum required version of the SDK to 6.0 Preview 1: 3e31241. The target version is currently a nightly preview 2 build, because we needed a newer SDK change in CI (for linker tests).

MSBuild respects and checks the minimum required SDK version but doesn't know anything about the target version in global.json as that's something Arcade custom. If you merge this change, a dotnet build src/installer/pkg/sfx/Microsoft.NETCore.App/Microsoft.NETCore.App.Runtime.sfxproj will fail without a corresponding SDK installed (>= 6.0 P2). Don't know how common it is to directly build the sfxprojs but it's something that would regress.

@ViktorHofer
Copy link
Member

I recommend to hold this PR off until 4/5 (first Monday of April) when we will update to the next publicly available SDK which I believe will be Preview 2.

@trylek trylek added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 2, 2021
@trylek
Copy link
Member Author

trylek commented Mar 2, 2021

OK, I have marked it with the no-merge label; for now I assume this is going to be merged in as part of or after the April infra rollout per your suggestion.

@ViktorHofer
Copy link
Member

Thanks for your understanding 👍

@mangod9
Copy link
Member

mangod9 commented Mar 29, 2021

Is the preview2 sdk integrated into runtime now, or will that happen with the April rollout.

@trylek
Copy link
Member Author

trylek commented Mar 29, 2021

As @ViktorHofer explained to me, in practice we're already consuming SDK Preview2 in the runtime repo but technically the runtime repo still supports SDK Preview1. For this reason I plan to rebase and merge this in after Viktor completes the April rollout next Monday (or maybe Tuesday considering next Monday is most likely national holiday in Austria just like in Czechia).

@ViktorHofer
Copy link
Member

The PR is now unblocked as we just updated the SDK to 6.0 Preview 2: #50084.

@trylek trylek force-pushed the CG2InstallerCleanup branch from 90644b5 to 58545ea Compare April 6, 2021 17:55
@trylek trylek removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 6, 2021
@trylek
Copy link
Member Author

trylek commented Apr 6, 2021

I have rebased the change and I plan to merge it in once testing successfully completes as the change has already been reviewed unless anyone objects.

@trylek
Copy link
Member Author

trylek commented Apr 6, 2021

I have rebased the change but now it's hitting a new error I didn't see previously; @jkoritzinsky, could you please take a quick look at the error message and advise me what else needs changing or what most likely broke it as I guess you'll see that off the top of your head? I believe the framework gets compiled successfully, this error happens when we compile the framework for the second time as part of CG2 publishing. Thank you in advance!

D:\git\runtime5\.dotnet\sdk\6.0.100-preview.2.21155.3\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Publish.targets(404,5): error : Error: [D:\git\runtime5\src\installer\pkg\sfx\Microsoft.NETCore.App\Microsoft.NETCore.App.Crossgen2.sfxproj]
D:\git\runtime5\.dotnet\sdk\6.0.100-preview.2.21155.3\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Publish.targets(404,5): error :   An assembly specified in the application dependencies manifest (crossgen2.deps.json) has already been found but with a different file extension: [D:\git\runtime5\src\installer\pkg\sfx\Microsoft.NETCore.App\Microsoft.NETCore.App.Crossgen2.sfxproj]
D:\git\runtime5\.dotnet\sdk\6.0.100-preview.2.21155.3\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Publish.targets(404,5): error :     package: 'crossgen2', version: '6.0.0-dev' [D:\git\runtime5\src\installer\pkg\sfx\Microsoft.NETCore.App\Microsoft.NETCore.App.Crossgen2.sfxproj]
D:\git\runtime5\.dotnet\sdk\6.0.100-preview.2.21155.3\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Publish.targets(404,5): error :     path: 'crossgen2.dll' [D:\git\runtime5\src\installer\pkg\sfx\Microsoft.NETCore.App\Microsoft.NETCore.App.Crossgen2.sfxproj]
D:\git\runtime5\.dotnet\sdk\6.0.100-preview.2.21155.3\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Publish.targets(404,5): error :     previously found assembly: 'D:\git\runtime5\artifacts\bin\coreclr\windows.x64.Release\crossgen2\crossgen2.exe' [D:\git\runtime5\src\installer\pkg\sfx\Microsoft.NETCore.App\Microsoft.NETCore.App.Crossgen2.sfxproj]
D:\git\runtime5\.dotnet\sdk\6.0.100-preview.2.21155.3\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Publish.targets(404,5): error : Error: [D:\git\runtime5\src\installer\pkg\sfx\Microsoft.NETCore.App\Microsoft.NETCore.App.Crossgen2.sfxproj]

trylek added 2 commits April 15, 2021 08:50
In my previous change I missed the fact that, once we run
Crossgen2 through "dotnet", we need to pass in the dll, not the
exe. That allowed for an additional bit of cleanup in the script.

Thanks

Tomas
@trylek trylek force-pushed the CG2InstallerCleanup branch from 1f9215c to 6d4096e Compare April 15, 2021 06:51
@trylek trylek merged commit e646d7a into dotnet:main Apr 15, 2021
@trylek trylek deleted the CG2InstallerCleanup branch April 15, 2021 17:10
@ghost ghost locked as resolved and limited conversation to collaborators May 15, 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.

Clean up ReadyToRun.targets in the installer after consuming SDK repo update with CG2 execution fixes

4 participants