Further centralize shared Fx and TFM transition workarounds #27473

dougbu · 2020-11-02T23:06:37Z

import generated Directory.Build.* files in regular build
- add $(UpdateAspNetCoreKnownFramework) controlling Microsoft.AspNetCore.App KnownFrameworkReference update
- warn if requested Microsoft.AspNetCore.App version does not exist in $(DOTNET_ROOT)
- remove now-duplicate property and item settings outside generated Directory.Build.* files
- remove central $(BuildingTestAppsIndependently) special case
  - not needed because compiler toolset version is always available
- correct $(KnownAppHostPackOrFrameworkReferenceTfm) when not targeting the default TFM
mention errors with missing generated files and the new warning in BuildFromSource.md
- address numerous Markdown lint warnings, typos, and spelling mistakes in this file
use generated Directory.Build.* files for local template tests
- move Infrastructure/ files to TestInfrastructure/
  - Infrastructure/ sub-directories were functionally identical
- move shared parts of template test project files to PrepareForTest.targets
use generated Directory.Build.* files for local Razor tests
- set $(TargetLatestRuntimePatch) instead of $(RuntimeFrameworkVersion); simpler

Directory.Build.targets

dougbu · 2020-11-02T23:10:51Z

docs/BuildFromSource.md

@@ -15,13 +15,13 @@ This tutorial assumes that you are familiar with:

 ASP.NET Core uses git submodules to include the source from a few other projects. In order to pull the sources of the these submodules when cloning the repo, be sure to pass the `--recursive` flag to the `git clone` command.

-```ps1
+```powershell


This file wasn't really incredibly broken; it just gave the VS Code Markdown extension plenty to complain about

docs/BuildFromSource.md

dougbu · 2020-11-02T23:12:39Z

docs/BuildFromSource.md

@@ -154,26 +159,29 @@ to open the .sln file or one of the project specific .slnf files to work on the
   > branch, we regularly update the versions of .NET Core SDK required to build the repo.
   > You will need to restart Visual Studio every time we update the .NET Core SDK.

-Typically, you want to focus on a single project within the monorepo. For example,
+> :bulb: Rerunning the above command or, perhaps, the quicker `.\build.cmd -noBuildNative -noBuildManaged` may be
+> necessary after switching branches, especially if the `$(DefaultNetCoreTargetFramework)` value changes.


dougbu · 2020-11-02T23:13:16Z

docs/BuildFromSource.md

-### Use the result of your build
+### Resx files
+
+If you need to make changes to a .resx file, run `dotnet msbuild t:/Resgen <path to csproj>`. This will update the generated C#.


Slight reordering

docs/BuildFromSource.md

dougbu · 2020-11-02T23:23:02Z

eng/tools/GenerateFiles/Directory.Build.props.in

@@ -1,7 +1,6 @@
 <Project>
  <PropertyGroup>
    <DefaultNetCoreTargetFramework>${DefaultNetCoreTargetFramework}</DefaultNetCoreTargetFramework>
-    <TargetFramework>${DefaultNetCoreTargetFramework}</TargetFramework>


We don't have any projects lacking a $(TargetFramework) setting and this can cause problems where we use $(TargetFrameworks)

eng/tools/GenerateFiles/Directory.Build.targets.in

src/Razor/test/testassets/Directory.Build.props

dougbu · 2020-11-02T23:49:26Z

@captainsafia I'd appreciate your thoughts on docs/BuildFromSource.md changes in particular

captainsafia

BuildFromSource.md changes look good. Left a few notes inline.

docs/BuildFromSource.md

eng/tools/GenerateFiles/Directory.Build.targets.in

dougbu · 2020-11-20T04:20:40Z

@wtgodbe and @JunTaoLuo please review. Lots has changed since @captainsafia signed off.

dougbu · 2020-11-20T04:25:20Z

eng/tools/GenerateFiles/Directory.Build.targets.in

+      <DefaultRuntimeFrameworkVersion Condition=" '$(IsServicingBuild)' != 'true' AND
+          '$(TargetLatestDotNetRuntime)' != 'false' ">${MicrosoftNETCoreAppRuntimeVersion}</DefaultRuntimeFrameworkVersion>
+      <TargetingPackVersion Condition=" '$(IsServicingBuild)' != 'true' AND
+          '$(TargetLatestDotNetRuntime)' != 'false' ">${MicrosoftNETCoreAppRefVersion}</TargetingPackVersion>


@jkotalik the AND '$(TargetLatestDotNetRuntime)' != 'false' additions here are what #27845 needs (in root Directory.Build.targets) until this gets in.

