Simple build infra improvements

[sdmaclea]

Distinguish between msbuild for cmake which require desktop msbuild.exe, and other cases which should all use dotnet msbuild

Fix small bash syntax error.

[hoyosjs]

Any reason not to use eng/common/msbuild.(ps1|sh)?

If we add

"tools": {
        "vs": {
            "version": "15.9",
            "components": [
                "Microsoft.Net.Component.4.6.TargetingPack",
                "Microsoft.VisualStudio.Component.PortableLibrary",
                "Microsoft.VisualStudio.Component.Static.Analysis.Tools",
                "Microsoft.VisualStudio.Component.Roslyn.Compiler",
                "Microsoft.VisualStudio.Component.VC.140",
                "Microsoft.VisualStudio.Component.VC.Tools.x86.x64",
                "Microsoft.Component.VC.Runtime.UCRTSDK",
                "Microsoft.VisualStudio.Component.VC.CoreIde",
                "Microsoft.VisualStudio.Component.Windows10SDK.17134",
                "Microsoft.VisualStudio.Component.Windows10SDK.16299.Desktop.arm",
                "Microsoft.VisualStudio.Component.VC.Tools.ARM",
                "Microsoft.VisualStudio.Component.VC.Tools.ARM64"
            ]
        }
    }

It will automatically pick up VS MSbuild at build time only for Windows architectures. Last time I tried this we had issues with it working with run.exe due to the default parameters obtained from config.json. However, I have a feeling this might just work now.

[sbomer]

Agree with @hoyosjs that using arcade's msbuild detection in msbuild.ps1 is worth looking into. It looks to me like if "vs" is present under tools, msbuild.ps1 will default to using the VS msbuild (even if "dotnet" is also specified): see https://github.com/dotnet/arcade/blob/29e9f23b054602e7b201fee3cbf3ba41c28cce8b/eng/common/tools.ps1#L361. So if we do that, we should be careful that we use dotnet msbuild for everything except the native build, by either:

• not specifying "vs" in global.json, but setting $msbuildEngine = "vs" in a wrapper script that we use for the native build, or
• adding "vs" to global.json, but using wrapper scripts that set $msbuildEngine = "dotnet" in a wrapper script that we use for the managed build.

I'm also fine with the current change since we will need some kind of wrapper script either way. We could use arcade vs detection in a later change.

[sbomer]

I'd prefer a name like vs_msbuild.cmd, just a suggestion.

[hoyosjs]

I'm just asking because I'm not familiar enough with all that buildtools does for our repo, but do we never have a case where this script could be called without having initialized tools? This uses the semaphore so it should be a no-op if left in here.

[sdmaclea]

init-tools should not be used for cmake use cases. Since I am explicitly trying to eliminate build tools references and trying to make this support the cmake usage only, I deliberately removed it.

[hoyosjs]

Ok, if it's only used for generating the vcxproj then this is a good idea. I just wasn't sure if we had any native-build targets that come from this that any native build flavor would depend on :) But most likely that would be caught by a screaming CI.

[hoyosjs]

Yes 👍 I like this

[sdmaclea]

@sbomer @hoyosjs I had considered eng/common/msbuild et al. Given the need for two msbuild engines, I assumed it might be difficult for arcade to support both, but hadn't gotten a chance to look at it yet. msbuildEngine approach looks correct.

I will probably deliberately limit the components in tool.vs so that it doesn't easily build managed code.
So I will probably not include

                "Microsoft.Net.Component.4.6.TargetingPack",
                "Microsoft.VisualStudio.Component.Roslyn.Compiler",

Most of the recommended change logically belongs in #22755. I will do it there.

vs_msbuild.cmd

I am on the fence. cmake explains why. vs explains how. Given the intent to only use this for cmake, I guess I prefer cmake_msbuild.cmd

[sdmaclea]

I am going to move the cmake_msbuild.cmd patch back to #22755.

[jkoritzinsky]

I think it would be good to explore using cmake --build /path/to/cmake/cache so we can leave it to cmake to detect the correct msbuild version in the future.

[sdmaclea]

@jkoritzinsky You may be right, but adding to global.json has a nice advantage of describing (and hopefully installing) the build dependencies. It is moved to #22755 now.

[sdmaclea]

@jkoritzinsky's approach may be better. Having two msbuilds is not easily supported as there are early outs if the build tools have been installed.