Add target to set default target manifest #31

JunTaoLuo · 2017-04-17T22:16:00Z

Adding this target to set target manifest used before publish based on the current OS and Platform. The actual manifest.RID.xml will be added to this package during the build when it's generated by dotnet store. For cross-publish scenarios, AspNetCoreTargetManifestRuntimeIdentifier can be set in the project. PublishWithAspNetCoreTargetManifest can be set to false to publish without manifest.

JunTaoLuo · 2017-04-17T22:16:10Z

src/Microsoft.AspNetCore.All/build/PublishWithAspNetCoreTargetManifest.targets

+      <ResolvedRuntimeIdentifier Condition="'$(AspNetCoreTargetManifestRuntimeIdentifier)'=='' and '$(OS)'=='Unix'">linux-x64</ResolvedRuntimeIdentifier>
+    </PropertyGroup>
+
+    <Message


Remove this.

JunTaoLuo · 2017-04-17T22:16:34Z

src/Microsoft.AspNetCore.All/build/PublishWithAspNetCoreTargetManifest.targets

+
+    <PropertyGroup>
+      <ResolvedRuntimeIdentifier>$(AspNetCoreTargetManifestRuntimeIdentifier)</ResolvedRuntimeIdentifier>
+      <ResolvedRuntimeIdentifier Condition="'$(AspNetCoreTargetManifestRuntimeIdentifier)'=='' and '$(OS)'=='Windows_NT' and '$(Platform)'!='x86'">win7-x64</ResolvedRuntimeIdentifier>


Platform can be: x86, x64 and AnyCPU. Not sure if this works as expected and I'll test on an actual 32bit windows machine.

Doesn't quite work, Platform defaults to AnyCPU regardless whether the os is x86 or x64, but PlatformTarget seems to be correct.

natemcmaster · 2017-04-17T23:08:59Z

src/Microsoft.AspNetCore.All/build/PublishWithAspNetCoreTargetManifest.targets

+      <ResolvedRuntimeIdentifier>$(AspNetCoreTargetManifestRuntimeIdentifier)</ResolvedRuntimeIdentifier>
+      <ResolvedRuntimeIdentifier Condition="'$(AspNetCoreTargetManifestRuntimeIdentifier)'=='' and '$(OS)'=='Windows_NT' and '$(Platform)'!='x86'">win7-x64</ResolvedRuntimeIdentifier>
+      <ResolvedRuntimeIdentifier Condition="'$(AspNetCoreTargetManifestRuntimeIdentifier)'=='' and '$(OS)'=='Windows_NT' and '$(Platform)'=='x86'">win7-x86</ResolvedRuntimeIdentifier>
+      <ResolvedRuntimeIdentifier Condition="'$(AspNetCoreTargetManifestRuntimeIdentifier)'=='' and '$(OS)'=='OSX'">osx.10.12-x64</ResolvedRuntimeIdentifier>


On macOS, $(OS) == 'Unix' 😢 . Instead,

$([System.Runtime.InteropServices.RuntimeInformation]::IsOSPlatform($([System.Runtime.InteropServices.OSPlatform]::OSX)))

cref dotnet/msbuild#1926

Wasn't aware that we had access to RuntimeInformation, that makes things a lot easier.

Only in MSBuild 15.3. dotnet-CLI 1.0.0 has MSBuild 15.1. Using it will require users to upgrade VS and dotnet-CLI.

natemcmaster

A couple nits

natemcmaster · 2017-04-17T23:13:08Z

src/Microsoft.AspNetCore.All/Microsoft.AspNetCore.All.csproj

@@ -147,4 +147,9 @@
    <PackageReference Include="Microsoft.Extensions.Logging.AzureAppServices" Version="$(AspNetCoreVersion)" PrivateAssets="None" Condition="'$(BUILD_PACKAGE_CACHE)' == 'true'" />
  </ItemGroup>

+  <ItemGroup>
+    <Content Include="build\**\*.xml" Pack="true" PackagePath="%(Identity)" />