Do I need to these changes specifically in this file as well? Sounds like no.

wtgodbe

Assuming this works, it seems reasonable, but I'm concerned that it seems to be adding complexity and might make our build even harder to understand than it already is - could you explain if/how the value it adds is large enough to overcome that (or if I'm overestimating the added complexity)?

dougbu · 2020-11-21T04:31:59Z

could you explain if/how the value it adds is large enough to overcome that (or if I'm overestimating the added complexity)?

I started this branch to

remove duplication of very similar workarounds sprinkled around the repo

this should make it easier to correct problems in an area we've had some churn and late-breaking concerns

avoid problems such as Improve KnownFrameworkReference workarounds in templatetests.props.in #14895

The work also helped me remove duplication I was less aware of e.g. between src/ProjectTemplates/BlazorTemplates.Tests/ and src/ProjectTemplates/test/.

What specific added complexity are you concerned about❔ I suspect at least some if what you're seeing existed before but in code I'm deleting.

dougbu · 2020-11-23T05:11:56Z

@wtgodbe this is now green. Do you object to merging❔

wtgodbe · 2020-11-30T20:28:57Z

What specific added complexity are you concerned about❔ I suspect at least some if what you're seeing existed before but in code I'm deleting.

Main concern is the use of the generated .props/.targets file everywhere now, it seems a bit unintuitive and may leave devs wondering where certain common properties "live" now

wtgodbe · 2020-11-30T22:39:19Z

If I clean the repo then run dotnet build on an arbitrary project, will it work/create the generated file correctly pre-build? If so I think I'm ok with this.

wtgodbe · 2020-12-01T23:24:13Z

Seems like that works, so I'm satisfied

dougbu · 2020-12-01T23:29:06Z

If I clean the repo then run dotnet build on an arbitrary project, will it work/create the generated file correctly pre-build? If so I think I'm ok with this.

dotnet build after git clean -e .dotnet/ won't work for many projects in the repo today. In fact, it'll fail for every project if the .dotnet/ folder is not up-to-date. That's why docs/BuildFromSource.md recommends running ./restore.cmd before doing much else.

If the SDK is up-to-date, the new minimum requirement after cleaning for dotnet build (in projects that don't require native assets) is ./build.cmd -noBuildNative -noBuildManaged or ./build.sh --no-build-managed. That just builds the repo tasks and generated the artifacts/bin/GenerateFiles/* files and takes a few seconds. I documented that in this PR.

dougbu · 2020-12-01T23:33:08Z

Seems like that works, so I'm satisfied

As I said, that isn't expected to work. I should have added that ./build.* -projects ... will always work because the command builds native assets (if on Windows), updates the SDK, ensures the needed runtimes are available, as well as generates the artifacts/bin/GenerateFiles/* files.

dougbu · 2020-12-01T23:35:00Z

Do those caveats undo your approval❔

wtgodbe

If the SDK is up-to-date, the new minimum requirement after cleaning for dotnet build (in projects that don't require native assets) is ./build.cmd -noBuildNative -noBuildManaged or ./build.sh --no-build-managed

I did my normal workflow, which is clean, restore, build a project using the restored .dotnet folder. Given that this breaks folks who are running dotnet build with a globally installed SDK that matches the one in global.json (e.g. once we ship 6.0 RTM, or if anyone has a 6.0 preview installed globally), I'm more hesitant. My gut feeling is that that scenario should work. Maybe if every restore operation creates the generated file if it doesn't already exist? In any case I'd prefer to talk about this more before making that breaking change.

dougbu · 2020-12-02T02:27:14Z

I did my normal workflow, which is clean, restore, build a project…

Ah, I thought you wanted to avoid ./restore.*. If you ./restore.* after cleaning, everything is just fine for any project that doesn't require native assets.

Given that this breaks folks who are running dotnet build with a globally installed SDK

This PR doesn't break using a globally-installed SDK. Globally-installed SDKs only work if they also include up-to-date runtimes from dotnet/runtime e.g. just after rebranding post-release. This restriction comes from the need to have the additional runtimes in the SDK folder. Nothing new here w.r.t. that restriction.

Did you request changes because you thought something about globally-installed SDKs had changed in this PR❔

wtgodbe · 2020-12-02T17:39:21Z

So what causes the new requirement to run ./build.cmd -noBuildNative -noBuildManaged or ./build.sh --no-build-managed before building a single project?

dougbu · 2020-12-02T20:51:42Z

So what causes the new requirement to run ./build.cmd -noBuildNative -noBuildManaged or ./build.sh --no-build-managed before building a single project?

I probably I confused you because I thought you wanted to avoid using ./restore.* and responded with the absolute minimal requirement to build a managed project. Those commands do less than ./restore.*.

wtgodbe · 2020-12-02T20:53:15Z

I probably I confused you because I thought you wanted to avoid using ./restore.* and responded with the absolute minimal requirement to build a managed project. Those commands do less than ./restore.*.

I understand that part, I just want to understand what (if any) scenarios worked before this PR, that do not work after - that is, who has to change their workflow?

dougbu · 2020-12-02T21:01:25Z

what (if any) scenarios worked before this PR, that do not work after

About the only things that change at all are scenarios involving switches between branches for which all needed runtimes are already in .dotnet/. In those cases, it's better to use ./build.* -projects {path} instead of or ./build.* -noBuildManaged -noBuildNative before dotnet build. But, the worst case would be using a different Microsoft.NETCore.App and that can only occur if the two branches began at different 'master' commits with dependency updates between.

wtgodbe

Ok, then this works for me

- make generated `Directory.Build.*` files more widely applicable - warn if requested Microsoft.AspNetCore.App version does not exist in `$(DOTNET_ROOT)` - add `$(UpdateAspNetCoreKnownFramework)` for Microsoft.AspNetCore.App `@(KnownFrameworkReference)` update - remove central `$(BuildingTestAppsIndependently)` special case - not needed because compiler toolset version is always available - correct `$(KnownAppHostPackOrFrameworkReferenceTfm)` when not targeting the default TFM - use MSBuild intrinsic functions for this and in framework projects; future-proofing - correct `@(KnownFrameworkReference)` metadata when in servicing - should not override default runtime and targeting pack versions - use generated `Directory.Build.*` files in regular build - remove now-duplicate property and item settings outside generated `Directory.Build.*` files - use generated `Directory.Build.*` files for local Razor tests - set `$(TargetLatestRuntimePatch)` instead of `$(RuntimeFrameworkVersion)`; simpler - do not restore Razor SDK test asset projects until just before tests run - depend on Microsoft.AspNetCore.App projects - disable `$(TreatWarningsAsErrors)` for a few Razor SDK tests - tests expect projects to build successfully despite a few warnings - improve (widen) Microsoft.AspNetCore.App `Condition` in Blazor SDK tests nit: do not pass `$(MicrosoftNetCompilersToolsetVersion)` into Razor test asset projects - not needed because generated files already contain the right information - even without that, the Directory.Build.props file imports eng/Versions.props

- move Infrastructure/ files to TestInfrastructure/ - Infrastructure/ sub-directories were functionally identical - move shared parts of template test project files to PrepareForTest.targets

…uildFromSource.md - address numerous Markdown lint warnings, typos, and spelling mistakes in this file

@captainsafia

- thanks @captainsafia Co-authored-by: Safia Abdalla <safia@microsoft.com>

- some was already duplicated - fix Markdown lint issues in BuildErrors.md too

- required for the new `msbuild` intrinsics - it is _not_ recommended to parse TFMs manually anymore - revert this after Visual Studio 16.8 reaches the regular agents

- move `_InstallFrameworkIntoLocalDotNet` earlier because other builds depend on this part - this target sometimes executes after dependent projects continue - add `DependsOnTargets` attributes to further constrain ordering nit: `IncludeFrameworkListFile` should run before `_ResolveSharedFrameworkContent`

- #27653 (review)

- let's see if 16.8.0 is new enough - regular queues were updated today This reverts commit e4ed1755ee74565b232bf0d08b7c631a6e775778.

…27473)" This reverts commit 5fd1db2.

…rounds (#27473)"" This reverts commit 5edfba8.

@captainsafia

…spnetcore#27473) * Further centralize shared Fx and TFM transition workarounds - make generated `Directory.Build.*` files more widely applicable - warn if requested Microsoft.AspNetCore.App version does not exist in `$(DOTNET_ROOT)` - add `$(UpdateAspNetCoreKnownFramework)` for Microsoft.AspNetCore.App `@(KnownFrameworkReference)` update - remove central `$(BuildingTestAppsIndependently)` special case - not needed because compiler toolset version is always available - correct `$(KnownAppHostPackOrFrameworkReferenceTfm)` when not targeting the default TFM - use MSBuild intrinsic functions for this and in framework projects; future-proofing - correct `@(KnownFrameworkReference)` metadata when in servicing - should not override default runtime and targeting pack versions - use generated `Directory.Build.*` files in regular build - remove now-duplicate property and item settings outside generated `Directory.Build.*` files - use generated `Directory.Build.*` files for local Razor tests - set `$(TargetLatestRuntimePatch)` instead of `$(RuntimeFrameworkVersion)`; simpler - do not restore Razor SDK test asset projects until just before tests run - depend on Microsoft.AspNetCore.App projects - disable `$(TreatWarningsAsErrors)` for a few Razor SDK tests - tests expect projects to build successfully despite a few warnings - improve (widen) Microsoft.AspNetCore.App `Condition` in Blazor SDK tests nit: do not pass `$(MicrosoftNetCompilersToolsetVersion)` into Razor test asset projects - not needed because generated files already contain the right information - even without that, the Directory.Build.props file imports eng/Versions.props * Use generated `Directory.Build.*` files for local template tests - move Infrastructure/ files to TestInfrastructure/ - Infrastructure/ sub-directories were functionally identical - move shared parts of template test project files to PrepareForTest.targets * Describe errors with missing generated files and the new warning in BuildFromSource.md - address numerous Markdown lint warnings, typos, and spelling mistakes in this file * Apply suggestions from code review - thanks @captainsafia * Move all troubleshooting information into BuildErrors.md - some was already duplicated - fix Markdown lint issues in BuildErrors.md too * Reorder App.Runtime build slightly - move `_InstallFrameworkIntoLocalDotNet` earlier because other builds depend on this part - this target sometimes executes after dependent projects continue - add `DependsOnTargets` attributes to further constrain ordering nit: `IncludeFrameworkListFile` should run before `_ResolveSharedFrameworkContent` * Add temporary workaround for `[AssemblyVersion]` changes * Address @wtgodbe's nit from dotnet/aspnetcore#27653 - dotnet/aspnetcore#27653 (review) Co-authored-by: Safia Abdalla <safia@microsoft.com> Commit migrated from dotnet/aspnetcore@5fd1db26257c

dougbu requested review from pranavkm and a team November 2, 2020 23:06

dougbu requested review from SteveSandersonMS and a team as code owners November 2, 2020 23:06

dougbu commented Nov 2, 2020

View reviewed changes

dougbu requested a review from captainsafia November 2, 2020 23:49

captainsafia approved these changes Nov 3, 2020

View reviewed changes

docs/BuildFromSource.md Show resolved Hide resolved

docs/BuildFromSource.md Outdated Show resolved Hide resolved

docs/BuildFromSource.md Outdated Show resolved Hide resolved

dougbu added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Nov 3, 2020

dougbu mentioned this pull request Nov 4, 2020

Improve KnownFrameworkReference workarounds in templatetests.props.in #14895

Closed

dougbu force-pushed the dougbu/more.build.in branch 3 times, most recently from 1788d8c to fd353d6 Compare November 8, 2020 03:36

dougbu force-pushed the dougbu/more.build.in branch from fd353d6 to 8a939b9 Compare November 17, 2020 19:34

dougbu commented Nov 17, 2020

View reviewed changes

eng/tools/GenerateFiles/Directory.Build.targets.in Outdated Show resolved Hide resolved

dougbu force-pushed the dougbu/more.build.in branch 3 times, most recently from 8cdc40e to d558c0a Compare November 20, 2020 03:45

dougbu mentioned this pull request Nov 20, 2020

[master] Update dependencies from dotnet/efcore dotnet/runtime #27845

Merged

dougbu force-pushed the dougbu/more.build.in branch from d558c0a to 70068f5 Compare November 20, 2020 04:11

dougbu commented Nov 20, 2020

View reviewed changes

This was referenced Nov 20, 2020

Use $(TargetFrameworkIdentifier) and $(TargetFrameworkVersion) consistently #25428

Merged

[release/5.0] Update API baseline files #27653

Merged

wtgodbe reviewed Nov 20, 2020

View reviewed changes

dougbu force-pushed the dougbu/more.build.in branch from 8c5173c to 15b0406 Compare November 21, 2020 04:08

dougbu force-pushed the dougbu/more.build.in branch from 15b0406 to 0e78d2d Compare November 22, 2020 23:37

wtgodbe approved these changes Dec 1, 2020

View reviewed changes

wtgodbe requested changes Dec 1, 2020

View reviewed changes

wtgodbe approved these changes Dec 2, 2020

View reviewed changes

dougbu force-pushed the dougbu/more.build.in branch from 0e78d2d to ac6fbe3 Compare December 3, 2020 03:08

dougbu and others added 10 commits December 2, 2020 19:17

Use generated Directory.Build.* files for local template tests

6619820

- move Infrastructure/ files to TestInfrastructure/ - Infrastructure/ sub-directories were functionally identical - move shared parts of template test project files to PrepareForTest.targets

Describe errors with missing generated files and the new warning in B…

3af70d4

…uildFromSource.md - address numerous Markdown lint warnings, typos, and spelling mistakes in this file

Apply suggestions from code review

d1fda49

- thanks @captainsafia Co-authored-by: Safia Abdalla <safia@microsoft.com>

Move all troubleshooting information into BuildErrors.md

a1d0f2f

- some was already duplicated - fix Markdown lint issues in BuildErrors.md too

Switch to .Pre queues for Windows build agents

906013e

- required for the new `msbuild` intrinsics - it is _not_ recommended to parse TFMs manually anymore - revert this after Visual Studio 16.8 reaches the regular agents

Add temporary workaround for [AssemblyVersion] changes

bf7ce1c

Address @wtgodbe's nit from #27653

9d62c79

- #27653 (review)

Revert "Switch to .Pre queues for Windows build agents"

1259f96

- let's see if 16.8.0 is new enough - regular queues were updated today This reverts commit e4ed1755ee74565b232bf0d08b7c631a6e775778.

dougbu force-pushed the dougbu/more.build.in branch from ac6fbe3 to 1259f96 Compare December 3, 2020 03:19

dougbu merged commit 5fd1db2 into master Dec 3, 2020

dougbu deleted the dougbu/more.build.in branch December 3, 2020 06:50

jkotalik added a commit that referenced this pull request Dec 4, 2020

Revert "Further centralize shared Fx and TFM transition workarounds (#…

5edfba8

…27473)" This reverts commit 5fd1db2.

jkotalik added a commit that referenced this pull request Dec 4, 2020

Revert "Revert "Further centralize shared Fx and TFM transition worka…

3b40f09

…rounds (#27473)"" This reverts commit 5edfba8.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Further centralize shared Fx and TFM transition workarounds #27473

Further centralize shared Fx and TFM transition workarounds #27473

dougbu commented Nov 2, 2020

dougbu Nov 2, 2020

dougbu Nov 2, 2020

dougbu Nov 2, 2020

dougbu Nov 2, 2020

dougbu commented Nov 2, 2020

captainsafia left a comment

dougbu commented Nov 20, 2020

dougbu Nov 20, 2020

jkotalik Nov 20, 2020

wtgodbe left a comment

dougbu commented Nov 21, 2020

dougbu commented Nov 23, 2020

wtgodbe commented Nov 30, 2020

wtgodbe commented Nov 30, 2020

wtgodbe commented Dec 1, 2020

dougbu commented Dec 1, 2020 •

edited

Loading

dougbu commented Dec 1, 2020 •

edited

Loading

dougbu commented Dec 1, 2020

wtgodbe left a comment

dougbu commented Dec 2, 2020

wtgodbe commented Dec 2, 2020

dougbu commented Dec 2, 2020

wtgodbe commented Dec 2, 2020

dougbu commented Dec 2, 2020

wtgodbe left a comment

Further centralize shared Fx and TFM transition workarounds #27473

Further centralize shared Fx and TFM transition workarounds #27473

Conversation

dougbu commented Nov 2, 2020

dougbu Nov 2, 2020

Choose a reason for hiding this comment

dougbu Nov 2, 2020

Choose a reason for hiding this comment

dougbu Nov 2, 2020

Choose a reason for hiding this comment

dougbu Nov 2, 2020

Choose a reason for hiding this comment

dougbu commented Nov 2, 2020

captainsafia left a comment

Choose a reason for hiding this comment

dougbu commented Nov 20, 2020

dougbu Nov 20, 2020

Choose a reason for hiding this comment

jkotalik Nov 20, 2020

Choose a reason for hiding this comment

wtgodbe left a comment

Choose a reason for hiding this comment

dougbu commented Nov 21, 2020

dougbu commented Nov 23, 2020

wtgodbe commented Nov 30, 2020

wtgodbe commented Nov 30, 2020

wtgodbe commented Dec 1, 2020

dougbu commented Dec 1, 2020 • edited Loading

dougbu commented Dec 1, 2020 • edited Loading

dougbu commented Dec 1, 2020

wtgodbe left a comment

Choose a reason for hiding this comment

dougbu commented Dec 2, 2020

wtgodbe commented Dec 2, 2020

dougbu commented Dec 2, 2020

wtgodbe commented Dec 2, 2020

dougbu commented Dec 2, 2020

wtgodbe left a comment

Choose a reason for hiding this comment

dougbu commented Dec 1, 2020 •

edited

Loading

dougbu commented Dec 1, 2020 •

edited

Loading