Refactor coreclr packaging #9263

chcosta · 2017-02-02T04:38:24Z

https://github.com/dotnet/core-eng/issues/21

This aligns how coreclr does packaging more closely with recent changes in corefx packaging which also enables building rid split packages on unsupported distros.

@ericstj, can you review this? One of the concepts I introduced was SupportedPackageOSGroups. This was required because the Microsoft.NETCore.Native package only builds on *nix platforms, and I needed some way to express which platforms were supported. Without this concept (or something similar), we would produce a different lineup package when building on Windows for that package than we would on the *nix OS's and that seems problematic to me. Another angle would be to make this an exclusion list. I see some benefit to taking the exclusion list approach in case we build on a new os, but if we do build on a new os, I think there's other work that will need to be done to enable the new platform as well, so being explicit about which platforms we build on isn't terrible.

I had to annotate ProjectReferences in the lineup package with an IsLineupPackage=false "AdditionalProperties" metadata item. This was to prevent that project reference from in turn pulling in the rid packages for the reference and adding them to the runtime.json file. We might want to have some other way to denote this, and it would be nice to move it into buildtools so that it just happens by default, but I didn't want to block on that.

@ellismg, I pushed my changes from the source-build submodule and it looks like I pulled in one of your commits as well, let me know if you want me to exclude that change. 1359351

/cc @ellismg @gkhanna79 @ericstj

This is in line with how the `dotnet` tooling works.

chcosta · 2017-02-02T04:53:25Z

src/.nuget/Microsoft.NETCore.ILAsm/runtime.Linux.Microsoft.NETCore.ILAsm.props

    <PackagePlatforms>x64;</PackagePlatforms>
+    <PackagePlatforms Condition="'$(RuntimeOS)' == 'ubuntu.14.04'">x64;arm;</PackagePlatforms>


@ericstj It's not clear to me if these types of exceptions are needed, I figured I'd pick your brain about this and the armel arch builds as well.

I think we can remove PackagePlatforms. This was an older version of filtering where we dummy-ed out the BuildTarget when PackagePlatforms didn't contain the build Platform. We no longer use this in CoreFx, we should remove it here as well. We're doing more advanced filtering in the builds file.

I don't think I can remove PackagePlatforms yet, because the filtering still exists. That's a slightly more complex fix to remove PackagePlatforms. I'll leave this as-is for now.

You should be able to due to this:https://github.com/dotnet/buildtools/blob/08573119630248c2ee1b636ceaf3ca8fc8a2ecda/src/Microsoft.DotNet.Build.Tasks.Packaging/src/PackageFiles/Packaging.targets#L129-L130

If we remove PackagePlatforms, then we stop producing lineup packages because those packages don't have a PackageTargetRuntime .

Then that means that CoreCLR is not setting PackagePlatform correctlywhen building the identity packages. See https://github.com/dotnet/buildtools/blob/08573119630248c2ee1b636ceaf3ca8fc8a2ecda/src/Microsoft.DotNet.Build.Tasks.Packaging/src/PackageFiles/Packaging.targets#L131-L132.

I've pushed a change to remove PackagePlatforms

ellismg · 2017-02-03T03:10:31Z

build.sh

-                    ;;
-                *)
-            esac
+            if [ "$__msbuildonunsupportedplatform" == "1" ]; then


To save the indentation, you could consider just doing this the *) clause of the case (e.g set __isMSBuildOnNETCoreSupported to __msbuildonunsupportedplatform)

ellismg · 2017-02-03T03:10:45Z

dir.traversal.targets

@@ -18,4 +18,5 @@

  <Target Name="Rebuild" DependsOnTargets="Clean;Build" />

+  <Target Name="Dump" />


Did you intend to remove this.

ericstj · 2017-02-14T00:03:42Z

src/.nuget/Microsoft.NETCore.ILAsm/Microsoft.NETCore.ILAsm.builds

@@ -1,89 +1,11 @@
 <?xml version="1.0" encoding="utf-8"?>
 <Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
-  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), packaging.props))\packaging.props" />


Why the different convention? Couldn't you have named these dir.props and dir.targets in src/.nuget?

I didn't want to cause a conflict with Microsoft.TargetingPackage.Private.CoreClr, which doesn't build using these props / targets. Also, there are enough places where we don't want to import packaging.targets, that we would have to substitute ..\..\..\dir.targets instead, that it seemed ugly and I didn't want to do it.

If you want to have different modes for packages I think it would be clearer to describe those modes with properties rather than importing different targets with names that don't correspond to how they are different.

ericstj · 2017-02-14T00:05:16Z

src/.nuget/packaging.props

