Build with MSBuild #238

natemcmaster · 2016-11-24T00:17:51Z

Please take a look. The migration of project.json to csproj is mostly done. The shared infrastructure around it needs work (e.g. KoreBuild's replacement).

Resolves #179

cc @bricelam

bricelam · 2016-11-29T04:12:38Z

build.xml

+
+    <Target Name="Test">
+        <MSBuild Targets="VSTest"
+                 Projects="@(TestProjects)" />


I think this will run test assemblies in parallel. You may want to use task batching (i.e. Projects="%(TestProjects.Identity)") to prevent this.

Oh, I lied. BuildInParallel defaults to false. But in that case, you may want to consider adding it wherever you use the MSBuild task with multiple projects. It does a topological sort (based on ProjectReference items) and builds the parts in can in parallel.

👍 Would be good if it works well. I'll play it. Not sure how well VSTest handles parallel test projects.

I don't think we should test in parallel. Just build/pack.

bricelam · 2016-11-29T04:16:43Z

build.xml

+    </PropertyGroup>
+    <ItemGroup>
+        <SourceProjects Include="src\*\*.csproj" />
+        <TestProjects Include="test\*\*.csproj" />


I've always wanted the unit tests to run before functional/integration/end-to-end tests... It seems like a good way to "fail fast".

Would be awesome. My MSBuild-ese isn't quite good enough to know the best way to code a good sorting algorithm like that....does the .NET Core MSBuild support code tasks?

does the .NET Core MSBuild support code tasks

Not last I checked.

I'd just do it in two passes.

<ItemGroup> <UnitTestProject Include="test/**/*.Test.csproj" /> <OtherTestProject Include="test/**/*.csproj" Exclude="@(UnitTestProject)" /> </ItemGroup>

Yeah, that's easier. added it.

bricelam · 2016-11-29T16:38:11Z

build.ps1

+    throw 'Restoring packages for build.xml failed'
+}
+& $env:DOTNET_HOME/dotnet.exe msbuild build.xml /nologo /v:m $args


Add /m for a bit more oomph.

ooh, nice. Yeah, let's put that CPU to work

bricelam · 2016-11-29T16:45:12Z

build.xml

+    <Target Name="Pack">
+        <MSBuild Targets="Rebuild;Pack"
+                 Projects="@(SourceProjects)"
+                 Properties="PackageOutputPath=$(ArtifactsPath)" />


May also want to flow Configuration=$(Configuration)

(On other MSBuild usages too.)

So i'm still a little fuzzy on how properties flow form outer to inner builds, but I thought properties like Configuration just automatically flow into inner builds unless explicitly listed in 'RemoveProperties'...

Calling the MSBuild task creates a new context/scope--no properties are preserved unless you pass them here.

AFAIK, the RemoveProperties is used to remove properties specified in the Projects items' Properties and AdditionalProperties metadata.

I ❤️ MSBuild. (In a Stockholm syndrome kind of way)

bricelam · 2016-11-29T16:48:49Z

build.xml

+    </Target>
+
+    <Target Name="Pack">
+        <MSBuild Targets="Rebuild;Pack"


Why force a full rebuild every time?

Good point. Will remove.

davidfowl · 2016-11-29T17:17:36Z

src/Microsoft.DotNet.Watcher.Tools/Microsoft.DotNet.Watcher.Tools.csproj

+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <Import Project="$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props" />
+  <Import Project="..\..\build\common.props" />


Remind me, by default this is relative to the project file right? I still think this should be a package.

by default this is relative to the project file right?

Yes.

I still think this should be a package.

Agreed. As per ongoing discussion in https://github.com/aspnet/specs/pull/49, I fully intend to move most of these common properties to a package. When that happens, we can remove duplication from that file. That said, there is still a high chance we will still want a common.props file per repo for settings that truly are repo specific, such as RepositoryUrl.

this is relative to the project file right

Yes, the project you pass to msbuild, and you use $(MSBuildThisFileDirectory) to reference files relative to the current one.

davidfowl · 2016-11-29T17:23:09Z

src/Microsoft.DotNet.Watcher.Tools/Microsoft.DotNet.Watcher.Tools.csproj

+    <!--<PackageReference Include="Microsoft.Extensions.Process.Sources" Version="1.1.0-*" PrivateAssets="All" />-->
+
+    <!-- ExcludeAssets="build" to workaround https://github.com/dotnet/corefx/issues/12913 -->
+    <PackageReference Include="Microsoft.DiaSymReader.Native" Version="1.4.0" ExcludeAssets="build" PrivateAssets="All" />


