Workload Target Imports Design by sfoslund · Pull Request #224 · dotnet/designs

sfoslund · 2021-06-04T22:18:18Z

No description provided.

terrajobst

This is a very well written document!

terrajobst · 2021-06-07T18:35:46Z

+### Change import location of `Directory.Build.targets`
+
+Change`Directory.Build.targets` to be imported after TargetFramework parsing but before workloads and most of the .NET SDK and common targets. This would be a big breaking change but might not affect most people who use `Directory.Build.targets`. It would break people who use `Directory.Build.targets` to override properties or targets in the MSBuild common targets.
+
+#### Pros:
+
+- Logic to enable workloads in `Directory.Build.targets` would “just work”, which is likely what developers expect


I strongly prefer this option. It took a while to evangelize the use of Directory.Build.props and Directory.Build.targets. I believe we should strive to optimize for letting developers do 99% of their tweaks by just these files. Having to put more stuff in new files whose import order is slightly different is a complexity nightmare. Yes, this is a breaking change but I think that's a perfect example where the net harm of the breaking change is much lower than the additional cognitive load over more files over time.

It's hard to grok how significant this is or is not though. Can we get some examples of which properties we can no longer override in Directory.Build.targets if we take this approach. At the moment it's pretty nebulous hence it's hard to decide if this is or is not the right approach.

terrajobst · 2021-06-07T18:37:16Z

+  - Logic would be split betweenthe .NET SDK and MSBuild, as the .NET SDK would now be responsible for importing `Directory.Build.targets` for projects that use the .NET SDK, but other projects would still need MSBuild to import `Directory.Build.targets`
+  - The new behavior would need to be behind a change wave and ideally a separate opt-out flag
+- This goes against the original intent behind `Directory.Build.targets`, which was that it would be imported after all built-in targets.  However, the .NET SDK (or any MSBuild SDK) already break this.
+  - As part of this work we could possibly add another extension point that allows you to specify `.targets` files that are imported after all other projects or imports.  This would likely be an MSBuild engine feature.


It would be good if you could state a preference from your team as you're the experts.

jaredpar · 2021-06-07T18:46:16Z

Have we evaluated how this would impact some of our repos? Basically will it cause us to change? I think that would be instructive for understanding the potential scope of the change for customers.

Co-authored-by: Immo Landwerth <immo@landwerth.net>

ghost · 2021-06-07T20:54:31Z

+
+Conceptually, `Directory.Build.targets` is imported after the body of the main project file. The exact location it is imported is not something most developers likely think about, but it is a good place to put common build logic that depends on properties set in the project file, such as the `TargetFramework`.
+
+There's not a perfect place to import `Directory.Build.targets`. However, given what we've learned, it may be that the best place to import it is after the TargetFramework parsing, and before the workloads are imported.  That way the logic in `Directory.Build.targets` would still be able to depend on the parsed TargetFramework information, but would not be able to override all of the targets and properties set by the .NET SDK and MSBuild common targets that it can today.


Would it be at all possible to give users a bit more control over shimming each of these stages? E.g. in addition to Directory.Build.targets, maybe also Directory.Build.AfterSdks.targets and Directory.Build.BeforeSdk.targets. Sure, you'll still probably break things when you change things around, but then at least users that relied on the order would have a destination to move their definitions to.

Also, very related to ordering, is there any chance that msbuild might eventually support lazily-evaluated properties? A large minority of the build ordering issues I help peers with revolves around reading properties too early (e.g. trying to read OutputPath in Directory.Build.props). Being able to set these using lazy evaluation (e.g. like gnu make = vs :=) in these cases would resolve a few issues. Admittedly, would create its own issues too, but worth a thought here.

poke · 2021-06-07T21:31:33Z

+
+### Change import location of `Directory.Build.targets`
+
+Change`Directory.Build.targets` to be imported after TargetFramework parsing but before workloads and most of the .NET SDK and common targets. This would be a big breaking change but might not affect most people who use `Directory.Build.targets`. It would break people who use `Directory.Build.targets` to override properties or targets in the MSBuild common targets.


Do we have a list of which targets would be affected here? (Alternatively, a link to the files, so we could check if this affected any of our use cases?)

RussKie · 2021-06-08T02:09:54Z

