Update to MSBuild #478

pranavkm · 2017-03-07T06:57:55Z

No description provided.

pranavkm · 2017-03-07T15:30:02Z

build/RepositoryBuild.targets

+    <Message Text="============ Done building $(RepositoryToBuild) ============" Importance="High" />
+  </Target>
+
+  <Target Name="_PinVersions">


Summary of what we're doing here: prior to building a repository:

We look at the restore graph spec to determine packages with floating versions

Look at artifacts/build/*.nupkg and .deps/build/*.nupkgto determine if we have a package that has the same{Major.Minor.Build}` number as the floated version

Pin the build if we found something in either of the two packages. The resulting file looks like this:

<Project> <ItemGroup> <PackageReference Update="Microsoft.AspNetCore.Http.Abstractions" Version="1.2.0-preview1-t0041a40df" Condition="'$(TargetFramework)'=='net451'" /> <PackageReference Update="Microsoft.AspNetCore.Http.Extensions" Version="1.2.0-preview1-t0041a40df" Condition="'$(TargetFramework)'=='net451'" /> <PackageReference Update="Microsoft.AspNetCore.WebUtilities" Version="1.2.0-preview1-t0041a40df" Condition="'$(TargetFramework)'=='net451'" /> <PackageReference Update="Microsoft.AspNetCore.Http.Abstractions" Version="1.2.0-preview1-t0041a40df" Condition="'$(TargetFramework)'=='netstandard1.3'" /> <PackageReference Update="Microsoft.AspNetCore.Http.Extensions" Version="1.2.0-preview1-t0041a40df" Condition="'$(TargetFramework)'=='netstandard1.3'" /> <PackageReference Update="Microsoft.AspNetCore.WebUtilities" Version="1.2.0-preview1-t0041a40df" Condition="'$(TargetFramework)'=='netstandard1.3'" /> </ItemGroup> </Project>

We drop this file in the project's obj directory with the right naming so that the sdk's auto include picks it up.

This removes the need for us to juggle around with the feeds to produce a coherent build. It also works around issues like the one Kestrel hit the other day where a higher version package (that we hadn't mirrored) ended up on NuGet.

Could we take this one step farther and replace the PackageReference with a ProjectReference?

Could we try that as a follow up? I wanted to keep the Update as simple as possible for now.

Yeah, I should have been clearer. This is okay for now. Was just wondering if you had looked into ProjectReference. At one point I played with generating an MSBuild file in obj/ that removed PackageReference and added ProjectReference...but I ran into lingering issues in Package vs Project precedence in ResolveAssemblyReferences.

pranavkm · 2017-03-07T15:43:26Z

build/RepositoryBuild.targets

+    <Message Text="============ Building $(RepositoryToBuild) ============" Importance="High" />
+    <Exec
+      Command="./$(BuildScriptToExecute) $(_RepositoryBuildTargets) /p:BuildNumber=$(BuildNumber) /p:Configuration=$(Configuration)"
+      IgnoreStandardErrorWarningFormat="true"


For a full build, MSBuild takes super long to collate all our warnings at the end of the build. aspnet/BuildTools#190 might help, but there's a plethora of warning MSB3277: Found conflicts between different versions of the same dependent assembly that could not be resolved. that I'm not sure we are doing anything about. Turning this off causes us to lose colorization, but I think it's an acceptable cost for faster builds.

This might have been completely unrelated. I think I ran in to this: dotnet/msbuild#1808 (comment)

pranavkm · 2017-03-07T15:50:07Z

build/repo.targets

-    <MSBuild Projects="$(MSBuildProjectFullPath)"
-      Targets="Sake"
-      Properties="SakeTargets=$(_SakeTargets);RepositoryRoot=$(RepositoryRoot)" />
+  <Target Name="BuildRepositories"


This always attempts to produce a coherent build from the list of cloned repositories. Doing this includes fiddling around with repo's NuGet.configs + pinning package versions. We could add a different (default?) target that does what .\build used to do with sake - i.e build the repositories in sequence (ever since we broke build install (aspnet/KoreBuild#17).

That said, it might be simpler to say this repo is primarily a way for our build system to co-ordinate builds rather than for users to build all of AspNet. I'm not sure there's a lot of value in doing this. Thoughts @davidfowl ?

pranavkm · 2017-03-07T15:50:26Z

build/repo.targets

+    <MSBuild
+      Projects="$(MSBuildThisFileDirectory)RepositoryBuild.targets"
+      Targets="BuildRepositories"
+      Properties="


Is there a saner way to do this?

You can build up a list of properties incrementally.
https://github.com/aspnet/KoreBuild/blob/dev/build/targets/KoreBuild.SolutionBuild.targets#L148-L151

pranavkm · 2017-03-07T15:51:44Z

tools/BuildGraph/DGMLFormatter.cs

+
+namespace BuildGraph
+{
+    public class DGMLFormatter : GraphFormatter


Stolen from https://github.com/aspnet/BuildTrain/blob/dev/src/aspnet-coupler/Graph/DgmlGraphFormatter.cs

Any C# code we need to review in detail? Or was the build train code already reviewed thoroughly?

@anurse would know. I know a couple of folks including me looked at it at some point, but I'm not sure how thorough it was.

I'd take a look at the code. BuildTrain didn't really get any eyes because it was very prototypey and then kinda just got abandoned :)

pranavkm · 2017-03-07T15:52:20Z

tools/BuildGraph/Project.cs

+        {
+            var project = new Project(name)
+            {
+                // PackageReferences = MSBuildEval.GetPackageReferences(path),


Will remove

pranavkm · 2017-03-07T16:15:40Z

build/RepositoryBuild.targets

+  <Target Name="_PinVersions">
+    <PropertyGroup>
+      <PinToolBinary>$(RepositoryRoot)tools\PinVersions\bin\$(Configuration)\netcoreapp1.1\PinVersions.dll</PinToolBinary>
+      <PinVersionArgs>$(DotNetPath) $(PinToolBinary) --graph-specs-root &quot;$(_RestoreGraphSpecsDirectory) &quot; -s &quot;$(BuildDir) &quot; &quot;$(BuildRepositoryRoot) &quot;</PinVersionArgs>


dotnet/sdk#951 🤕

natemcmaster · 2017-03-07T17:24:58Z

build/RepositoryBuild.targets

+
+  <Target Name="_BuildRepository" DependsOnTargets="_PinVersions">
+    <PropertyGroup>
+      <BuildScriptToExecute Condition="'$(OS)'!='Windows_NT'">build.sh</BuildScriptToExecute>


Instead of MSBuild => shell script => MSBuild, have you considered going straight to using MSBuild? KoreBuild is designed to key all locations off the RepositoryRoot property.

<MSBuild Projects="$(MSBuildProjectFullPath)" Properties="RepositoryRoot=$(BuildRepositoryRoot)" />

This avoids the pre-flight restore N+1 times for building N repositories, takes advantage of using the MSBuild already loaded in memory, and potentially avoids some thrashing by letting MSBuild orchestrate the entire build top to bottom.

We could do that. My only mild concern was if the repos had out-of-sync versions of KoreBuild (like we did during the Tooling migration). Maybe that's an issue we can deal with when it arises.

I think this should totally be the end goal. If we can do it now that'd be great, but there may be some other vestigial things in the build system that prevent it right now. If we can't do it now though, let's file an issue to track it because I think Nate's right that it'll be significantly faster.

Maybe that's an issue we can deal with when it arises.

That's a good concern. I was actually just thinking about that. I don't expect we will hit this soon. In the long run, I'm hoping we can do something like aspnet/KoreBuild#196. In the short run, we could fall back to invoking build.cmd if we need to.

natemcmaster · 2017-03-07T17:26:35Z

build/RepositoryBuild.targets

+
+    <Copy
+      SourceFiles="@(ArtifactsToCopy)"
+      DestinationFolder="$(BuildDir)" />


You can reduce disk usage by overriding the BuildDir setting for each repo so that it copies the packages into the Universe artifact folder directly.

natemcmaster · 2017-03-07T17:30:05Z

build/repo.targets

+      <Repository Remove="@(RepositoriesToExclude)" />
+    </ItemGroup>
+
+    <Warning Text="KOREBUILD_REPOSITORY_EXCLUDE AND KOREBUILD_REPOSITORY_INCLUDE are specified."


Error? What's the problem here?

I think you want to pick one or the other. It's likely a bad configuration when both are specified. I could make this an Error

Yeah, let's make it an error. Sometimes the only way to find bad configs is to force them to fail.

natemcmaster · 2017-03-07T17:32:02Z

build/repo.targets

+      </_CloneRepository>
+    </ItemGroup>
+
+    <RemoveDir Directories="$(_CloneRepositoryRoot)" Condition="Exists('$(_CloneRepositoryRoot)')" />


Can we not do this? When doing cross-repo work, I work inside the Universe folder. It's nice to repeatedly run universe commands without it deleting all my work.

I can move this to Clean.

Awesome, thanks!

natemcmaster · 2017-03-07T17:51:34Z

build/repo.targets

+      SourceFiles="$(MSBuildThisFileDirectory)Repositories.props"
+      DestinationFiles="$(RepositoryFileWithCommit)" />
+
+    <Exec


Did you consider using MSBuild instead of a console tool to do this? IIUC what the AddCommitInfo file does, you can probably reduce this to something like:

<GetGitCommitInfo WorkingDirectory="$(_CloneRepositoryRoot)"> <Output TaskParameter="CommitHash" PropertyName="_Hash" /> </GetGitCommitInfo> <XmlPoke2 XmlInputPath="$(RepositoryFileWithCommit)" Query="//Repository[@Include='$(RepoName)']/@Commit" Value="$(_Hash)" />

I was under the impression that XmlPoke(2) can't add new attributes.

When you generate "RepositoryFileWithCommit", you could add an empty Commit attribute.

natemcmaster · 2017-03-07T17:53:58Z

tools/AddCommitInfo/Program.cs

+
+        private static string ReadCommitHash(string repositoryPath)
+        {
+            // Based on https://github.com/aspnet/BuildTools/blob/dev/src/Internal.AspNetCore.BuildTools.Tasks/GetGitCommitInfo.cs


I'm against copy + pasting code from other repos. If you don't my suggestion above (implementing this tool in MSbuild instead), let's instead make this tool depend on Internal.AspNetCore.BuildTools.Tasks directly. We can abstract what GetGitCommitInfo to make it more usable if necessary.

natemcmaster · 2017-03-07T17:56:21Z

tools/shared/DotNetMuxer.cs

+using System.IO;
+using System.Runtime.InteropServices;
+
+// Copy of code from https://github.com/aspnet/DotNetTools/blob/047e4a8533bbcdf22831c40dd9d9aaf0c80d1953/shared/DotNetMuxer.cs


Ahhh! We need to put this in a common place. dotnet/extensions#173

I could reference CLI.Utils here instead and use their version of this type. How about that?

Yeah, let's do that for now.

natemcmaster · 2017-03-07T18:07:17Z

I think this is a great step forward. I'm in favor of the work, but I think we'll get even more benefit from the MSBuild upgrade to make our build system MSbuild top-to-bottom. That means instead of 'Exec'-ing to dotnet-run, let's make those tools MSBuild tasks.

The benefits we get:

Makes build components usable.
- Example: your BuildGraph tool could capture the the solution-to-solution build ordering logic in a reusable, abstract way. If implemented as a task, it could be defined in BuildTools and reused in Universe, Coherence, Coherence-Patch, CoreCLR, etc.
- Saves some time on re-compiling the build graph tool for every run of Universe. This tool won't change much.
consistent programming model
unified logging. Tasks can call Log.LogMessage, Log.LogError etc and let MSbuild handle the file logger, coloring, verbosity, etc.
avoiding issues with PATH and .NET Core runtime installation. Tasks compile against .NET Standard and aren't bound to a specific version of .NET Core.
- This opens the door to using MSBuild.exe on Windows. I'm not saying we should as dotnet.exe continues to be the easiest way to ensure we have the right SDKs (for now), but we have some projects that still only work with MSBuild.exe, like Razor. It would be nice if we could just swap to MSBuild.exe for those projects.

analogrelay

LGTM but I haven't been able to look in depth.

analogrelay · 2017-03-07T18:20:38Z

build/Repositories.props

+    <Repository Include="Testing" />
+    <Repository Include="WebSockets" />
+  </ItemGroup>
+</Project>


(totally serious btw)

analogrelay · 2017-03-07T18:22:06Z

build/RepositoryBuild.targets

+
+  <Target Name="_BuildRepository" DependsOnTargets="_PinVersions">
+    <PropertyGroup>
+      <BuildScriptToExecute Condition="'$(OS)'!='Windows_NT'">build.sh</BuildScriptToExecute>


I think this should totally be the end goal. If we can do it now that'd be great, but there may be some other vestigial things in the build system that prevent it right now. If we can't do it now though, let's file an issue to track it because I think Nate's right that it'll be significantly faster.

natemcmaster

Looks good enough for now. I don't want to hold up progress on getting Universe builds onto MSBuild.

dougbu

Agree w/ @natemcmaster. Let's get this in and improve as-needed later.

dougbu · 2017-03-08T06:23:57Z

Universe.sln

+EndProject
+Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "shared", "shared", "{085280EC-7055-426A-BF9C-1B692B9599AB}"
+	ProjectSection(SolutionItems) = preProject
+		tools\shared\DependencyGraphSpecProvider.cs = tools\shared\DependencyGraphSpecProvider.cs


Think I noticed @(Compile) items don't show up in projects. If that's the reason this is here, please file a bug on the project system.

dougbu · 2017-03-08T15:42:51Z

build/RepositoryBuild.targets

+      Projects="@(BatchedRepository)"
+      BuildInParallel="$(BatchBuilds)"
+      Targets="_BuildRepository"
+      Properties="BuildGroup=%(BatchedRepository.BuildGroup)" />


What reads the $(BuildGroup) property?

It's used to batch the builds. Essentially the GroupBy argument.

dougbu · 2017-03-08T15:55:24Z

tools/BuildGraph/DGMLFormatter.cs

+
+namespace BuildGraph
+{
+    public class DGMLFormatter : GraphFormatter


Any C# code we need to review in detail? Or was the build train code already reviewed thoroughly?

pranavkm requested a review from natemcmaster March 7, 2017 06:57

dnfclas added the cla-already-signed label Mar 7, 2017

pranavkm force-pushed the prkrishn/msbuild branch 2 times, most recently from 88abeff to cddc984 Compare March 7, 2017 15:20

pranavkm requested a review from analogrelay March 7, 2017 15:53

pranavkm commented Mar 7, 2017

View reviewed changes

pranavkm force-pushed the prkrishn/msbuild branch from cddc984 to a39f81d Compare March 7, 2017 15:55

Update to MSBuild

27581e4

pranavkm force-pushed the prkrishn/msbuild branch from a39f81d to 27581e4 Compare March 7, 2017 16:15

pranavkm commented Mar 7, 2017

View reviewed changes

natemcmaster reviewed Mar 7, 2017

View reviewed changes

natemcmaster mentioned this pull request Mar 7, 2017

Remove Sake aspnet/KoreBuild#195

Merged

analogrelay reviewed Mar 7, 2017

View reviewed changes

pranavkm requested a review from dougbu March 8, 2017 00:42

Changes per PR comments

a333469

natemcmaster approved these changes Mar 8, 2017

View reviewed changes

dougbu approved these changes Mar 8, 2017

View reviewed changes

pranavkm closed this Mar 8, 2017

pranavkm mentioned this pull request Mar 8, 2017

MSBuild migration follow ups #479

Closed

natemcmaster mentioned this pull request Mar 13, 2017

Upgrade to VS 2017 #469

Closed

pranavkm deleted the prkrishn/msbuild branch September 20, 2017 22:02

Update to MSBuild #478

Update to MSBuild #478

Conversation

pranavkm commented Mar 7, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemcmaster commented Mar 7, 2017

analogrelay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemcmaster left a comment

Choose a reason for hiding this comment

dougbu left a 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