What is this for?

It was a workaround for dotnet/roslyn#15137. Just checked again and the workaround can be removed now.

natemcmaster · 2016-11-29T23:51:08Z

⌚ Putting this work on hold. Found some un-workaround-able issues.

microsoft/vstest#241 - dotnet test returns exit code 0 even when tests fail
microsoft/vstest#254 - VSTest breaks TeamCity test reporting

ajaybhargavb · 2016-12-09T23:30:52Z

build.xml

+        <TargetFramework>netcoreapp1.0</TargetFramework>
+    </PropertyGroup>
+    <ItemGroup>
+        <PackageReference Include="NuGetPackageVerifier" Version="1.0.1-*" />


Version 1.0.2-*?

natemcmaster · 2016-12-12T21:22:13Z

🆙 📅 Changing course. Sticking with KoreBuild for now to simplify our conversion process.

Removing wip: label. This PR is ready.

cc @prafullbhosale can you take a look?

prafullbhosale

Looks mostly good.

prafullbhosale · 2016-12-12T21:41:17Z

build/common.props

+    </PropertyGroup>
+
+    <PropertyGroup Condition="$(PackWithoutBuildNumber)">
+        <PackageVersion>$(VersionPrefix)</PackageVersion>


Where is this being set in case PackWithoutBuildNumber == false? or does it not need to be?

In this repo, version.props has it...but you make a good point. This should be made safer so that it can be generalized.

version.props only sets the VersionPrefix and VersionSuffix but not the PackageVersion.
Am I missing something?

Yeah, PackageVersion is in NuGet's pack targets. It defaults to $(Version) but can be overridden if you want the pkg version to differ from the build version.

👍 Did not know that.

prafullbhosale · 2016-12-12T21:46:26Z

src/dir.props

+    <PackageType>DotnetCliTool</PackageType>
+  </PropertyGroup>
+  <ItemGroup>
+      <PackageReference Include="Microsoft.NETCore.App" Version="1.0.0" />


nit: I would rather have this package reference included in every csproj to be explicit.

prafullbhosale · 2016-12-12T21:56:01Z

src/Microsoft.Extensions.Caching.SqlConfig.Tools/Properties/AssemblyInfo.cs

@@ -2,10 +2,5 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

 using System.Reflection;
-using System.Resources;

 [assembly: AssemblyMetadata("Serviceable", "True")]


Is this still required? This seems to be set in the pack options in common.props

yes, still required. This is different from the serviceable tag on the nupkg.

dnfclas added the cla-already-signed label Nov 24, 2016

bricelam reviewed Nov 29, 2016

View reviewed changes

davidfowl suggested changes Nov 29, 2016

View reviewed changes

natemcmaster force-pushed the namc/build-with-msbuild branch from dc8938c to 13e6424 Compare November 29, 2016 18:14

natemcmaster force-pushed the namc/build-with-msbuild branch 2 times, most recently from 1e99fc1 to e71a8f2 Compare December 8, 2016 19:02

ajaybhargavb reviewed Dec 9, 2016

View reviewed changes

natemcmaster force-pushed the namc/build-with-msbuild branch 2 times, most recently from a5abd62 to 180586c Compare December 12, 2016 21:19

natemcmaster changed the title ~~wip: Build with MSBuild~~ Build with MSBuild Dec 12, 2016

prafullbhosale reviewed Dec 12, 2016

View reviewed changes

prafullbhosale approved these changes Dec 12, 2016

View reviewed changes

Build with MSBuild

902ff8d

natemcmaster force-pushed the namc/build-with-msbuild branch from a109f19 to 902ff8d Compare December 12, 2016 23:55

natemcmaster merged commit 902ff8d into feature/msbuild Dec 12, 2016

natemcmaster deleted the namc/build-with-msbuild branch December 12, 2016 23:57

natemcmaster mentioned this pull request Jan 5, 2017

Upgrade to VS 2017 aspnet/Universe#469

Closed

Build with MSBuild #238

Build with MSBuild #238

Conversation

natemcmaster commented Nov 24, 2016 • edited

Choose a reason for hiding this comment

bricelam Nov 29, 2016 • edited

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

bricelam Nov 29, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bricelam Nov 29, 2016 • edited

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 Nov 29, 2016

Choose a reason for hiding this comment

natemcmaster commented Dec 12, 2016

prafullbhosale 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

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 Nov 24, 2016 •

edited

bricelam Nov 29, 2016 •

edited

bricelam Nov 29, 2016 •

edited

bricelam Nov 29, 2016 •

edited