Nit: Pack="true" is default for Content

natemcmaster · 2017-04-17T23:13:15Z

src/Microsoft.AspNetCore.All/build/PublishWithAspNetCoreTargetManifest.targets

@@ -0,0 +1,42 @@
+<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+
+  <!--<Import Project="Microsoft.NET.RuntimeIdentifierInference.targets" />-->


Nit: remove

JunTaoLuo · 2017-04-18T00:06:29Z

🆙📅

natemcmaster · 2017-04-18T00:07:08Z

src/Microsoft.AspNetCore.All/build/PublishWithAspNetCoreTargetManifest.targets

+    Condition="'$(PublishWithAspNetCoreTargetManifest)'=='true' and '$(PublishableProject)'=='true'" >
+
+    <PropertyGroup>
+      <ManifestRuntimeIdentifier>$(AspNetCoreTargetManifestRuntimeIdentifier)</ManifestRuntimeIdentifier>


Should this also pick up $(RuntimeIdentifier)?

It's a bit trickier to pick up RuntimeIdentifier. We will eventually generate runtime package store for win-x64, win-x86, osx-x64, linux-x64 only. We can use the RuntimeIdentifier for cross-publishing scenarios but that would require logic to potentially resolve ubuntu.16.04-x64 to linux-x64.

pranavkm · 2017-04-18T00:08:28Z

src/Microsoft.AspNetCore.All/build/PublishWithAspNetCoreTargetManifest.targets

@@ -0,0 +1,49 @@
+<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">


Do we need the xmlns?

pranavkm · 2017-04-18T00:10:15Z

src/Microsoft.AspNetCore.All/build/PublishWithAspNetCoreTargetManifest.targets

+
+    <PropertyGroup>
+      <ManifestRuntimeIdentifier>$(AspNetCoreTargetManifestRuntimeIdentifier)</ManifestRuntimeIdentifier>
+      <ManifestRuntimeIdentifier Condition="'$(AspNetCoreTargetManifestRuntimeIdentifier)'==''


Can we declare this in a separate propertygroup so we shouldn't have to repeat '$(AspNetCoreTargetManifestRuntimeIdentifier)'==''?

pranavkm · 2017-04-18T00:11:19Z

src/Microsoft.AspNetCore.All/build/PublishWithAspNetCoreTargetManifest.targets

+    <Message
+      Text="Appending default ASP.NET Core runtime package store manifest for use during publish based for the runtime $(ManifestRuntimeIdentifier)."
+      Importance="normal"
+      Condition="'$(ManifestRuntimeIdentifier)'!=''" />


Shouldn't need the condition. The Error should handle this for you.

Wasn't sure if it should be an error. If we couldn't resolve the manifest, maybe we should just give a warning and publish without manifest.

pranavkm · 2017-04-18T00:11:55Z

src/Microsoft.AspNetCore.All/build/PublishWithAspNetCoreTargetManifest.targets

+
+    <Message
+      Text="Appending default ASP.NET Core runtime package store manifest for use during publish based for the runtime $(ManifestRuntimeIdentifier)."
+      Importance="normal"


Low? Is this a useful piece of information for most folks to know?

pranavkm · 2017-04-18T00:12:25Z

src/Microsoft.AspNetCore.All/build/PublishWithAspNetCoreTargetManifest.targets

+      Condition="'$(ManifestRuntimeIdentifier)'!=''" />
+
+    <PropertyGroup>
+      <TargetManifest Condition="'$(ManifestRuntimeIdentifier)'!=''">$(TargetManifest);$(MSBuildThisFileDirectory)manifest.$(ManifestRuntimeIdentifier).xml</TargetManifest>


Don't need the condition

pranavkm · 2017-04-18T00:12:29Z

src/Microsoft.AspNetCore.All/build/PublishWithAspNetCoreTargetManifest.targets

