Ensure that the correct VC++ toolset is selected in all build environments by eiriktsarpalis · Pull Request #104763 · dotnet/runtime

eiriktsarpalis · 2024-07-11T22:05:46Z

Follow-up on #104695, this PR tries to set the correct build architectures for all configurations. The changes result in cross-compilation failing in the gen-buildsys.cmd stage, figured I'd post the PR for group troubleshooting.

Fix #104674. Fix #76516.

dotnet-policy-service · 2024-07-11T22:07:06Z

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

eng/native/init-vs-env.cmd

src/coreclr/build-runtime.cmd

Co-authored-by: Jan Kotas <[email protected]>

src/coreclr/build-runtime.cmd

MichalStrehovsky

Looks great, thanks. I think this is also fixing #76516!

am11 · 2024-07-12T12:48:59Z

Same change in src/coreclr/nativeaot/BuildIntegration/findvcvarsall.bat?

eiriktsarpalis · 2024-07-12T13:05:22Z

Same change in src/coreclr/nativeaot/BuildIntegration/findvcvarsall.bat?

Where is it being used?

am11 · 2024-07-12T13:06:50Z

Where is it being used?

dotnet publish -p:PublishAot=true --runtime win-arm64 etc.

eiriktsarpalis · 2024-07-12T13:09:32Z

Are you suggesting that the script is being used by the product? (rather than building the product?)

MichalStrehovsky · 2024-07-12T13:10:46Z

Same change in src/coreclr/nativeaot/BuildIntegration/findvcvarsall.bat?

Where is it being used?

This is tracked in #76079.

If you'd like to volunteer your time for that one too, we'd need to make the change and then run this on a arm64 machine to validate (I assume both build.cmd will build arm64 flavors):

build.cmd clr.aot+libs -c Release
src\tests\build.cmd nativeaot release tree nativeaot
src\tests\run.cmd runnativeaottests release

It should be an easy fix, we've just not done it because Microsoft doesn't want to spend money on giving me an ARM64 machine and I will not spend my mental health on using an ARM64 machine on a different continent interactively.

It doesn't have to be part of this PR, it's not an infra change, it's a product change.

am11 · 2024-07-12T13:12:57Z

I have win-arm64 VM running on osx-arm64, I can take a look based on @eiriktsarpalis changes (unless Eirik beats me to it 😄).

eiriktsarpalis · 2024-07-12T13:15:35Z

I expect some of the assumptions we're making for the build (e.g. #104763 (comment)) might be different in the context of the product, so we should keep the changes in separate PRs. @am11 feel free to have a go at this.

eiriktsarpalis · 2024-07-12T13:29:06Z

@am11 what certainly seems to be the case is that we should be adding provision for 32-bit machines that this PR is skipping.

am11 · 2024-07-12T14:15:54Z

BuildIntegration script is used by both, product build as well as shipped in ILCompiler nuget package for windows. Quick testing suggests this should do the trick: am11@33ce9b3.

jkotas · 2024-07-12T14:25:10Z

BuildIntegration script is used by both, product build as well as shipped in ILCompiler nuget package for windows. Quick testing suggests this should do the trick: am11@33ce9b3.

I do not think we need to bother with 32-bit Windows x86 machines. Those machines do not really exist anymore as dev machines and Visual Studio (that is a pre-req for naor publish) requires 64-bit. I would keep it simple and always use 64-bit tools, same as what we do in the repo build.

eng/native/init-vs-env.cmd

am11 · 2024-07-12T14:37:35Z

I would keep it simple and always use 64-bit tools, same as what we do in the repo build.

main...am11:patch-5

I think -requires Microsoft.VisualStudio.Component.VC.Tools.%toolsSuffix% is something we should use with vswhere in other places as well (instead of hardcoded x86.x64).

Co-authored-by: Jan Kotas <[email protected]>

This reverts commit a1b95c1.

hez2010 · 2024-07-12T14:53:21Z

Can you also include a fix to

runtime/eng/native/configurecompiler.cmake

Line 999 in f9eda07

    
                 file(TO_CMAKE_PATH "$ENV{VCToolsInstallDir}\\bin\\HostX86\\arm64\\armasm64.exe" CMAKE_ASM_COMPILER)

