MSBuildifying gulpified node #237

moozzyk · 2017-02-25T00:06:08Z

Creating an MSBuild project for the TS client
Adding project references to the TS client project from projects that need the client - (ensures the correct targets dependency graph and prevents building the client multiple times and related races)
Removing gulp tasks from individual projects (allows containing npm only in the TS client source and node tests)
Using incremental compilation to build the TS client only when inputs change (prevents building the client multiple times or when not needed at all)
Removing npm install from all the projects (takes up to 10 seconds even if there is nothing to restore) - npm packages will still be installed when running full build (if needed) or need to be installed manually

moozzyk · 2017-02-25T00:14:39Z

~~VS does not like this change...~~

moozzyk · 2017-02-25T02:40:09Z

VS should be fixed now.

moozzyk · 2017-02-25T02:40:56Z

src/Microsoft.AspNetCore.SignalR.Client.TS/Microsoft.AspNetCore.SignalR.Client.TS.csproj

+    <RemoveDir Directories="$(SignalRClientDistFolder)" />
+  </Target>
+
+  <Import Project="$(MSBuildThisFileDirectory)\..\..\build\repo.targets" />


This import is no longer needed.

Yes, remove this.

moozzyk · 2017-02-26T17:41:30Z

src/Microsoft.AspNetCore.SignalR.Client.TS/Microsoft.AspNetCore.SignalR.Client.TS.csproj

+
+  <Import Project="..\..\build\common.props" />
+  <PropertyGroup>
+    <TargetFramework>netstandard1.0</TargetFramework>


We probably need: <IsPackable>false</IsPackable>

Why does a typescript project need a target framework?

I needed to pretend it's a csproj otherwise project references did not work.

And it could not be open in VS

davidfowl · 2017-02-27T16:45:44Z

src/Microsoft.AspNetCore.SignalR.Client.TS/Microsoft.AspNetCore.SignalR.Client.TS.csproj

@@ -0,0 +1,34 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project Sdk="Microsoft.NET.Sdk">


Is this even a typescript project?

Probably don't need to reference Microsoft.NET.Sdk.

Similar to above - it makes project references work.

