Allow override of build tool framework version

[NikolaMilosavljevic]

Contributes to dotnet/source-build#3171

As discussed in dotnet/command-line-api#2283 this is a backport to Arcade of a change that allows source-build (and others) to override build tool framework version.

[ViktorHofer]

Can you please provide context on why this is necessary?

[NikolaMilosavljevic]

Can you please provide context on why this is necessary?

Source-build needs to use the unified version for all repos. Source-build is on latest Arcade and defaults to that value, i.e. net8.0. Repos that are on older arcade version, like command-line-api and xdt have older versions specified in their copies of tools.sh file. To workaround this, source-build modifies tools.sh before building specific repos. We are moving away from modification of source tree, as that will negatively impact our goals in 9.0 - see dotnet/installer#17664.

The proposed change helps ensure source-build can set the required version for all repos.

[ViktorHofer]

Do we believe that we will still need this change with the full VMR in .NET 9? I would expect us to use the same Arcade scripts and Arcade.Sdk which should avoid this issue. If this is just about xdt and command-line-api in main, I would prefer to just get them on the latest.

[NikolaMilosavljevic]

Do we believe that we will still need this change with the full VMR in .NET 9? I would expect us to use the same Arcade scripts and Arcade.Sdk which should avoid this issue. If this is just about xdt and command-line-api in main, I would prefer to just get them on the latest.

My original plan was to make a change in command-line-api and xdt, to unblock VMR until they get the 8.0 or later Arcade. The concern is that in future, when most repos are on 9.0 arcade, there could be some holdouts on 8.0 - that would require a new set of workarounds in VMR, for this very issue. Having a fix in arcade now would prevent something like it.

[ViktorHofer]

The concern is that in future, when most repos are on 9.0 arcade, there could be some holdouts on 8.0 - that would require a new set of workarounds in VMR, for this very issue. Having a fix in arcade now would prevent something like it.

That won't be possible anymore with the VMR full, right? I expect the VMR to only allow a single version of eng/common and the Arcade.Sdk. cc @mmitche @premun

[premun]

@viktor we had a short discussion on this topic but we don't have a plan yet. We still have to figure out what happens to code changes of VMR's, Arcade's eng/common and how those would sync between each other. We even had some ideas like packing eng/common inside of the Arcade.Sdk NuGet to make sure the scripts match the binaries always but we haven't really decided anything.

Once we switch onto the flat flow, all repos will hopefully get on the same Arcade eventually and then it will be easier to stay there but we are designing to keep this flexible and we will allow you to opt out of Arcade updates to allow for the transition.

We should also figure out whether we want to unify eng/common right away or maybe keep things as they are at first for the initial rollout of the new flow and only unifying/eradicating the copies later.

[mmitche]

At the very least, the Arcade SDK that the VMR uses (even in 8.0), is consistent. It first starts with the previously SB arcade, then all repos use the just-built one after that.

However! This does not mean that an individual repo, in its individual repo context, is not using an older arcade. That is a typical scenario. We cannot force upgrade across the stack. Tooling repos straddle .NET major versions. It does mean that you need to be able to build a repo using a newer arcade though, which is fine.

Scripts are in an inconsistent state today. I believe the build scripts used are from a component repo's eng/common layout, not a common layout in the VMR. Since the Arcade SDK and the scripts are designed to be aligned, that means that there is a possibility that the arcade scripts will be incompatible with the SDK being executed.

IMO, we want the following:

• The bootstrap portion of the VMR (building up to arcade), uses a specified Arcade SDK, with a companion set of scripts that match that SDK.
• After building arcade, all repos then use that freshly built arcade, as well as the scripts that correspond.

[NikolaMilosavljevic]

Hmm - hitting this issue in source-build:
2023-10-31T17:36:06.4116969Z /__w/1/s/eng/common/tools.sh: line 345: _OverrideArcadeInitializeBuildToolFramework: unbound variable

What am I missing? This is the actual command that failed:
if [[ -z "${_OverrideArcadeInitializeBuildToolFramework}" ]]; then

I'm hitting the same issue in command-line-api PR: dotnet/command-line-api#2283

[mthalman]

