Generate framework manifest file by ViktorHofer · Pull Request #38792 · dotnet/corefx

ViktorHofer · 2019-06-23T11:35:17Z

Requires #36207

The property that points to the manifest which is passed down to the _HandlePackageFileConflicts task is set in CoreFxTesting. The only work necessary here is to generate the manifest file.

ViktorHofer · 2019-06-23T12:13:08Z

Also tested locally with netfx and uap. On netfx it seems to not matter at all, on uap the conflict resolution brings in wrong items but we overwrite those when we run the test by copying the uap runtime assemblies to the output directory.

Directory.Build.props

ericstj · 2019-06-24T15:29:06Z

You need to deal with this hack

corefx/src/shims/manual/Directory.Build.props

Line 17 in 816cd5d

    
           <RefPath>$(ArtifactsBinDir)runtime/$(TargetGroup)-$(_runtimeOSGroup)-$(_bc_ConfigurationGroup)-$(ArchGroup)/</RefPath>

. Also make sure that however you do it, it works well for AllConfigurations build. These manual shims are building against implementation and need to be able to see all implementation assemblies for the current TargetGroup (see https://github.com/dotnet/corefx/issues/36715). 😿

You can probably just persist the hack for the short term to use your test sharedc fx folders, but you can't do that once you start removing things from the test shared fx, since these shims need to forward to assemblies which aren't necessarily inbox.

ViktorHofer · 2019-06-24T15:37:11Z

CoreFxTesting doesn't care about which vertical is currently built and instead uses TargetFrameworkIdentifier. It sounds like the manual shims also only care about the current TargetGroup and not about the vertical. Could that work? a28f360

ericstj · 2019-06-24T16:34:03Z

Could that work?

I haven't looked at all the places that use $(RuntimePath) but I can imagine that the problems with constraining it to target group (vs what it was before) are that it won't have the right value for projects that target netstandard*.

ViktorHofer · 2019-06-25T08:17:04Z

I can imagine that the problems with constraining it to target group (vs what it was before) are that it won't have the right value for projects that target netstandard*.

We currently only use that property for test projects (referenceFormRuntime, Directory.Build.targets and inside ApiCompat.proj). None of those should cause problems.

I linked to my private package in my feed to get CI coverage to be sure nothing regresses.

ViktorHofer · 2019-06-25T09:52:37Z

I decided to keep the runtime binplacing as I don't see an easy way around solving the manual shims dependency on it. If you have any idea how to solve this, I'm open for suggestions (not part of this PR). What if we don't use the aggregated binplaced folder but instead reference the assembly's output directories with a glob pattern?

ViktorHofer · 2019-06-30T21:21:11Z

@ericstj this is ready to be merged but I had to workaround a few issues:

Manually pass in native Unix runtime assets in conflict resolution to not pollute the outdir with a runtimes folder which contains the Unix-only runtime assets.
Generate the manifest file on UWP (and netfx - just because) as well to make conflict resolution work on other verticals as well. The PackageId is harcoded to netcoreapp currently but I don't think that matters: https://github.com/dotnet/arcade/blob/master/src/Microsoft.DotNet.CoreFxTesting/build/Microsoft.DotNet.CoreFxTesting.targets#L83

ViktorHofer · 2019-07-01T17:48:33Z

Spoke with @ericstj offline. I'm going to merge this meanwhile and will react to his feedback when he has time to review the additional changes.

ericstj

LGTM, just a few small comments for follow up?

ericstj · 2019-07-01T18:28:43Z

src/Directory.Build.targets

+          BeforeTargets="_HandlePackageFileConflicts"
+          Condition="'$(IsTestProject)' == 'true' and '@(DefaultReferenceExclusions)' != ''">
+    <ItemGroup>
+      <Reference Include="@(DefaultReferenceExclusions->'$(RefPath)%(Identity).dll')" />


What was this for? It seems like you're trying to coflict resolve against exclusions, but then you just remove them regradless. Why not instead just remove any exclusions after?

That's a good suggestion. Remove the reference after the conflict resolution is done. For that I need to touch arcade, let me do that in a subsequent PR, whenever I touch CoreFxTesting again.

ericstj · 2019-07-01T18:30:46Z

src/System.Linq.Expressions/src/System.Linq.Expressions.csproj

    <DefineConstants Condition=" '$(FeatureInterpret)' == 'true' ">$(DefineConstants);FEATURE_INTERPRET</DefineConstants>
  </PropertyGroup>
-  <ItemGroup>
-    <ReferenceFromRuntime Include="System.Private.CoreLib" />


I wonder if you could get rid of the OS-specific targeting too? I didn't see anywhere this different between Windows and Unix.

I'll check.

@marek-safar noticed this today. I think we can remove it.

ericstj · 2019-07-01T18:31:47Z

src/pretest.proj

+          Condition="'$(PlatformManifestFile)' != ''"
+          BeforeTargets="GenerateFileVersionProps">
+    <ItemGroup Condition="'$(BuildingNETCoreAppVertical)' == 'true'">
+      <_manualSharedFrameworkRuntimeFiles Include="System.Security.Cryptography.Native.OpenSsl.so" />


Can you just do a glob of the native folder?

I'm not sure how I would get the file-extensions (so and dylib) and 2) System.Security.Cryptography.Native.OpenSsl isn't a file/dir name anywhere.

The exact file names will be under $(NativeBinDir). If they aren't, then I don't imagine you'd care about feeding them to conflict resolution.

Just wanted to follow-up on this. System.Security.Cryptography.Native.OpenSsl isn't under $(NativeBinDir) but we care about feeding it to conflict resolution as otherwise it would restore runtime assets for it into the test app's output dir.

* Generate framework manifest file * Manual darc update from build '20190629.2' * Handle DefaultReferenceExclusions * Add runtime assets filter * Generate manifest file for all verticals Commit migrated from dotnet/corefx@c224a24

ViktorHofer added the area-Infrastructure-libraries label Jun 23, 2019

ViktorHofer requested a review from ericstj June 23, 2019 11:35

ViktorHofer self-assigned this Jun 23, 2019

ViktorHofer changed the title ~~Generate framework manifest file~~ Generate framework manifest file and disable runtime binplacing Jun 23, 2019

ericstj reviewed Jun 24, 2019

View reviewed changes

Directory.Build.props Outdated Show resolved Hide resolved

ViktorHofer force-pushed the ManifestFile branch 2 times, most recently from 6465cb3 to c8e162f Compare June 25, 2019 08:13

ViktorHofer force-pushed the ManifestFile branch from b622fa4 to 9ba9d9d Compare June 25, 2019 08:47

ViktorHofer changed the title ~~Generate framework manifest file and disable runtime binplacing~~ Generate framework manifest file Jun 25, 2019

ViktorHofer force-pushed the ManifestFile branch 3 times, most recently from 38c3f99 to 442a75b Compare June 27, 2019 20:00

ViktorHofer mentioned this pull request Jun 28, 2019

Update xunit versions to latest stable dotnet/arcade#3193

Merged

ViktorHofer force-pushed the ManifestFile branch 3 times, most recently from 918417a to d039faa Compare June 29, 2019 18:22

ViktorHofer closed this Jun 29, 2019

ViktorHofer reopened this Jun 29, 2019

ViktorHofer force-pushed the ManifestFile branch from d87cdb4 to b50292a Compare June 29, 2019 19:58

ViktorHofer added 2 commits June 29, 2019 22:21

Generate framework manifest file

d15410e

Manual darc update from build '20190629.2'

670a39a

ViktorHofer force-pushed the ManifestFile branch from b50292a to 670a39a Compare June 29, 2019 20:21

ViktorHofer added 2 commits June 29, 2019 23:10

Handle DefaultReferenceExclusions

27e85aa

Add runtime assets filter

a4a82da

Generate manifest file for all verticals

1543361

ViktorHofer mentioned this pull request Jun 30, 2019

[master] Update dependencies from dotnet/arcade #39061

Closed

ViktorHofer merged commit c224a24 into dotnet:master Jul 1, 2019

ViktorHofer deleted the ManifestFile branch July 1, 2019 17:48

ericstj reviewed Jul 1, 2019

View reviewed changes

karelz added this to the 3.0 milestone Jul 16, 2019

Conversation

ViktorHofer commented Jun 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ViktorHofer commented Jun 23, 2019

Uh oh!

Uh oh!

ericstj commented Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ViktorHofer commented Jun 24, 2019

Uh oh!

ericstj commented Jun 24, 2019

Uh oh!

ViktorHofer commented Jun 25, 2019

Uh oh!

ViktorHofer commented Jun 25, 2019

Uh oh!

ViktorHofer commented Jun 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ViktorHofer commented Jul 1, 2019

Uh oh!

ericstj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ViktorHofer commented Jun 23, 2019 •

edited

Loading

ericstj commented Jun 24, 2019 •

edited

Loading

ViktorHofer commented Jun 30, 2019 •

edited

Loading