Port MSBuild targets from Korebuild into an SDK package #145

natemcmaster · 2017-01-10T19:53:39Z

Fixes the requirement for running 'build initialize' before opening aspnet repos in VS 2017.

Changes:
Instead of our repos importing MSBuild targets directly from the korebuild download folder, we will use NuGet to distribute these shared properties and targets.

More context:
MSBuild is working on adding first-class support for distributing SDKs via NuGet. (See dotnet/msbuild#1493). When this is in, we may be able to use that instead of a PackageReference.

cc @Eilon for naming stuff
cc @pranavkm for implementation review

BrennanConroy · 2017-01-10T20:44:47Z

.editorconfig

@@ -0,0 +1,27 @@
+; EditorConfig to support per-solution formatting.


Remove this file?

This is actually a new VS2017 thing. cref https://blogs.msdn.microsoft.com/dotnet/2016/12/15/code-style-configuration-in-the-vs2017-rc-update/

BrennanConroy · 2017-01-10T21:48:58Z

src/Internal.AspNetCore.Sdk/build/Internal.AspNetCore.Sdk.targets

@@ -0,0 +1,8 @@
+<!--
+WARNING: These targets are indented for building Microsoft's ASP.NET Core repos, and is not intended


Is indented supposed to be intended

Oops. yeah.

BrennanConroy · 2017-01-10T21:58:58Z

global.json

@@ -0,0 +1,5 @@
+{
+    "sdk": {
+        "version": "1.0.0-preview4-004233"


global.json in an msbuild project?

Yup. global.json also used by the dotnet.exe muxer to determine SDK version.

Eilon · 2017-01-10T21:57:53Z

src/Internal.AspNetCore.Sdk/Internal.AspNetCore.Sdk.nuspec

+    <version>1.0.0-$versionsuffix$</version>
+    <authors>Microsoft</authors>
+    <description>Build targets and customizations to Microsoft.NET.Sdk</description>
+    <!-- TODO use contentFiles for cs files-->


This work item is tracked somewhere?

It's part of aspnet/KoreBuild#150

Eilon · 2017-01-10T21:58:41Z

src/Internal.AspNetCore.Sdk/build/AssemblyInfo.Serviceable.cs

+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+[assembly: System.Reflection.AssemblyMetadata("Serviceable", "True")]


Is this needed? There's not supposed to be a DLL in this project, right?

There is not .dll for the SDK. This file is added to the <Compile> items.

Is it needed? Oh wait, is this for all our regular projects to consume (i.e. we can delete this from all the other repos)? Or is this actually compiled into a DLL right here?

Right. This is for regular projects. we started deleting AssemblyInfo.cs from our projects because all of its content is generated by Microsoft.NET.SDK (with the exception of the this serviceable attrbute).

Example: https://github.com/aspnet/FileSystem/pull/252/files#diff-b2a717cca5ec711b2ea6505d9b2ead9b

BrennanConroy · 2017-01-10T22:01:25Z

src/Internal.AspNetCore.Sdk/targets/BuildNumber.targets

+    <BuildNumber Condition="'$(BuildNumber)'==''">$(_SecondBasedTimeStamp)</BuildNumber>
+    <VersionSuffix Condition="'$(VersionSuffix)'!='' AND '$(BuildNumber)' != ''">$(VersionSuffix)-$(BuildNumber)</VersionSuffix>
+    <VersionSuffix Condition="'$(VersionSuffix)'==''">$(BuildNumber)</VersionSuffix>
+    <PackWithoutBuildNumber Condition="'$(PackWithoutBuildNumber)'==''">false</PackWithoutBuildNumber>


Is the CI setup to use this?

Not yet. At the moment, CI unzips *.nupkg files, edits the nuspec xml, and re-zips with the timestamp-free version. This piece here will let us set the version numbers correctly upfront.

We're likely not going to do that. We almost never want to publish timestamp free builds until they're verified which this allows. But leaving this in doesn't hurt.

BrennanConroy · 2017-01-10T22:27:44Z

makefile.shade

+#pack-sdk target='package'
+    @{
+    	var buildNumber = E("BuildNumber") ?? BuildNumber;
+        NugetPack(E("KOREBUILD_NUGET_EXE") ?? ".build/NuGet.exe",


nit: extra space

BrennanConroy · 2017-01-10T22:28:27Z

🚢 🇮🇹 after minor thing(s)

pranavkm · 2017-01-10T22:45:04Z

NuGet.Config

@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="utf-8"?>
 <configuration>
  <packageSources>
-    <add key="AspNetVNext" value="https://www.myget.org/F/aspnetcidev/api/v3/index.json" />
+    <add key="AspNetVNext" value="https://dotnet.myget.org/F/aspnetcore-ci-dev/api/v3/index.json" />


<add key="AspNetCore" value="https://dotnet.myget.org/F/aspnetcore-ci-dev/api/v3/index.json" /> to be consistent with other repos. That said, does this actually need to reference our dev feed? Can we get away pointing to aspnet-master or something like that?

Actually, it looks like this project doesn't need anything from the nightly feed. I'll just remove altogether.

pranavkm · 2017-01-10T22:46:55Z

src/Internal.AspNetCore.Sdk/build/Workarounds.targets

@@ -0,0 +1,49 @@
+<!--
+WARNING: These targets are indented for building Microsoft's ASP.NET Core repos, and is not intended


typo: These targets are ~~indented~~ intended for

pranavkm · 2017-01-10T22:52:01Z

src/Internal.AspNetCore.Sdk/targets/BuildNumber.targets

+      Create second-based build number for local builds.
+      635556672000000000 is Jan 1, 2015.
+    -->
+    <_SecondBasedTimeStamp>$([System.DateTime]::UtcNow.Subtract($([System.DateTime]::FromBinary(635556672000000000))).TotalSeconds.ToString("F0"))</_SecondBasedTimeStamp>


Does <_Timestamp>$([System.DateTime]::UtcNow.Ticks.ToString("x").PadLeft(20, '0')</_Timestamp> work? I know they removed the special version length restriction in recent builds of NuGet, so we could simplify this.

Might work. But I still prefer the shorter version.

Plus, it's possible some might build with NuGet 4.0 but need to install to project.json which uses NuGet 3.5. I'm not confident the version-length restriction is fixed there.

pranavkm · 2017-01-10T22:53:33Z

src/Internal.AspNetCore.Sdk/targets/BuildNumber.targets

+    <_SecondBasedTimeStamp>t$([System.Int64]::Parse($(_SecondBasedTimeStamp)).ToString("x9"))</_SecondBasedTimeStamp>
+
+    <!-- for aspnet CI -->
+    <BuildNumber Condition="'$(BuildNumber)'==''">$(KOREBUILD_BUILD_NUMBER)</BuildNumber>


Let's just modify the CI to specify /p:BuildNumber=%build.number% and remove references to KoreBuild here.

Too early. We can do this when we migrate everything to MSbuild.

pranavkm · 2017-01-10T22:55:43Z

src/Internal.AspNetCore.Sdk/targets/BuildNumber.targets

+    <BuildNumber Condition="'$(BuildNumber)'==''">$(_SecondBasedTimeStamp)</BuildNumber>
+    <VersionSuffix Condition="'$(VersionSuffix)'!='' AND '$(BuildNumber)' != ''">$(VersionSuffix)-$(BuildNumber)</VersionSuffix>
+    <VersionSuffix Condition="'$(VersionSuffix)'==''">$(BuildNumber)</VersionSuffix>
+    <PackWithoutBuildNumber Condition="'$(PackWithoutBuildNumber)'==''">false</PackWithoutBuildNumber>


We're likely not going to do that. We almost never want to publish timestamp free builds until they're verified which this allows. But leaving this in doesn't hurt.

pranavkm · 2017-01-10T23:00:17Z

src/Internal.AspNetCore.Sdk/build/Workarounds.targets

+            <_PackageReferences Include="@(_ResolvedFloatingVersion)" Condition="'%(Identity)'=='%(_ResolvedFloatingVersion.ResolvedName)'" />
+        </ItemGroup>
+
+        <Message Importance="Normal" Text="Proj: %(_ProjectReferences.FileName) %(_ProjectReferences.PackageVersion)" />


Debugging code?

Sure, but it doesn't hurt to leave it. Doesn't show up by default in log output.

Maybe make Importance='Low"?

pranavkm · 2017-01-10T23:01:42Z

src/Internal.AspNetCore.Sdk/build/Workarounds.targets

+    </PropertyGroup>
+    <!-- end workaround -->
+
+    <!-- workaround https://github.com/NuGet/Home/issues/4063 -->


Is there an RC3 build we could use that addresses these workarounds?

Not yet. We're still stuck on RC.2 because of blocking issues.

pranavkm · 2017-01-10T23:02:51Z

makefile.shade

+          "src/Internal.AspNetCore.Sdk/Internal.AspNetCore.Sdk.nuspec",
+          BUILD_DIR,
+          "-Verbosity Detailed"
+          + " -Properties versionsuffix=alpha-" + buildNumber);


Weird formatting. Can you move this to the previous line?

pranavkm · 2017-01-10T23:07:33Z

build/common.props

+
+  <PropertyGroup>
+    <!-- these imports substitute for a package reference to Internal.AspNetCore.Sdk -->
+    <CustomBeforeMicrosoftCommonProps>$(MSBuildThisFileDirectory)..\src\Internal.AspNetCore.Sdk\build\Internal.AspNetCore.Sdk.props</CustomBeforeMicrosoftCommonProps>


<_SdkProjectDirectory>$(MSBuildThisFileDirectory)..\src\Internal.AspNetCore.Sdk</_SdkProjectDirectory> and reuse. Probably better if we do

$([MSBuild]::GetDirectoryNameOfFileAbove($(MsBuildThisFileDirectory), NuGet.config)) since it makes multiple levels of folder hierarchy possible without hardcoding the relative directory structure

pranavkm · 2017-01-10T23:09:56Z

makefile.shade

+    @{
+    	var buildNumber = E("BuildNumber") ?? BuildNumber;
+        NugetPack(E("KOREBUILD_NUGET_EXE") ?? ".build/NuGet.exe",
+          "src/Internal.AspNetCore.Sdk/Internal.AspNetCore.Sdk.nuspec",


Why can't we dotnet pack the csproj?

There is no csproj.

natemcmaster requested review from pranavkm and Eilon January 10, 2017 19:53

dnfclas added the cla-already-signed label Jan 10, 2017

natemcmaster mentioned this pull request Jan 10, 2017

Remove project-targets from KoreBuild aspnet/KoreBuild#155

Merged

natemcmaster requested review from BrennanConroy and removed request for pranavkm January 10, 2017 20:40

BrennanConroy reviewed Jan 10, 2017

View reviewed changes

Eilon reviewed Jan 10, 2017

View reviewed changes

BrennanConroy reviewed Jan 10, 2017

View reviewed changes

pranavkm reviewed Jan 10, 2017

View reviewed changes

pranavkm approved these changes Jan 11, 2017

View reviewed changes

Port MSBuild targets from Korebuild into an SDK package

efa9ff1

natemcmaster force-pushed the namc/common-build branch from a826c28 to efa9ff1 Compare January 11, 2017 01:25

natemcmaster merged commit efa9ff1 into dev Jan 11, 2017

natemcmaster pushed a commit to aspnet/PlatformAbstractions that referenced this pull request Jan 11, 2017

React to aspnet/BuildTools#145

2a3f6f2

natemcmaster deleted the namc/common-build branch February 15, 2017 23:36

		@@ -0,0 +1,27 @@
		; EditorConfig to support per-solution formatting.

		@@ -0,0 +1,8 @@
		<!--
		WARNING: These targets are indented for building Microsoft's ASP.NET Core repos, and is not intended

		@@ -0,0 +1,49 @@
		<!--
		WARNING: These targets are indented for building Microsoft's ASP.NET Core repos, and is not intended

Port MSBuild targets from Korebuild into an SDK package #145

Port MSBuild targets from Korebuild into an SDK package #145

Conversation

natemcmaster commented Jan 10, 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

BrennanConroy commented Jan 10, 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