if [[ -z "${_OverrideArcadeInitializeBuildToolFramework}" ]]; then

The script is requiring all parameter references to be bound, meaning that a variable has to be set before it can be referenced. Since you're checking for the existence of the parameter, that gets a bit trickier.

This should work:

if ! [[ -z "${_OverrideArcadeInitializeBuildToolFramework+x}" ]]; then

This will allow you to check for the existence of the variable without triggering the unbound error. If it's not set, it will evaluate to x but that won't cause the condition to evaluate to true.

[NikolaMilosavljevic]

if [[ -z "${_OverrideArcadeInitializeBuildToolFramework}" ]]; then

The script is requiring all parameter references to be bound, meaning that a variable has to be set before it can be referenced. Since you're checking for the existence of the parameter, that gets a bit trickier.

This should work:

if ! [[ -z "${_OverrideArcadeInitializeBuildToolFramework+x}" ]]; then

This will allow you to check for the existence of the variable without triggering the unbound error. If it's not set, it will evaluate to x but that won't cause the condition to evaluate to true.

I was thinking of trying the following which should always work:
if [ x"${_OverrideArcadeInitializeBuildToolFramework}" == "x" ]; then

[mthalman]

I was thinking of trying the following which should always work:
if [ x"${_OverrideArcadeInitializeBuildToolFramework}" == "x" ]; then

That has the same unbound variable issue.

[markwilkie]

Please update the docs too

[NikolaMilosavljevic]

I was thinking of trying the following which should always work:
if [ x"${_OverrideArcadeInitializeBuildToolFramework}" == "x" ]; then

That has the same unbound variable issue.

OK - the following seems to work - testing fully before updating this PR:
if [[ "${_OverrideArcadeInitializeBuildToolFramework:-x}" == "x" ]]; then

[NikolaMilosavljevic]

Please update the docs too

Various properties with default values are documented in other places, but there are no environment variables that would lead to a suitable place for this new doc entry.

I did find this as the only reference to tools.sh:

arcade/Documentation/ArcadeSdk.md

Lines 102 to 104 in aa96cf3

	- eng/common/[tools.ps1](https://github.com/dotnet/arcade/tree/master/eng/common/tools.ps1)|[tools.sh](https://github.com/dotnet/arcade/tree/master/eng/common/tools.sh)
	Defines global variables and functions used in all builds scripts. This includes helpers that install .NET SDK, invoke MSBuild, locate Visual Studio, report build telemetry, etc.

Adding a new entry at the end of this file seems like the best option - would that work?

@markwilkie any other suggestions?

[markwilkie]

Yep - the SDK doc is the place where I'd put it. Thanks!

[NikolaMilosavljevic]

Yep - the SDK doc is the place where I'd put it. Thanks!

Thanks - fixed in eedbf1f

[NikolaMilosavljevic]

I can merge this - @mmitche @ViktorHofer any additional concerns?

[ViktorHofer]

I'm fine with the change as it helps with an existing scenario but I really hope that we don't need it with the VMR full model and make sure that we only have one /eng/common layout in the VMR which then gets synced back to the individual repos.

[ViktorHofer]

Should we backport this change into Arcade 8? We have couple of repositories in the source build graph that stay on Arcade 8's eng/common scripts: command-line-api, fsharp, msbuild, razor and roslyn (data from dotnet/source-build#3770).

We should also respect this switch in tools.ps1 on Windows: #14342

This will help until we come up with a design for dotnet/source-build#3710 (comment).

[mmitche]

Should we backport this change into Arcade 8? We have couple of repositories in the source build graph that stay on Arcade 8's eng/common scripts: command-line-api, fsharp, msbuild, razor and roslyn (data from dotnet/source-build#3770).

We should also respect this switch in tools.ps1 on Windows: #14342

This will help until we come up with a design for dotnet/source-build#3710 (comment).

Yep I think we should.

[mmitche]

/backport to release/8.0

[github-actions]

Started backporting to release/8.0: https://github.com/dotnet/arcade/actions/runs/7452632869

[ViktorHofer]

I think we should also backport all the other required eng/common updates. Let's bundle them all together into the backport PR.