Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi framework #2343

Merged
merged 5 commits into from
May 6, 2019
Merged

Multi framework #2343

merged 5 commits into from
May 6, 2019

Conversation

chcosta
Copy link
Member

@chcosta chcosta commented Mar 26, 2019

Fixes #1441

This change ensures the "dotnet-install" script is always downloaded to [repo root]\.dotnet so that it is available during toolset restore and then it downloads additional runtimes by default only if you are a CI build.

Validated in Roslyn builds with https://github.com/dotnet/roslyn/pull/34298/files

Additional validation is in progress as I've modified this PR slightly since my last validation build

Documentation/ArcadeSdk.md Outdated Show resolved Hide resolved
@tmat
Copy link
Member

tmat commented Mar 26, 2019

Will take a look.

@ericstj
Copy link
Member

ericstj commented Mar 26, 2019

Looks like this will acquire shared frameworks only on CI. It seems to me that folks would at least like to "ensure" shared frameworks even for a local build (even when running on a machine-wide CLI): did you consider this functionality?

Taking that a step further, one could imagine that the presence of shared frameworks would go into the selection of a dotnet CLI to use, but I can see how this is hard with your current approach of placing these in MSBuild.

@chcosta
Copy link
Member Author

chcosta commented Mar 26, 2019

Ideally, to me, the shared framework acquisition wouldn't happen in MSBuild, it would happen in the tools scripts. There was some previous discussion that went back and forth on this and everyone agreed that using MSBuild was fine for now because otherwise you have to synchronize your global.json and a props files and it's a pain to keep things correct.

It seems to me that folks would at least like to "ensure" shared frameworks even for a local build (even when running on a machine-wide CLI): did you consider this functionality?

You can ensure shared frameworks by running with /p:EnableInstallDotNetCoreRuntimes=true. I opted out of this as the default because it seems bad to just go installing these runtime frameworks with every build into your globally selected CLI.

@tmat
Copy link
Member

tmat commented Mar 26, 2019

You can ensure shared frameworks by running with /p:EnableInstallDotNetCoreRuntimes=true. I opted out of this as the default because it seems bad to just go installing these runtime frameworks with every build into your globally selected CLI.

We should install the shared frameworks in every build (local or CI) in Restore phase. The installation should be no-op if the frameworks are already present. That can be easily done by checking for existence of the SDK directories, so it shouldn't cause any delay.

I don't think we should have EnableInstallDotNetCoreRuntimes option.

@tmat
Copy link
Member

tmat commented Mar 26, 2019

@ericstj

the presence of shared frameworks would go into the selection of a dotnet CLI to use

Do you mean selection of dotnet CLI to run the build by? Why would that one need any shared frameworks, other than the default one? My understanding is that we only need the extra shared frameworks for testing purposes.

@ericstj
Copy link
Member

ericstj commented Mar 26, 2019

Why would that one need any shared frameworks, other than the default one? My understanding is that we only need the extra shared frameworks for testing purposes.

Your build may depend on .NETCore based tools that run on different shared frameworks out of process. Imagine MSBuild is running on 2.1 and CSC needs to run on 3.0 (or vice-versa) and runs out of proc. This is a bit contrived, but I believe @ViktorHofer came across some scenarios like this.

@tmat
Copy link
Member

tmat commented Mar 26, 2019

@ericstj I'd rather have our build depend only on a single version of .NET Core.

@ericstj
Copy link
Member

ericstj commented Mar 26, 2019

@ericstj I'd rather have our build depend only on a single version of .NET Core.

Sure, I'd also prefer that, but I don't think that's a pre-existing requirement for everyone outside of source build. I believe there are a non-zero number of repos running on multiple shared-frameworks today. We have some standalone tools that build out of arcade: do those need to cross-compile for netcoreapp2.x and netcoreapp3.x so that they can be consumed by repos who wish to run on only one shared framework or the other? What about community tools that some of the repos use? Imagine a case where we're on a leading edge of transition to a new side-by-side non-rollforward shared framework (eg: 2.1 > 3.0) we create an all-or-nothing hurdle by not permitting side-by-side frameworks in the build. That said, maybe we're OK with repos forcing the local toolset in these cases.

@tmat
Copy link
Member

tmat commented Mar 26, 2019

Sure, I'd also prefer that, but I don't think that's a pre-existing requirement for everyone outside of source build.

Yes, source build would be the major concern, followed by a general complexity concern.

Imagine a case where we're on a leading edge of transition to a new side-by-side non-rollforward shared framework (eg: 2.1 > 3.0)

Why would we not enable roll-forward for these tools?

@ericstj
Copy link
Member

ericstj commented Mar 26, 2019

Why would we not enable roll-forward for these tools?

Either they don't work on the newer shared framework or you can't change their runtimeconfig or control how they are launched. @ViktorHofer has some data I think. You may want to look at what other repos hit when they try to move from a 2.x SDK to a 3.0 SDK.

Anyhow, as I previously mentioned I think we're fine with the first step of acquiring the shared Fx to the local folder.

@chcosta have you looked into providing scripts or establishing patterns for folks who want to use the locally acquired bits for VS development or were you thinking to address that in a separate PR?

@chcosta
Copy link
Member Author

chcosta commented Mar 26, 2019

@chcosta have you looked into providing scripts or establishing patterns for folks who want to use the locally acquired bits for VS development or were you thinking to address that in a separate PR?