+      <TargetManifest Condition="'$(ManifestRuntimeIdentifier)'!=''">$(TargetManifest);$(MSBuildThisFileDirectory)manifest.$(ManifestRuntimeIdentifier).xml</TargetManifest>
+    </PropertyGroup>
+  </Target>
+</Project>


Trailing newline

pranavkm · 2017-04-18T00:13:37Z

src/Microsoft.AspNetCore.All/Microsoft.AspNetCore.All.csproj

@@ -147,4 +147,9 @@
    <PackageReference Include="Microsoft.Extensions.Logging.AzureAppServices" Version="$(AspNetCoreVersion)" PrivateAssets="None" Condition="'$(BUILD_PACKAGE_CACHE)' == 'true'" />
  </ItemGroup>

+  <ItemGroup>
+    <Content Include="build\**\*.xml" PackagePath="%(Identity)" />


Package="true" and remove PackagePath. Also, what's the xml file that's being added?

I will be adding the manifests. This has to be done in Coherence-PackageCache though since that's when we actually generate the runtime store.

pranavkm · 2017-04-18T00:33:14Z

src/Microsoft.AspNetCore.All/build/netstandard1.6/Microsoft.AspNetCore.All.targets

@@ -0,0 +1,3 @@
+<Project>
+  <Import Project="$(MSBuildThisFileDirectory)..\PublishWithAspNetCoreTargetManifest.targets" />
+</Project>


Trailing new line

JunTaoLuo requested review from pranavkm and natemcmaster April 17, 2017 22:16

dnfclas added the cla-already-signed label Apr 17, 2017

JunTaoLuo commented Apr 17, 2017

View reviewed changes

natemcmaster reviewed Apr 17, 2017

View reviewed changes

natemcmaster reviewed Apr 18, 2017

View reviewed changes

pranavkm suggested changes Apr 18, 2017

View reviewed changes

pranavkm approved these changes Apr 18, 2017

View reviewed changes

JunTaoLuo force-pushed the johluo/publish-target branch from 7664b8b to 3c4ecd9 Compare April 18, 2017 00:35

Add target to set default target manifest

87c513b

JunTaoLuo force-pushed the johluo/publish-target branch from 3c4ecd9 to 87c513b Compare April 18, 2017 00:36

JunTaoLuo merged commit 87c513b into dev Apr 18, 2017

JunTaoLuo deleted the johluo/publish-target branch June 7, 2017 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add target to set default target manifest #31

Add target to set default target manifest #31

JunTaoLuo commented Apr 17, 2017

JunTaoLuo Apr 17, 2017

JunTaoLuo Apr 17, 2017 •

edited

Loading

JunTaoLuo Apr 17, 2017 •

edited

Loading

natemcmaster Apr 17, 2017

JunTaoLuo Apr 17, 2017

natemcmaster Apr 17, 2017

natemcmaster left a comment

natemcmaster Apr 17, 2017

natemcmaster Apr 17, 2017

JunTaoLuo commented Apr 18, 2017

natemcmaster Apr 18, 2017

JunTaoLuo Apr 18, 2017

pranavkm Apr 18, 2017

pranavkm Apr 18, 2017

pranavkm Apr 18, 2017

JunTaoLuo Apr 18, 2017

pranavkm Apr 18, 2017

pranavkm Apr 18, 2017

pranavkm Apr 18, 2017

pranavkm Apr 18, 2017

JunTaoLuo Apr 18, 2017

pranavkm Apr 18, 2017

		@@ -0,0 +1,42 @@
		<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">

		<!--<Import Project="Microsoft.NET.RuntimeIdentifierInference.targets" />-->

		@@ -0,0 +1,49 @@
		<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">

Add target to set default target manifest #31

Add target to set default target manifest #31

Conversation

JunTaoLuo commented Apr 17, 2017

Choose a reason for hiding this comment

JunTaoLuo Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

JunTaoLuo Apr 17, 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

natemcmaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunTaoLuo commented Apr 18, 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

JunTaoLuo Apr 17, 2017 •

edited

Loading

JunTaoLuo Apr 17, 2017 •

edited

Loading