Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change package testing to use RuntimeTargets rather than RID-specific restore #53575

Merged
merged 9 commits into from
Jun 7, 2021

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Jun 2, 2021

Fixes #2017

@Anipik and I were playing around with this today and realized we were very close to enabling package testing using just single restore. I went ahead and finished our prototype. This should greatly reduce the restore cost and total duration of package testing.

@ViktorHofer this should unblock #53439

@ghost
Copy link

ghost commented Jun 2, 2021

Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #2017

@Anipik and I were playing around with this today and realized we were very close to enabling package testing using just single restore. I went ahead and finished our prototype. This should greatly reduce the restore cost and total duration of package testing.

@ViktorHofer this should unblock #53439

Author: ericstj
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

<RuntimeLibToTestDependency Include="@(ReferencePath)" Condition="'%(ReferencePath.NuGetPackageId)' != '$(TestPackageId)'" />

<!-- Some dependent packages may be excluded from compile, consider these as candiates as well.
We assume they must expose a RID agnostic asset, thus we only check RuntimeCopyLocalItems. -->
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be more correct / replicate the actual runtime behavior here we would need to replicate the host selection logic to test with the right runtime assets for dependencies. It's not so interesting to do this type of testing since the SDK doesn't make it possible for a library to reference a runtime specific asset of another library or project, and we don't typically do this with our custom config system. It's bad practice for library to depend on implementation-only publics of another library; if one library needed it why wouldn't a customer library need it?

So the asset analysis ends up looking like this:

  1. Package under test: Foreach runtime asset group
  2. Package dependencies w/ exclude=comple: RID-less runtime asset group
  3. Package dependencies w/o exclude=compile: compile asset group
  4. Framework: ref-pack / reference assemblies only.

Note that 2-4 are changes from what we previously did, but these are "safe" changes, since if these validations succeed then API compat will ensure that transitively things should succeed.

@ericstj
Copy link
Member Author

ericstj commented Jun 2, 2021

I broke this in a hilarious way. The structure of the package testing projects used to be

{packageId}\{TFM}\project.csproj

I removed the TFM since I can make the projects cross-target now that they don't need RIDs

{packageId}\project.csproj

When hexlix runs these, it's removing the packageId folder and placing the project in the root.

In the same directory as the packages folder. So it looks like this:

{workItemDir}\project.csproj
{workItemDir}\packages\...

Then what happens is all the package content gets globbed as None, and passed in as CandidateAssemblyFiles to RAR. CandidateAssemblyFiles happen to be considered first in RAR's precedence when resolving assemblies. So we were getting seemingly random assemblies resolved, based on whatever appeared first in the package cache (often not even appropriate for the framework).

Guess I need to restructure the helix test directories for these 😆.

@ViktorHofer
Copy link
Member

As an alternative, you could also disable the globbing.

@ViktorHofer
Copy link
Member

As you are removing some of the frameworkSettings files, why not remove the portable and netcore50 as well?

@ericstj
Copy link
Member Author

ericstj commented Jun 2, 2021

As you are removing some of the frameworkSettings files, why not remove the portable and netcore50 as well?

I'm removing the ones that were no longer needed due to the change I made, since they were related to using the RuntimeIdentifier, or testing against runtime assemblies. I'll consider cleaning others if they're clearly dead code, but I don't want to feature creep this PR.

As an alternative, you could also disable the globbing.

I don't think that's necessary. Better to keep projects simple rather than mess with them to accommodate a wacky thing that customers should never do (putting packages restore folder under project).

@ericstj
Copy link
Member Author

ericstj commented Jun 4, 2021

As an alternative, you could also disable the globbing.

I don't think that's necessary. Better to keep projects simple rather than mess with them to accommodate a wacky thing that customers should never do (putting packages restore folder under project).

So it turns out we can even delete the custom packages folder as Helix already does this for us:
https://github.com/dotnet/arcade/blob/1b17d7c9f0c2100bac5766b154c99fb31ab20eb4/src/Microsoft.DotNet.Helix/Sdk/tools/dotnet-cli/DotNetCli.targets#L21-L22

I'm looking into keeping the binlog, but only uploading it when there is a failure.

@ericstj ericstj requested a review from MattGal June 4, 2021 16:00
ericstj added 4 commits June 4, 2021 11:17
Simplify outer build of package tests to just use InnerTargets extension point.

Use Helix's work-item isolated packages folder

Give a better name to binlogs
This item will be conflict-resolved whereas RuntimeCopyLocalItems was not.

Also fix the case where a package intentionally provides no assets (yet installs)
@ericstj
Copy link
Member Author

ericstj commented Jun 7, 2021

PKCS test is failing due to dotnet/sdk#18129. I'm planning to change how I did the workaround for this. Previously I was relying on conflict resolution dropping a compile asset to indicate what package references should not be considered when evaluating closure of runtimes, but this doesn't work if the packages that should have been dropped were Exclude=compile. I'm either going to feed all the ref-pack assets into conflict resolution for consideration at runtime as well, or I'll leverage the PackageOverrides mechanism if I can get it to work.

ericstj added 2 commits June 7, 2021 10:35
.NETStandard doesn't conflict resolve runtime assets.
Workaround by feeding .NETStandard references as runtime for purposes of conflict resolution.
@ericstj ericstj merged commit fcad78b into dotnet:main Jun 7, 2021
Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Jul 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve performance of Package testing leg
6 participants