+    <OSRid Condition="'$(OSRid)' == '' and '$(_distroRidIndex)' != '-1'">$(__DistroRid.SubString(0, $(_distroRidIndex)))</OSRid>
+    <OSRid Condition="'$(OSRid)' == ''">win10</OSRid>
+    <ArchGroup Condition="'$(ArchGroup)' == '' and '$(_archRidIndex)' != '0'">$(__DistroRid.SubString($(_archRidIndex)))</ArchGroup>
+  </PropertyGroup>


Combine these two groups.

ericstj · 2017-02-14T00:11:16Z

src/.nuget/Microsoft.NETCore.Native/Microsoft.NETCore.Native.pkgproj

-    </ProjectReference>
-  </ItemGroup>
+
+  <Import Condition="'$(_packageTargetOSGroup)' != ''" Project="$(MSBuildThisFileDirectory)runtime.$(_packageTargetOSGroup).$(MSBuildProjectName).props" />


Why import here rather than in packaging.props (dir.props)

Not every project has a runtime.$(_packageTargetOSGroup).$(MSBuildProjectName).props file. I could also condition on that existing, but then I was worried that we would hide a valid missing file if we're bringing up a new osgroup support or something.

ericstj · 2017-02-14T00:12:00Z

src/.nuget/packaging.targets

+  <Target Name="FilterProjects" BeforeTargets="Build">
+    <Error Condition="'$(PackageRID)' == ''" Text="'PackageRID' property must be specified."/>
+
+    <!-- Only build packages for current RID -->


Current RID or non-RID-specific.

…tor_packaging

ericstj · 2017-02-14T16:59:38Z

src/.nuget/Microsoft.NETCore.ILAsm/Microsoft.NETCore.ILAsm.pkgproj

@@ -16,5 +16,5 @@

  <Import Condition="'$(_packageTargetOSGroup)' != ''" Project="$(MSBuildThisFileDirectory)runtime.$(_packageTargetOSGroup).$(MSBuildProjectName).props" />

-  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <Import Project="$(MSBuildThisFileDirectory)..\..\..\dir.targets" />


Why not GetDirectoryNameOfFileAbove?

we don't want ..\..\dir.targets, we want ..\..\..\dir.targets. This was part of the reason I had named ..\..\dir.targets as ..\..\packaging.targets.

ericstj · 2017-02-14T16:59:49Z

src/.nuget/Microsoft.NETCore.ILDAsm/Microsoft.NETCore.ILDAsm.pkgproj

@@ -15,5 +15,5 @@

  <Import Condition="'$(_packageTargetOSGroup)' != ''" Project="$(MSBuildThisFileDirectory)runtime.$(_packageTargetOSGroup).$(MSBuildProjectName).props" />

-  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <Import Project="$(MSBuildThisFileDirectory)..\..\..\dir.targets" />


Why not GetDirectoryNameOfFileAbove?

Same goes for other instances of the same thing.

ericstj · 2017-02-15T16:41:54Z

src/.nuget/Microsoft.NETCore.ILDAsm/runtime.OSX.Microsoft.NETCore.ILDAsm.props

@@ -19,11 +12,10 @@
    <AdditionalLibPackageExcludes Include="%2A%2A\%2A.dwarf" />
    <AdditionalSymbolPackageExcludes Include="%2A%2A\%2A.dylib" />
    <AdditionalSymbolPackageExcludes Include="%2A%2A\%2A.dll" />
-    <ArchitectureSpecificNativeSymbol Include="..\..\_.pdb" />
+    <ArchitectureSpecificNativeSymbol Include="..\_.pdb" />


Nit: we should have a property for this sort of thing. I think we did in corefx. That can be addressed separately. /cc @dagood

ericstj · 2017-02-15T16:46:36Z

src/.nuget/Microsoft.NETCore.Runtime.CoreCLR/Microsoft.NETCore.Runtime.CoreCLR.pkgproj

+
+  <Import Condition="'$(_packageTargetOSGroup)' != ''"  Project="$(MSBuildThisFileDirectory)runtime.$(_packageTargetOSGroup).$(MSBuildProjectName).props" />
+
+  <Import Project="$(MSBuildThisFileDirectory)..\..\..\dir.targets" />


Did you look into using some sort of property to differentiate behavior, rather than importing different targets? I really think this type of difference is confusing, especially with no comments. We should have a consistent pattern followed by all projects.

I renamed src\.nuget\dir.targets to src\.nuget\dir.traversal.targets, which aligns closer with its functionality and allows us to use the GetDirectoryNameOfFileAbove syntax to import either dir.targets or dir.traversal.targets. This is similar to how imports were handled in CoreFx for pkgproj vs builds imports.

ericstj

This looks good with the exception of the targets import paths.

Refactor coreclr packaging Commit migrated from dotnet/coreclr@371d608