My intent was to address the issues brought up in issue #1441 and get something supported in place that wasn't throw away work and could unblock the various teams encountering issues in this space by providing a unified way to handle them. We should file an issue to address locally acquired bits for VS development (I hear there are some dragons in that work) and address that separately.

Documentation/ArcadeSdk.md Outdated Show resolved Hide resolved
Documentation/ArcadeSdk.md Outdated Show resolved Hide resolved
eng/common/tools.ps1 Outdated Show resolved Hide resolved
Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, just some details to sort out.

eng/common/dotnet-install.ps1 Outdated Show resolved Hide resolved
@vatsan-madhavan
Copy link
Member

so we'd have to a reliable way to find the assembly location that we could use

We should not depend on global installations of anything for product builds.

WPF already consumes the shared framework during builds without making any assumptions about global availability. Can you explain what you are trying to do and what’s not working for you? (Should this be moved to an issue on dotnet/wpf?)

WPF has a very few deliberate exceptions related to VS supplied tooling and libraries related to C++, but Json libraries shouldn’t be one.

@tmat
Copy link
Member

tmat commented Apr 22, 2019

@stevenbrix

Will this always install the shared framework to the .dotnet folder for local and ci builds? It sounds like if we add the "runtimes" to our global.json, that will ensure we always get the shared frameworks installed there?

Correct. If runtimes is specified dotnet is installed to .dotnet dir for all builds.

Are Arcade CLI builds planning to move to .NET Core instances of MSBuild?

Not sure what you mean. The build scripts use .NET Core msbuild unless $msbuildEngine parameter specifies otherwise, or global.json lists vs is tools. See doc.

Is it expected/supported that we can depend on the shared framework installation (or perhaps the global installation if it exists) for MSBuild targets? The WPF team has been looking into T4 generation for a lot of the code generation that we have, and would like to use System.Text.Json. However, there is no binary package available, so we'd have to a reliable way to find the assembly location that we could use.

You can only rely on common MSBuild targets and .NET Core SDK targets to be present. These will be of the version specified under tools.dotnet. T4 is not part of these targets. I assume T4 targets are distributed via a nuget package. You'd need to reference this package from your projects. However, you might have a problem with source build. We need to be able to build all repositories from source. So we would need to include T4 repository (whereever it lives) in source build.

@tmat
Copy link
Member

tmat commented Apr 22, 2019

Re System.Text.Json: my understanding is that this is available as a built-in reference for projects targeting netcoreapp3.0. It is also available as a source package for netstandard2.0.

In repos that require .NET Core SDK 3.0 for build (they specify tools.dotnet version 3.0 in global.json) build tasks can use System.Text.Json directly without adding the source package.

@stevenbrix
Copy link
Contributor

stevenbrix commented Apr 22, 2019

We should not depend on global installations of anything for product builds.

Well yes, I don't want to do that. But from reading through this PR it seems that it would be prefered that repos take a dependency on a version of dotnet, and not depend on whether it's installed globally or in the .dotnet folder. I was mostly just thinking of ways to work around the lack of System.Text.Json nuget package if we moved to a .NET Core T4 implementation.

Not sure what you mean. The build scripts use .NET Core msbuild unless $msbuildEngine parameter specifies otherwise, or global.json lists vs is tools

Cool, didn't know this! Yeah we are dependent on VS, so that would be why we are using the .NET Framework instance of MSBuild

You can only rely on common MSBuild targets and .NET Core SDK targets to be present. These will be of the version specified under tools.dotnet. T4 is not part of these targets. I assume T4 targets are distributed via a nuget package. You'd need to reference this package from your projects.

The T4 targets are part of the Visual Studio installation, so that is how they are available to us. We don't need a nuget reference or anything.

Re System.Text.Json: my understanding is that this is available as a built-in reference for projects targeting netcoreapp3.0. It is also available as a source package for netstandard2.0.

Yes, but the T4 engine runs in the .NET Framework instance of MSBuild, and how the assemblies are consumed doesn't work with how System.Text.Json is currently shipped. If there was a netstandard binary we could reference it. But we'll probably wind up not depending on json parsing at all so this won't be a blocker

@tmat
Copy link
Member

tmat commented Apr 22, 2019

I guess I'm missing key information about how T4 is distributed. Is there not a standalone nuget package with T4 msbuild targets and tasks that can be used without Visual Studio?

@tmat
Copy link
Member

tmat commented Apr 22, 2019

BTW, we should probably discuss T4 in another thread or in person. It's not directly related to this PR.

@vatsan-madhavan
Copy link
Member

@stevenbrix, please include myself and @rladuca in the T4 conversation. I'd love to learn about this in depth.

@stevenbrix
Copy link
Contributor

I'll follow up with you guys offline about T4

Documentation/ArcadeSdk.md Outdated Show resolved Hide resolved
Documentation/ArcadeSdk.md Outdated Show resolved Hide resolved
Documentation/ArcadeSdk.md Outdated Show resolved Hide resolved
Documentation/ArcadeSdk.md Outdated Show resolved Hide resolved
eng/common/build.ps1 Outdated Show resolved Hide resolved
@tmat
Copy link
Member

tmat commented May 3, 2019

Looks great. Just a few minor points.

Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@tmat
Copy link
Member

tmat commented May 6, 2019

Looks very clean now! Thanks for iterating on this.

@chcosta chcosta merged commit 917ed3a into dotnet:master May 6, 2019
@markwilkie
Copy link
Member

Hooray!!!

@chcosta - could you send mail to the working group please alerting folks that this about to be available please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for installing .NET Core shared frameworks
8 participants