Buildwork to remove dir.props files #26960

davidwrighton · 2019-10-01T01:20:00Z

No description provided.

…n Unix

…into buildwork

Capture logs from test runs Tweak Directory.Build.props use from runtest.proj and xunitconsolerunner so that it "works"

davidwrighton · 2019-10-02T20:57:24Z

/azp help

azure-pipelines · 2019-10-02T20:57:28Z

Supported commands help: Get descriptions, examples and documentation about supported commands Example: help "command_name" list: List all pipelines for this repository using a comment. Example: "list" run: Run all pipelines or a specific pipeline for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify a pipeline to run. Example: "run" or "run pipeline_name" where: Report back the Azure DevOps orgs that are related to this repository and org Example: "where" See additional documentation.

davidwrighton · 2019-10-02T20:58:06Z

/azp run coreclr-ci

azure-pipelines · 2019-10-02T20:58:11Z

Pull request contains merge conflicts.

…work

…ways be true

jashook · 2019-10-04T15:56:52Z

EmptyProps.props

@@ -0,0 +1,2 @@
+<Project>


What is the purpose of this file? Do you mind adding it as a comment in the file?

jashook · 2019-10-04T15:57:14Z

build-test.cmd

@@ -324,7 +324,7 @@ for /l %%G in (1, 1, %__NumberOfTestGroups%) do (

    if not "%__CopyNativeTestBinaries%" == "1" (
        REM Disable warnAsError - coreclr issue 19922
-        set __MSBuildBuildArgs=!__ProjectDir!\tests\build.proj -warnAsError:0 !__Logging! !TargetsWindowsMsbuildArg! !__msbuildArgs! !__PriorityArg! !__UnprocessedBuildArgs! /p:CopyNativeProjectBinaries=!__CopyNativeProjectsAfterCombinedTestBuild!
+        set __MSBuildBuildArgs=!__ProjectDir!\tests\build.proj -warnAsError:0 /nodeReuse:false !__Logging! !TargetsWindowsMsbuildArg! !__msbuildArgs! !__PriorityArg! !__UnprocessedBuildArgs! /p:CopyNativeProjectBinaries=!__CopyNativeProjectsAfterCombinedTestBuild!


Thank you for adding nodeReuse=false!

We should make a note to set nodeReuse to false in configure-toolset.ps1/sh (separately from this change).

jashook · 2019-10-04T15:58:33Z

build.cmd

@@ -302,6 +302,7 @@ if /i %__BuildType% NEQ Release set __RestoreOptData=0

 set "__BinDir=%__RootBinDir%\Product\%__BuildOS%.%__BuildArch%.%__BuildType%"
 set "__IntermediatesDir=%__RootBinDir%\obj\%__BuildOS%.%__BuildArch%.%__BuildType%"
+set "__ArtifactsIntermediatesDir=%__ProjectDir%\artifacts\obj\"


Are we no longer using bin/obj?

Are native ones still in bin/obj? Might be worth commenting.

jashook · 2019-10-04T15:59:18Z

build.cmd

@@ -321,6 +322,9 @@ if not exist "%__IntermediatesDir%"    md "%__IntermediatesDir%"
 if not exist "%__LogsDir%"             md "%__LogsDir%"
 if not exist "%__MsbuildDebugLogsDir%" md "%__MsbuildDebugLogsDir%"

+if not exist "%__RootBinDir%\Directory.Build.props" copy %__ProjectDir%\EmptyProps.props %__RootBinDir%\Directory.Build.props


Same as emptyprops, this seems to need comments explaining what this is doing and why.

jashook · 2019-10-04T15:59:58Z

build.cmd

@@ -430,7 +434,7 @@ REM ===
 REM =========================================================================================

 set __IntermediatesIncDir=%__IntermediatesDir%\src\inc
-set __IntermediatesEventingDir=%__IntermediatesDir%\Eventing
+set __IntermediatesEventingDir=%__ArtifactsIntermediatesDir%\Eventing\%__BuildArch%\%__BuildType%


+1 for buildarch and build type. Could you also add OS?

This matches with where the Arcade logic puts stuff. I'm not changing that.

jashook · 2019-10-04T16:06:20Z

...uget/Microsoft.TargetingPack.Private.CoreCLR/Microsoft.TargetingPack.Private.CoreCLR.pkgproj

@@ -1,6 +1,6 @@
 <?xml version="1.0" encoding="utf-8"?>
 <Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
-  <Import Project="$([MSBuild]::GetPathOfFileAbove(dir.props))" />
+  <Import Project="$([MSBuild]::GetPathOfFileAbove(Directory.Build.props))" />


Same issue as the ilasm pkgproj

jashook · 2019-10-04T16:08:12Z

src/System.Private.CoreLib/System.Private.CoreLib.csproj

@@ -419,7 +417,7 @@
  </ItemGroup>
  <Import Project="shared\System.Private.CoreLib.Shared.projitems" />
  <PropertyGroup>
-    <CheckCDefines Condition="'$(CheckCDefines)'==''">true</CheckCDefines>
+    <CheckCDefines Condition="'$(CheckCDefines)'=='' And '$(PYTHON)' != ''">true</CheckCDefines>


It seems like silently avoiding checks if python is not installed is incorrect. My opinion would be to allow it to fail, and we create a follow up future work item to convert the check to msbuild.

Mmm. I don't need to make this change at all. Its really just a convenience as I prefer for it to be possible to compile major projects in the tree without specifying any special parameters to the msbuild command. I'll remove this adjustment.

tests/build.proj

tests/src/dirs.proj

jashook · 2019-10-04T16:14:01Z

tests/src/runtest.proj

@@ -1,6 +1,6 @@
 <?xml version="1.0" encoding="utf-8"?>
 <Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
-  <Import Project="src\dir.props" />
+  <Import Project="Directory.Build.props" />


This also should probably be an sdk style project.

Converting .proj files into sdk style projects does odd things. If we need to do that, it should be a separate change.

dir.common.props

sbomer · 2019-10-07T22:10:05Z

tests/Directory.Build.props

  <!-- Add test-specific package restore sources. -->
  <PropertyGroup>
    <RestoreSources Condition="'$(DotNetBuildOffline)' != 'true'">
      https://dotnet.myget.org/F/dotnet-corefxlab/api/v3/index.json;
+      https://dotnetfeed.blob.core.windows.net/dotnet-core/index.json;


Why were these added, and would it be possible to put them in NuGet.Config instead? Arcade is recommending to move away from RestoreSources: https://github.com/dotnet/arcade/blob/f8fc65bce78b79d00c66cb22ff658fb3a3e46dfb/Documentation/RestoreSourcesUpdateStatus.md.

Well, we weren't using nuget.config in our test builds at all. But the move to unify the properties resulted in an empty RestoreSources at this point in the build. This then added a single entry, and that wasn't sufficient for restore to work. I've moved the test build process to use nuget.config and that appears to work fine. Nice catch.

elinor-fung · 2019-10-07T23:31:28Z

dependencies.props

@@ -11,9 +11,6 @@
    </RestoreSources>


Can we remove RestoreSources now that everything is through Arcade? These should be set in the repo's nuget.config already.

Yes we can. See my response to @sbomer

elinor-fung · 2019-10-07T23:37:45Z

dependencies.props

@@ -11,9 +11,6 @@
    </RestoreSources>
  </PropertyGroup>

-  <!-- [ARCADE REMOVE] Versions.props is imported by Arcade SDK targets when building with Arcade -->
-  <Import Project="eng/Versions.props" Condition="'$(ArcadeBuild)' != 'true'" />
-
  <!-- Source of truth for dependency tooling: the commit hash of the dotnet/versions master branch as of the last auto-upgrade. -->


How exactly do all these dependency tooling / auto-upgrade properties get used?

From the comments in this file (and only finding references to them in Microsoft.DotNet.VersionTools), I'd think we don't need or want them always chained in by the top-level Directory.Build.props?

To be honest, I have no idea what any of the dependency stuff does. I don't think these changes I've made don't actually add these to our build, they have always been there.

Yeah, just noticed that it is always being included now (by the top-level .props), which doesn't seem to hurt anything, but might be good to clean up. Maybe @sbomer knows from his work to consume Microsoft.DotNet.VersionTools for updating published versions?

I'm not familiar with them either - they were used for buildtools dependency flow, and we can probably get rid of most of them now that we're on arcade. I'd need to go through them one-by-one to see which ones are still required for VersionTools.

elinor-fung · 2019-10-07T23:39:27Z

dir.common.props

+    <!-- Default to portable build if not explicitly set -->
+    <PortableBuild Condition="'$(PortableBuild)' == ''">true</PortableBuild>
+
+    <OverrideRestoreOutputPath>true</OverrideRestoreOutputPath>


I think this property was for BuildTools

I dunno. There don't appear to be any uses of it. So I've removed it.

elinor-fung · 2019-10-07T23:51:29Z

src/.nuget/Directory.Build.targets

@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="utf-8"?>


Everything in this file should only be needed for .pkgproj files. It seems unnatural to me that, with this rename, all the SDK-style projects (.builds) - which only need the Directory.Build.targets in the parent directory - will automatically chain this in, while the non-SDK-style projects (.pkgproj) - which actually need the settings directly in the file - still need to explicitly chain this in.

Everything is now hierarchical. Yes, its not ideal to add pollution to the .builds files, but its consistent, and non-harmful.

elinor-fung · 2019-10-07T23:58:01Z

src/.nuget/Directory.Build.props

+  <Import Project="../Directory.Build.props" />
  <Import Project="packaging.props" />


These two imports are the only things that packages.builds needs from this file. I guess it isn't actually breaking anything right now, but it seems like adding noise.

Everything is now hierarchical. Yes, its not ideal to add pollution to the .builds files, but its consistent, and non-harmful.

elinor-fung · 2019-10-08T00:23:50Z

EmptyProps.props

+<!-- This file is intentionally empty. It is copied by build.cmd into the bin directory as both Directory.Build.props
+     and Directory.Build.targets to prevent CMake built projects from dependending on the Directory.Build infrastructure
+     in the root of the source tree. In particular this was necessary to compile DacTableGen, which is currently compiled
+     against the desktop framework.


Can we configure the generated projects in CMake such that they set ImportDirectoryBuildProps and ImportDirectoryBuildTargets to false?

elinor-fung · 2019-10-08T00:29:50Z

src/.nuget/Directory.Build.props

  <Import Project="packaging.props" />

+  <!-- Allow specialization of the packaging by creating a props file of the same name of the project immediately next to the project -->
+  <Import Condition="Exists('$(MSBuildProjectDirectory)/$(MSBuildProjectName).props')" Project="$(MSBuildProjectDirectory)/$(MSBuildProjectName).props"/>


Did you have a chance to try renaming the Microsoft.NETCore.Native.props instead? I think it should work and it would be more in line with msbuild's existing system for customization.

Yeah, that looks to work.

davidwrighton · 2019-10-09T18:57:21Z

/azp list

azure-pipelines · 2019-10-09T18:57:26Z

CI/CD Pipelines for this repository: coreclr-outerloop coreclr-ci dotnet-coreclr-nullablefeature coreclr-r2r coreclr-gc-longrunning

davidwrighton · 2019-10-09T18:57:45Z

/azp run coreclr-outerloop

azure-pipelines · 2019-10-09T18:58:00Z

Azure Pipelines successfully started running 1 pipeline(s).

davidwrighton · 2019-10-10T00:02:02Z

@elinor-fung could you take a look and confirm that your concerns have been addressed?

eng/run-test-job.yml

sync.cmd

elinor-fung · 2019-10-11T19:40:37Z

tests/build.proj

@@ -1,14 +1,14 @@
 <?xml version="1.0" encoding="utf-8"?>
 <Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
-  <Import Project="dir.props" />
+  <Import Project="Directory.Build.props" />

  <Import Project="$(ToolsDir)VersionTools.targets" Condition="Exists('$(ToolsDir)VersionTools.targets')" />


Remove? Pretty sure ToolsDir was for BuildTools

elinor-fung · 2019-10-11T19:41:58Z

tests/scripts/scripts.csproj

@@ -27,7 +27,6 @@
    <RuntimeIdentifiers>win7-x86;win7-x64</RuntimeIdentifiers>
    <IsTestProject>false</IsTestProject>
  </PropertyGroup>
-  <Import Project="$(MSBuildThisFileDirectory)..\src\dir.targets" />


Does this need to explicitly pull in a Directory.Build.targets?

elinor-fung · 2019-10-11T19:47:14Z

@davidwrighton yeah, I think I'm generally okay with the product - honestly, did not look hard at the tests...

davidwrighton added 10 commits September 24, 2019 14:17

Initial reduction in dir.props from test/src/dir.props

d7f4ffb

Move more stuff into Directory.Build.props

610398d

Get Directory.Build.props to work for tests entirely

e7d5371

Swap over product build and root directory to Directory.Build.props

ef45ad0

It seems to actually build

2d5aca2

Update EventSource creation to support artifacts intermediates path o…

dd1d68b

…n Unix

Remove unnecessary import of Directories.Build.props

c88b72f

Fix Windows CMake CSharp processing by creating empty vcxproj files

5b848ed

Merge branch 'buildwork' of https://github.com/davidwrighton/coreclr …

cfdbbd0

…into buildwork

Fix CSharp build

c78e5b3

Capture logs from test runs Tweak Directory.Build.props use from runtest.proj and xunitconsolerunner so that it "works"

Merge branch 'master' of https://github.com/dotnet/coreclr into build…

a7dd5b4

…work

davidwrighton force-pushed the buildwork branch from c417065 to a7dd5b4 Compare October 2, 2019 21:15

davidwrighton added 10 commits October 2, 2019 15:52

Fix build-test

c3cfe70

More fixes

65f5305

more fixes to test build and run

dc98e36

Continuing the helix job tweaks

f129236

Use Directory.Build.targets

2bc6b1f

runtest.proj moved, so adjust users of it

cf58dbb

Remove linkbench and perflab

73fd779

Make System.Private.CoreLib.csproj buildable without extra parameters

398f935

Remove the ArcadeBuild property from our build. Its now assumed to al…

644ec67

…ways be true

Optdata doesn't use Directory.Build.targets from the .nuget directory

fab48e9

davidwrighton requested review from jashook and sbomer October 4, 2019 01:57

davidwrighton changed the title ~~[WIP] Buildwork to remove dir.props files~~ Buildwork to remove dir.props files Oct 4, 2019

davidwrighton marked this pull request as ready for review October 4, 2019 01:57

jashook requested review from AaronRobinsonMSFT and elinor-fung October 4, 2019 15:56

jashook reviewed Oct 4, 2019

View reviewed changes

elinor-fung reviewed Oct 7, 2019

View reviewed changes

dir.common.props Outdated Show resolved Hide resolved

dir.common.props Show resolved Hide resolved

davidwrighton added 2 commits October 7, 2019 14:12

Code review comments

d794439

Feedback

b1f37d2

sbomer reviewed Oct 7, 2019

View reviewed changes

elinor-fung reviewed Oct 8, 2019

View reviewed changes

davidwrighton added 2 commits October 8, 2019 16:56

Use Directory.Build.props instead of homegrown init mechanism

d850aa7

Import Directory.Build.props not Directory.Build.props

6e6882b

elinor-fung reviewed Oct 11, 2019

View reviewed changes

eng/run-test-job.yml Outdated Show resolved Hide resolved

elinor-fung reviewed Oct 11, 2019

View reviewed changes

sync.cmd Show resolved Hide resolved

elinor-fung reviewed Oct 11, 2019

View reviewed changes

davidwrighton added 2 commits October 14, 2019 10:35

More feedback

f76ebb2

Errant ;

cc450bd

davidwrighton merged commit 2a17098 into dotnet:master Oct 15, 2019

alpencolt mentioned this pull request Jan 31, 2020

[armel/Tizen] Broken crossgen2 build dotnet/runtime#13586

Closed

		<Import Project="../Directory.Build.props" />
		<Import Project="packaging.props" />

Buildwork to remove dir.props files #26960

Buildwork to remove dir.props files #26960

Conversation

davidwrighton commented Oct 1, 2019

davidwrighton commented Oct 2, 2019

azure-pipelines bot commented Oct 2, 2019

davidwrighton commented Oct 2, 2019

azure-pipelines bot commented Oct 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidwrighton commented Oct 9, 2019

azure-pipelines bot commented Oct 9, 2019

davidwrighton commented Oct 9, 2019

azure-pipelines bot commented Oct 9, 2019

davidwrighton commented Oct 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elinor-fung commented Oct 11, 2019