To clarify - currently there is no Typescript support/tooling for VS2017. I wanted to see what they did in previous versions and if you create a "Typescript project" in VS2015 it will be a *.csproj. I tried using their MSBuild tasks from VS2015 but they are compiled against .NET Framework and "our" MSBuild could not load them. When they have tools for VS2017 and netcore MSBuild tasks we need to make sure that there is an easy way to use the latest version of the compiler (i.e. you don't have to reinstall typescript MSBuild/tools if you want to move to a new compiler version)

cref microsoft/TypeScript#11

See also https://github.com/dotnet/cli/issues/4785#issuecomment-272315965 cc @mlorbetske

davidfowl · 2017-02-27T16:46:09Z

Does this still work with VS code?

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

build/repo.targets

-  </Target>
-
-  <Target Name="RestoreNpm" AfterTargets="Restore">
+  <Target Name="RestoreNpm">


Why did you remove this from the lifecycle?

It's a mistake. I originally depended on this target directly but I decided not to do this anymore because it adds at least a few secs to the build even if there is nothing to install. Will revert this change.

natemcmaster · 2017-02-27T17:05:21Z

samples/ChatSample/ChatSample.csproj

@@ -15,6 +15,7 @@

  <ItemGroup>
    <ProjectReference Include="..\..\src\Microsoft.AspNetCore.SignalR\Microsoft.AspNetCore.SignalR.csproj" />
+    <ProjectReference Include="..\..\src\Microsoft.AspNetCore.SignalR.Client.TS\Microsoft.AspNetCore.SignalR.Client.TS.csproj" />


Try adding ReferenceOutputAssembly="false" to all ProjectReference to the TS project. This will suppress NuGet attempting to use the project reference as a compile reference.

Turned out that I can't do that. With this settings I can build from command line but when I build with VS it fails to build the project due to:
Cannot find project info for 'C:\Source\SignalR-Core\src\Microsoft.AspNetCore.SignalR.Client.TS\Microsoft.AspNetCore.SignalR.Client.TS.csproj'. This can indicate a missing project reference.

natemcmaster · 2017-02-27T17:06:11Z

src/Microsoft.AspNetCore.SignalR.Client.TS/Microsoft.AspNetCore.SignalR.Client.TS.csproj

+    <RemoveDir Directories="$(SignalRClientDistFolder)" />
+  </Target>
+
+  <Import Project="$(MSBuildThisFileDirectory)\..\..\build\repo.targets" />


Yes, remove this.

natemcmaster · 2017-02-27T17:07:00Z

src/Microsoft.AspNetCore.SignalR.Client.TS/Microsoft.AspNetCore.SignalR.Client.TS.csproj

@@ -0,0 +1,34 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project Sdk="Microsoft.NET.Sdk">


Probably don't need to reference Microsoft.NET.Sdk.

natemcmaster · 2017-02-27T17:08:12Z

src/Microsoft.AspNetCore.SignalR.Client.TS/Microsoft.AspNetCore.SignalR.Client.TS.csproj

+  </PropertyGroup>
+
+  <ItemGroup>
+    <Inputs Include="*.ts;" />


Not sure how well VS does with this project. You might try using None instead so VS recognizes the files.

VS showed the files just fine.

I was also able to build the ts client.

natemcmaster · 2017-02-27T17:09:46Z

src/Microsoft.AspNetCore.SignalR.Client.TS/Microsoft.AspNetCore.SignalR.Client.TS.csproj

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


Nit: suggest changing extension to .proj. This isn't a C# project.

I started with .proj but VS refused to add it to the solution. Hence changing to project to pretend it's a .csproj

moozzyk · 2017-02-27T17:35:25Z

@davidfowl - VS Code should work. I plugged in things to MSBuild but under the cover it sill invokes npm/gulp to build things. This has not changed.

natemcmaster · 2017-02-27T17:37:33Z

My only hesitation to this is that we're abusing what ProjectReference and Microsoft.NET.Sdk are meant to do, and this may be fragile. There are other ways to enforce build dependencies that may be cleaner, such as adding a BeforeBuild target to invoke MSBuild on the TS project, and using Project Dependencies in the solution to ensure build order.

moozzyk · 2017-02-27T17:48:28Z

@natemcmaster - this needs to be a .csproj because VS otherwise won't open it why using BeforeBuild (which I believe adds magic and requires creating additional targets in each project to have it under control) is better than a project dependency?
A bit more context: TypeScript tools are not available for VS2017 and in VS2015 a typescript project is... a .csproj (to be fair, it creates a host which is the cs part as far as I understand but apparently there isn't really a TypeScript project in the ecosystem AFAICT)

moozzyk · 2017-02-28T19:05:32Z

🆙📅

natemcmaster · 2017-02-28T21:24:54Z

src/Microsoft.AspNetCore.SignalR.Client.TS/Microsoft.AspNetCore.SignalR.Client.TS.csproj

+    <Exec Command="npm run gulp -- --gulpfile $(MSBuildThisFileDirectory)gulpfile.js build-ts-client" />
+  </Target>
+
+  <Target Name="Clean">


This target will be overridden by Microsoft.NET.Sdk because this project has implicit top/bottom imports. If you want to override this target, use explicit imports instead. Example:

<Project> <Import Project="Sdk.props" Sdk="Microsoft.NET.Sdk" />  <Import Project="Sdk.targets" Sdk="Microsoft.NET.Sdk" />  <Target Name="Clean" /> <Target Name="Build" /> </Project>

Adding this results in the following build warnings:
warning MSB4011: "C:\Users\XXXX\AppData\Local\Microsoft\dotnet\sdk\1.0.0-rc4-004913\Sdks\Microsoft.NET.Sdk\Sdk\Sdk.targets" cannot be imported again. It was already imported at "C:\source\SignalR-Core\src\Microsoft.AspNetCore.SignalR.Client.TS\Microsoft.AspNetCore.SignalR.Client.TS.csproj (30,3)". This is most likely a build authoring error. This subsequent import will be ignored.

Does your file say <Project> or <Project Sdk="microsoft.net.sdk">?

Oh, I see - it did say <Project Sdk="microsoft.net.sdk"> - I did not notice it that it should have been removed.

Without Sdk attribute on Project it does work indeed. I decided however to use AfterTargets. The equivalence between the Sdk attribute and Imports is magic and we cannot override the Build target anyways.

natemcmaster · 2017-02-28T21:25:37Z

test/Microsoft.AspNetCore.SignalR.Test.Server/Microsoft.AspNetCore.SignalR.Test.Server.csproj

-    <TargetFrameworks>netcoreapp1.1;net46</TargetFrameworks>
+    <TargetFrameworks>netcoreapp1.1</TargetFrameworks>
+    <PreserveCompilationContext>true</PreserveCompilationContext>
+    <OutputType>Exe</OutputType>


PreserveCompilationContext and OutputType are not required. These are the default settings for Microsoft.NET.Sdk.Web projects.

natemcmaster · 2017-02-28T21:25:54Z

test/Microsoft.AspNetCore.SignalR.Test.Server/Microsoft.AspNetCore.SignalR.Test.Server.csproj

@@ -3,38 +3,31 @@
  <Import Project="..\..\build\common.props" />

  <PropertyGroup>
-    <TargetFrameworks>netcoreapp1.1;net46</TargetFrameworks>
+    <TargetFrameworks>netcoreapp1.1</TargetFrameworks>


Nit: make this singular TargetFramework

analogrelay · 2017-02-28T22:09:43Z

Given that there aren't TypeScript tools for 2017 yet, I'm OK with using a csproj here. Let's be prepared to move over to a tsproj-based system when it releases though.

- Creating an MSBuild project for the TS client - Adding project references to the TS client project from projects that need the client - (ensures the correct targets dependency graph and prevents building the client multiple times and related races) - Removing gulp tasks from individual projects (allows containing npm only in the TS client source and node tests) - Using incremental compilation to build the TS client only when inputs change (prevents building the client multiple times or when not needed at all) - Removing `npm install` from all the projects (takes up to 10 seconds even if there is nothing to restore) - npm packages will still be installed when running full build (if needed) or need to be installed manually

dnfclas added the cla-already-signed label Feb 25, 2017

moozzyk requested review from analogrelay, davidfowl and natemcmaster February 25, 2017 00:07

moozzyk commented Feb 25, 2017

View reviewed changes

moozzyk commented Feb 26, 2017

View reviewed changes

davidfowl reviewed Feb 27, 2017

View reviewed changes

natemcmaster reviewed Feb 27, 2017

View reviewed changes

moozzyk mentioned this pull request Feb 28, 2017

Integrate TS client into SignalR build and solution #251

Closed

moozzyk force-pushed the pawelka/ts-msbuild branch from 412e370 to 6ae650c Compare February 28, 2017 18:58

moozzyk force-pushed the pawelka/ts-msbuild branch from 6ae650c to 08df7b3 Compare February 28, 2017 21:23

natemcmaster reviewed Feb 28, 2017

View reviewed changes

analogrelay approved these changes Feb 28, 2017

View reviewed changes

natemcmaster approved these changes Mar 1, 2017

View reviewed changes

moozzyk force-pushed the pawelka/ts-msbuild branch from 61cfc68 to 75b8249 Compare March 1, 2017 19:04

Merge branch 'dev' into pawelka/ts-msbuild

3f42e91

moozzyk merged commit 0162c19 into dev Mar 1, 2017

moozzyk deleted the pawelka/ts-msbuild branch March 4, 2017 03:10

		@@ -0,0 +1,34 @@
		<?xml version="1.0" encoding="utf-8"?>
		<Project Sdk="Microsoft.NET.Sdk">

MSBuildifying gulpified node #237

MSBuildifying gulpified node #237

Conversation

moozzyk commented Feb 25, 2017

moozzyk commented Feb 25, 2017 • edited Loading

moozzyk commented Feb 25, 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

natemcmaster Feb 27, 2017 • edited Loading

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

davidfowl commented Feb 27, 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

natemcmaster Feb 27, 2017 • edited Loading

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

moozzyk commented Feb 27, 2017

natemcmaster commented Feb 27, 2017

moozzyk commented Feb 27, 2017 • edited Loading

moozzyk commented Feb 28, 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

analogrelay commented Feb 28, 2017

moozzyk commented Feb 25, 2017 •

edited

Loading

natemcmaster Feb 27, 2017 •

edited

Loading

natemcmaster Feb 27, 2017 •

edited

Loading

moozzyk commented Feb 27, 2017 •

edited

Loading