ellismg and others added 10 commits January 24, 2017 10:51

Only consider /etc/os-release for distro info

1359351

This is in line with how the `dotnet` tooling works.

Refactor coreclr packaging

9497910

Default runtimeos, remove unused variables from jit pkgproj

b7bd7ff

Cleanup jit package, remove redundant includes

91dafc4

Use __distrorid, distrorid is not set on osx

12a37e3

add arm packageplatform for ubuntu 14.04 and 16.04

f20878d

Add missing dependency to ilasm / ildasm packages, parse distrorid

9d8b1f4

Break projectreference traversal when building a lineup package

ef57f44

Fix index logic bug

5bfce2c

Update usage info

b93cbfe

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

chcosta commented Feb 2, 2017

View reviewed changes

chcosta added 2 commits February 2, 2017 11:37

default derivedpackagetargetosgroup to __buildos

8ce3fab

tweak derivedpackagetargetosgroup definition

458a972

ellismg reviewed Feb 3, 2017

View reviewed changes

dagood mentioned this pull request Feb 4, 2017

Add argument that exclusively enables symbol stripping #9326

Closed

ericstj reviewed Feb 14, 2017

View reviewed changes

chcosta added 2 commits February 13, 2017 17:10

Respond to PR feedback

6458745

Merge branch 'master' of https://github.com/dotnet/coreclr into refac…

cdb4728

…tor_packaging

ericstj reviewed Feb 14, 2017

View reviewed changes

chcosta added 5 commits February 14, 2017 10:46

Always specify a default for derivedpackagetargetosgroup

7f2137c

Default distrorid on osx

03a5ce1

Fix typo

409ba7e

Add arch to distrorid

f361873

Remove PackagePlatforms property

8dcf966

chcosta added 2 commits February 14, 2017 16:14

Exclude binaries not built on armel

6a54a44

change package_platform to platform

5623099

ericstj reviewed Feb 15, 2017

View reviewed changes

ericstj approved these changes Feb 15, 2017

View reviewed changes

Rename dir.targets -> dir.traversal.targets

3ec9032

chcosta merged commit 371d608 into dotnet:master Feb 15, 2017

ericstj mentioned this pull request Mar 1, 2017

[master] Updating CoreFx, CoreClr, Standard to beta-25022-02, beta-25101-04, beta-25101-03 respectively dotnet/core-setup#1604

Merged

janvorli mentioned this pull request Mar 28, 2017

Fixed tests build on Linux #10532

Merged

karelz modified the milestone: 2.0.0 Aug 28, 2017

ericstj mentioned this pull request Jan 31, 2020

Crossgen is in the wrong folder in arm package. dotnet/runtime#7531

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/coreclr#9263 from chcosta/refactor_packaging

cf7eac7

Refactor coreclr packaging Commit migrated from dotnet/coreclr@371d608

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor coreclr packaging #9263

Refactor coreclr packaging #9263

chcosta commented Feb 2, 2017

chcosta Feb 2, 2017

ericstj Feb 14, 2017

chcosta Feb 14, 2017

ericstj Feb 14, 2017

chcosta Feb 14, 2017

ericstj Feb 14, 2017 •

edited

Loading

chcosta Feb 14, 2017

ellismg Feb 3, 2017

ellismg Feb 3, 2017

ericstj Feb 14, 2017

chcosta Feb 14, 2017

ericstj Feb 14, 2017

ericstj Feb 14, 2017

ericstj Feb 14, 2017

chcosta Feb 14, 2017

ericstj Feb 14, 2017

ericstj Feb 14, 2017

chcosta Feb 14, 2017

ericstj Feb 14, 2017

ericstj Feb 14, 2017

ericstj Feb 15, 2017

ericstj Feb 15, 2017

chcosta Feb 15, 2017

ericstj left a comment

		<PackagePlatforms>x64;</PackagePlatforms>
		<PackagePlatforms Condition="'$(RuntimeOS)' == 'ubuntu.14.04'">x64;arm;</PackagePlatforms>

		@@ -18,4 +18,5 @@

		<Target Name="Rebuild" DependsOnTargets="Clean;Build" />

		<Target Name="Dump" />


		<Import Condition="'$(_packageTargetOSGroup)' != ''" Project="$(MSBuildThisFileDirectory)runtime.$(_packageTargetOSGroup).$(MSBuildProjectName).props" />

		<Import Project="$(MSBuildThisFileDirectory)..\..\..\dir.targets" />

Refactor coreclr packaging #9263

Refactor coreclr packaging #9263

Conversation

chcosta commented Feb 2, 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

ericstj Feb 14, 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

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

ericstj left a comment

Choose a reason for hiding this comment

ericstj Feb 14, 2017 •

edited

Loading