before closing #104674?
It needs to use $ENV{VCToolsInstallDir}\\bin\\Hostarm64\\arm64\\armasm64.exe instead of $ENV{VCToolsInstallDir}\\bin\\HostX86\\arm64\\armasm64.exe.
And CLR_CMAKE_HOST_WIN32+CLR_CMAKE_HOST_ARCH_ARM isn't a thing now so the branch can be removed:

runtime/eng/native/configurecompiler.cmake

Lines 979 to 995 in f9eda07

    
               if(CLR_CMAKE_HOST_ARCH_ARM) 
        
                 # Explicitly specify the assembler to be used for Arm32 compile 
        
                 file(TO_CMAKE_PATH "$ENV{VCToolsInstallDir}\\bin\\HostX86\\arm\\armasm.exe" CMAKE_ASM_COMPILER) 
        
                 set(CMAKE_ASM_MASM_COMPILER ${CMAKE_ASM_COMPILER}) 
        
                 message("CMAKE_ASM_MASM_COMPILER explicitly set to: ${CMAKE_ASM_MASM_COMPILER}") 
        
                 # Enable generic assembly compilation to avoid CMake generate VS proj files that explicitly 
        
                 # use ml[64].exe as the assembler. 
        
                 enable_language(ASM) 
        
                 set(CMAKE_ASM_COMPILE_OPTIONS_MSVC_RUNTIME_LIBRARY_MultiThreaded         "") 
        
                 set(CMAKE_ASM_COMPILE_OPTIONS_MSVC_RUNTIME_LIBRARY_MultiThreadedDLL      "") 
        
                 set(CMAKE_ASM_COMPILE_OPTIONS_MSVC_RUNTIME_LIBRARY_MultiThreadedDebug    "") 
        
                 set(CMAKE_ASM_COMPILE_OPTIONS_MSVC_RUNTIME_LIBRARY_MultiThreadedDebugDLL "") 
        
                 set(CMAKE_ASM_COMPILE_OBJECT "<CMAKE_ASM_COMPILER> -g <INCLUDES> <FLAGS> -o <OBJECT> <SOURCE>")

This reverts commit 1eeadb3.

eiriktsarpalis · 2024-07-12T14:57:43Z

@hez2010 is it a question of simply updating the path or do we need to condition this on the build architecture?

hez2010 · 2024-07-12T15:08:45Z

@hez2010 is it a question of simply updating the path or do we need to condition this on the build architecture?

Simply updating the path should be okay. It has already been guarded by both CLR_CMAKE_HOST_WIN32 and CLR_CMAKE_HOST_ARCH_ARM64.

eiriktsarpalis · 2024-07-12T15:15:31Z

That would still be entered if you're cross compiling to arm64 on x64 though, right?

eiriktsarpalis · 2024-07-12T16:14:14Z

I updated the assembler logic as well, but it turns out that some branching is required for cross-compilation on x64 to keep working.

Same as #82080, but this time we have #104763 Fixes #94102.

Second attempt at fixing ARM64 cross compilation

06ddd90

ghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 11, 2024

dotnet-policy-service bot assigned eiriktsarpalis Jul 11, 2024

eiriktsarpalis requested review from ViktorHofer and jkotas July 11, 2024 22:05

eiriktsarpalis added area-Infrastructure area-Infrastructure-coreclr and removed area-Infrastructure needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 11, 2024

This was referenced Jul 12, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

The job running on agent NetCore-Public ran longer than the maximum time #104044

Closed

jkotas reviewed Jul 12, 2024

View reviewed changes

eng/native/init-vs-env.cmd Outdated Show resolved Hide resolved

jkotas reviewed Jul 12, 2024

View reviewed changes

src/coreclr/build-runtime.cmd Outdated Show resolved Hide resolved

jkotas reviewed Jul 12, 2024

View reviewed changes

src/coreclr/build-runtime.cmd Outdated Show resolved Hide resolved

jkotas reviewed Jul 12, 2024

View reviewed changes

src/coreclr/build-runtime.cmd Outdated Show resolved Hide resolved