+
+## Background
+
+To support .NET SDK workloads, we [changed the order of targets imports](https://github.com/dotnet/sdk/pull/14393) to allow SDK workloads to change property defaults. When we did this, we also changed the import order of some Windows and WPF targets, as we want to make it a workload in the future, which caused a breaking change. It appears that it is not possible to allow workloads to change property defaults and make support for Windows a .NET SDK workload without introducing a breaking change. As a result, this document explores possible solutions to minimize the user impact.


Did you mean "Windows Forms and WPF"?

Suggested change

To support .NET SDK workloads, we [changed the order of targets imports](https://github.com/dotnet/sdk/pull/14393) to allow SDK workloads to change property defaults. When we did this, we also changed the import order of some Windows and WPF targets, as we want to make it a workload in the future, which caused a breaking change. It appears that it is not possible to allow workloads to change property defaults and make support for Windows a .NET SDK workload without introducing a breaking change. As a result, this document explores possible solutions to minimize the user impact.

To support .NET SDK workloads, we [changed the order of targets imports](https://github.com/dotnet/sdk/pull/14393) to allow SDK workloads to change property defaults. When we did this, we also changed the import order of some Windows Forms and WPF targets, as we want to make it a workload in the future, which caused a breaking change. It appears that it is not possible to allow workloads to change property defaults and make support for Windows a .NET SDK workload without introducing a breaking change. As a result, this document explores possible solutions to minimize the user impact.

RussKie · 2021-06-08T02:36:08Z

+
+### Extension Point via Property
+
+Support an `AfterTargetFrameworkInferenceTargets` property. This property could be used by creating a `Directory.AfterTargetFrameworkInference.targets` file and putting the following in the `Directory.Build.props` file located in the same folder:


If it is to accommodate workloads, why not introduce something like Workload.Build.props and Workload.Build.targets?

RussKie · 2021-06-08T02:36:46Z

+
+#### Cons
+
+- This would be a breaking change, possibly a very big one


A massive one...

RussKie · 2021-06-08T02:39:10Z

+
+#### MSBuild Evaluation
+
+We are primarily concerned with "[Pass 1](https://github.com/dotnet/msbuild/blob/6f9e0d620718578aab8dafc439d4501339fa4810/src/Build/Evaluation/Evaluator.cs#L613)" of [MSBuild Evaluation](https://docs.microsoft.com/en-us/visualstudio/msbuild/build-process-overview#evaluation-phase), where properties are evaluated and project imports are loaded.


https://review.docs.microsoft.com/help/contribute/links-how-to?branch=master#locale-codes-in-external-links

Suggested change

We are primarily concerned with "[Pass 1](https://github.com/dotnet/msbuild/blob/6f9e0d620718578aab8dafc439d4501339fa4810/src/Build/Evaluation/Evaluator.cs#L613)" of [MSBuild Evaluation](https://docs.microsoft.com/en-us/visualstudio/msbuild/build-process-overview#evaluation-phase), where properties are evaluated and project imports are loaded.

We are primarily concerned with "[Pass 1](https://github.com/dotnet/msbuild/blob/6f9e0d620718578aab8dafc439d4501339fa4810/src/Build/Evaluation/Evaluator.cs#L613)" of [MSBuild Evaluation](https://docs.microsoft.com/visualstudio/msbuild/build-process-overview#evaluation-phase), where properties are evaluated and project imports are loaded.

ditto below

sfoslund added 2 commits June 4, 2021 15:08

First draft of workload target imports design doc

c6c25de

Adding project import picture

5d99299

sfoslund changed the title ~~Workload dir build targets~~ Workload Target Imports Design Jun 4, 2021

terrajobst and others added 2 commits June 7, 2021 09:29

Add separator

e75502e

Update index

a00b933

terrajobst reviewed Jun 7, 2021

View reviewed changes

Apply suggestions from code review

516bad3

Co-authored-by: Immo Landwerth <immo@landwerth.net>

ghost reviewed Jun 7, 2021

View reviewed changes

poke reviewed Jun 7, 2021

View reviewed changes

Updating based on partner sync feedback

fdacd18

sfoslund force-pushed the WorkloadDirBuildTargets branch from dc944db to fdacd18 Compare June 7, 2021 22:35

RussKie reviewed Jun 8, 2021

View reviewed changes

marcpopMSFT mentioned this pull request Jun 28, 2021

Miscellaneous .NET 6 work dotnet/sdk#12900

Closed

15 tasks

marcpopMSFT mentioned this pull request Oct 27, 2021

Miscellaneous .NET 7 work dotnet/sdk#22362

Closed

7 tasks

sfoslund closed this Nov 23, 2021

sfoslund deleted the WorkloadDirBuildTargets branch November 23, 2021 18:10


		Conceptually, `Directory.Build.targets` is imported after the body of the main project file. The exact location it is imported is not something most developers likely think about, but it is a good place to put common build logic that depends on properties set in the project file, such as the `TargetFramework`.

		There's not a perfect place to import `Directory.Build.targets`. However, given what we've learned, it may be that the best place to import it is after the TargetFramework parsing, and before the workloads are imported. That way the logic in `Directory.Build.targets` would still be able to depend on the parsed TargetFramework information, but would not be able to override all of the targets and properties set by the .NET SDK and MSBuild common targets that it can today.


		### Change import location of `Directory.Build.targets`

		Change`Directory.Build.targets` to be imported after TargetFramework parsing but before workloads and most of the .NET SDK and common targets. This would be a big breaking change but might not affect most people who use `Directory.Build.targets`. It would break people who use `Directory.Build.targets` to override properties or targets in the MSBuild common targets.


		## Background

		To support .NET SDK workloads, we [changed the order of targets imports](https://github.com/dotnet/sdk/pull/14393) to allow SDK workloads to change property defaults. When we did this, we also changed the import order of some Windows and WPF targets, as we want to make it a workload in the future, which caused a breaking change. It appears that it is not possible to allow workloads to change property defaults and make support for Windows a .NET SDK workload without introducing a breaking change. As a result, this document explores possible solutions to minimize the user impact.


		### Extension Point via Property

		Support an `AfterTargetFrameworkInferenceTargets` property. This property could be used by creating a `Directory.AfterTargetFrameworkInference.targets` file and putting the following in the `Directory.Build.props` file located in the same folder:


		#### Cons

		- This would be a breaking change, possibly a very big one


		#### MSBuild Evaluation

		We are primarily concerned with "[Pass 1](https://github.com/dotnet/msbuild/blob/6f9e0d620718578aab8dafc439d4501339fa4810/src/Build/Evaluation/Evaluator.cs#L613)" of [MSBuild Evaluation](https://docs.microsoft.com/en-us/visualstudio/msbuild/build-process-overview#evaluation-phase), where properties are evaluated and project imports are loaded.

Conversation

sfoslund commented Jun 4, 2021

Uh oh!

terrajobst left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar commented Jun 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

terrajobst left a comment •

edited

Loading