Reconsider requiring global.json to specify the SDK version

[nguerrera]

See the earlier thread on this: dotnet/corefx#30919 (comment)

Continuing the discussion here. I won't repeat all of my thoughts, but here's a quick summary:

As long as global.json sdk specification behaves as it does today (*), I believe it is detrimental to use this same field as the toolset that build.cmd pulls down.

• It "pins" the SDK, it does not manifest a minimum version.
• It only pins the sdk tasks and targets, which are just a sliver of everything you actually would need to pin to make the IDE behave exactly as build.cmd.
• It prevents just opening projects in VS with bits installed for you from our dogfooding channel. You have to munge PATH or install random builds to Program Files.
• It harms dogfooding to use pinned versions unnecessarily.

I think we should be able to specify the CLI to download and use in command line builds without pinning down the IDE.

(*) I am 100% open to changing how global.json works and happy to discuss that here, but we should separate these two things in our conclusion here:

1. What is the best approach for the tooling we have today.
2. How can we change the tooling to enable different approaches that would be even better tomorrow.

For (1), I firmly believe we should provide a different way to specify which CLI to download and use in build.cmd that does not set the sdk version in global.json.

@weshaggard @eerhardt @tmat

[nguerrera]

cc @dsplaisted @dotnet/dotnet-cli

[weshaggard]

cc @mmitche @chcosta @markwilkie

We could consider just putting the version of the sdk to bootstrap as another field in global.json file similar to what we plan to do for native tools (https://github.com/dotnet/arcade/blob/master/Documentation/Projects/NativeDependencies/Design.md#questions). That way we wouldn't be required to use that version but if you build via the build scripts it will bootstrap that version and use it to build the repo.

[dsplaisted]

We could consider just putting the version of the sdk to bootstrap as another field in global.json file

I'd suggest using something besides json, because json doesn't support comments, and if you have a list of dependencies with version numbers you're eventually probably going to want to add a comment about one of them.

I thought the way it used to work with RepoToolset was decent: Put the property in a specific .props file that's imported by the build, and parse the XML from the build scripts. You do need to make sure not to use MSBuild features such as conditions or variable substitutions.

[weshaggard]

That is fair as well given we were only using the global.json file because of the sdk version, I'd be fine using another file that could be shared with the build and parsed easily by our bootstrapping scripts.

[weshaggard]

I'd say we could get rid of our global.json dependency completely but I think we still need it for our sdk package versions. There isn't anyway currently that I"m aware of to fully specify that a targets/props file. However if we move to the model of only specifying it once in the root Directory.Build.props/targets file (https://github.com/dotnet/arcade/blob/master/Directory.Build.props#L7) we could consider hard-coding it in both of those places instead.

[mmitche]

/fyi @jcagme since this is darc related too. @weshaggard Yeah i'm okay with that.

[nguerrera]

cc @jeffkl

I believe you could use a variable for the nuget msbuild SDK imports: https://docs.microsoft.com/en-us/visualstudio/msbuild/how-to-use-project-sdk
https://docs.microsoft.com/en-us/visualstudio/msbuild/how-to-use-project-sdk

When using the element, you can specify an optional Version attribute as well. For example, you can specify <Import Project="Sdk.props" Sdk="My.Custom.Sdk" Version="1.2.3" />.

This makes me think this would work fine:

<Import Project="Sdk.props" Sdk="Microsoft.DotNet.Arcade.Sdk" Version="$(MicrosoftDotNetArcadeSdkPackageVersion)" />

[tmat]

1. Do we not want to pin the SDK to specific version to have reproducible builds and tests? Are we ok with checking out an older branch of a repo, running tests and have them potentially failing if there is a breaking change in the newer SDK?
2. We need to be able to read the version from build.ps1 and build.sh, which download the .NET SDK. Having the version in a props file makes that hard.

//cc @jaredpar

[nguerrera]

This will not impact build.cmd/test.cmd being reproducible. You can achieve that without pinning what happens if you build in an IDE, or with dotnet on your PATH, or msbuild on your PATH. This is no different than how I am allowed to use the compiler from those places on my local machine for my local productivity and to ensure dogfooding builds actually get put to real work.

[nguerrera]

I have no opinion on which file or format specifies the sdk version to acquire and use in build script. Just that the "sdk" node in global.json is not the right place.

[mmitche]

I think it should be some other place than a props file if it's not going to be in global.json. And yes, the sha must tie to a specific version of the SDK (though should be overrideable, perhaps with a source change)

[tmat]

I am open to @weshaggard suggestion:

We could consider just putting the version of the sdk to bootstrap as another field in global.json file similar to what we plan to do for native tools

That would work well with minimal changes.

I don't like specifying the version in a props file since that makes parsing it in .sh and .ps1 file complicated and introduces restrictions on that props file that do not apply to other props files.

I don't like specifying the version in another text file since that increases the number of files we need to maintain.

[tmat]

Also there is an ongoing discussion on how to better restore .NET SDK in customer repos.
If the current way of specifying it in Arcade causes issues then let's make a minimal change that fixes that. Once the discussion is concluded we implement what we ship.

@KathleenDollard @richlander

[tmat]

Also, let's not make any changes until we agree on removing the version from here:
https://github.com/dotnet/roslyn/blob/master/global.json#L3

[nguerrera]

Re ongoing discussion, this is exactly the 1,2 split in my description

[weshaggard]

Are there any pointers to the ongoing discussion for the shipping product?

The only reason I think we put this into a props file would be to allow for us to override it however if we allow overriding we break our bootstrapping scripts because there isn't any real way for them to parse out an overridden value without doing an msbuild evaluation which we cannot do pre-bootstrap. For that reason I think putting it in another file makes sense whether that is global.json or another text file that can easily be read and parsed by our bootstrap scripts.

[dsplaisted]

The actual format of the file holding the version number is very much a secondary concern. I suggested a .props file because if you need to access it from MSBuild logic in addition to the bootstrapping scripts, then putting it in .props makes that easier than trying to parse json from MSBuild, and because I also like formats which allow for comments.

[mmitche]

Is it that difficult to parse out in *nix during bootstrapping if it's in xml? We assuming a well-formed xml element with a known name, isn't it pretty easy to grep for? We don't necessarily need to allow for multi-line xml elements or anything like that.

[weshaggard]

I agree we could parse a restricted props file for the versions, and I'd be fine with that option, but it does lead folks to think it is a normal msbuild property which can be overridden and such but our bootstrapping scripts will never be able to read the overridden value unless it is actually changed in the props file.

[jcagme]

Any updates on where to store the SDK version?

[tmat]

In global.json, under different name.

[nguerrera]

How about respecting "sdk" if different name was not found, then you if you decide to pin, you don't need to repeat yourself by specifying the same version twice. It also makes rolling this out simpler because it won't break existing use cases.

How about "sdk_download" as the different name?

I don't have strong opinions here, but just figure it would be helpful to land on something.

[tmat]

How about "sdk_download" as the different name?

We don't necessarily need to download the SDK if the exact same version is already installed.
build-sdk?

[nguerrera]

Works for me.

[tmat]

I'm gonna implement it like so:

{
  "tools": {
    "dotnet": "2.1.400-preview-009088"
  }
}

This makes it very easy to read in bash (regex match "dotnet": "(.*)") and it's similar to native-tools (see spec).
We can unify native tools installation later with dotnet installation and put everything under tools.

[weshaggard]

Do we have another issue tracking the "native-tools" change?