Update eng/native/init-vs-env.cmd

5006b02

Co-authored-by: Jan Kotas <[email protected]>

MichalStrehovsky reviewed Jul 12, 2024

View reviewed changes

src/coreclr/build-runtime.cmd Outdated Show resolved Hide resolved

eiriktsarpalis added 2 commits July 12, 2024 11:50

Remove x86 branch

6437901

Revert to using __HostArch when determining toolset.

e763e4a

MichalStrehovsky approved these changes Jul 12, 2024

View reviewed changes

Address feedback.

9b9abf5

eiriktsarpalis changed the title ~~Second attempt at fixing ARM64 cross compilation~~ Ensure that the correct VC++ toolset is selected in all build environments Jul 12, 2024

ViktorHofer approved these changes Jul 12, 2024

View reviewed changes

eiriktsarpalis mentioned this pull request Jul 12, 2024

Cross compilation not working on ARM64 machines #104788

Open

eiriktsarpalis mentioned this pull request Jul 12, 2024

Make build.ps1 default to the OS architecture #104792

Merged

jkotas reviewed Jul 12, 2024

View reviewed changes

eng/native/init-vs-env.cmd Show resolved Hide resolved

jkotas reviewed Jul 12, 2024

View reviewed changes

eng/native/init-vs-env.cmd Show resolved Hide resolved

jkotas approved these changes Jul 12, 2024

View reviewed changes

eiriktsarpalis and others added 3 commits July 12, 2024 15:51

Update eng/native/init-vs-env.cmd

a1b95c1

Co-authored-by: Jan Kotas <[email protected]>

Update eng/native/init-vs-env.cmd

1eeadb3

Co-authored-by: Jan Kotas <[email protected]>

Revert "Update eng/native/init-vs-env.cmd"

39d0454

This reverts commit a1b95c1.

Revert "Update eng/native/init-vs-env.cmd"

4008fa4

This reverts commit 1eeadb3.

Set the correct arm64 assembler host

f71c847

eiriktsarpalis merged commit 369db9a into dotnet:main Jul 13, 2024

eiriktsarpalis deleted the fix-cross-compilation branch July 13, 2024 04:40

MichalStrehovsky added a commit that referenced this pull request Jul 15, 2024

Speed up native AOT test build

50edd55

Same as #82080, but this time we have #104763 Fixes #94102.

MichalStrehovsky mentioned this pull request Jul 15, 2024

Speed up native AOT test build #104880

Merged

MichalStrehovsky added a commit that referenced this pull request Jul 15, 2024

Speed up native AOT test build (#104880)

99ec785

Same as #82080, but this time we have #104763 Fixes #94102.

github-actions bot locked and limited conversation to collaborators Aug 12, 2024

Conversation

eiriktsarpalis commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dotnet-policy-service bot commented Jul 11, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichalStrehovsky left a comment

Choose a reason for hiding this comment

Uh oh!

am11 commented Jul 12, 2024

Uh oh!

eiriktsarpalis commented Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

am11 commented Jul 12, 2024

Uh oh!

eiriktsarpalis commented Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichalStrehovsky commented Jul 12, 2024

Uh oh!

am11 commented Jul 12, 2024

Uh oh!

eiriktsarpalis commented Jul 12, 2024

Uh oh!

eiriktsarpalis commented Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

am11 commented Jul 12, 2024

Uh oh!

jkotas commented Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

am11 commented Jul 12, 2024

Uh oh!

hez2010 commented Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eiriktsarpalis commented Jul 12, 2024

Uh oh!

hez2010 commented Jul 12, 2024

Uh oh!

eiriktsarpalis commented Jul 12, 2024

Uh oh!

eiriktsarpalis commented Jul 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

eiriktsarpalis commented Jul 11, 2024 •

edited

Loading

eiriktsarpalis commented Jul 12, 2024 •

edited

Loading

eiriktsarpalis commented Jul 12, 2024 •

edited

Loading

eiriktsarpalis commented Jul 12, 2024 •

edited

Loading

jkotas commented Jul 12, 2024 •

edited

Loading

hez2010 commented Jul 12, 2024 •

edited

Loading