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

Prevent tests from importing "normal" NuGet props/targets #80573

Merged
merged 1 commit into from
Feb 27, 2023

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Jan 12, 2023

Fixes #70658.

We import them manually from a shared location.

Usually this is not a problem because batch build (via the build script)
does its own restore thing, but it is a problem in case the project is
restored on its own, such as when it is built with "dotnet build".
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 12, 2023
@ghost
Copy link

ghost commented Jan 12, 2023

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

CI testing...

Author: SingleAccretion
Assignees: -
Labels:

area-Infrastructure-coreclr, community-contribution

Milestone: -

@SingleAccretion
Copy link
Contributor Author

@dotnet/runtime-infrastructure

Comment on lines +200 to +202
<!-- Prevent project-specific NuGet props/targets from clashing with the ones we manually import below. -->
<ImportProjectExtensionProps>false</ImportProjectExtensionProps>
<ImportProjectExtensionTargets>false</ImportProjectExtensionTargets>
Copy link
Member

Choose a reason for hiding this comment

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

Not blocking this PR but here's some feedback from my side.

From someone that had to deal with non restore-able projects in corefx and the inability to do anything like PackageReference in the BCL, I would strongly argument towards allowing every project in this repo to use the SDK's common path, which includes importing NuGet props and targets.

What's the reason that props and targets files are manually injected into the test projects in the first place? Is it restore speed? Is it the intermediate's artifacts size?

Copy link
Contributor Author

@SingleAccretion SingleAccretion Jan 13, 2023

Choose a reason for hiding this comment

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

I suppose I don't have the context for why this is set up this way, though I do suspect build efficiency to be the primary motivation as there are a lot of runtime test projects.

Copy link
Member

Choose a reason for hiding this comment

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

The concern is that there are so many runtime test projects that we exceed the memory on the system and cause MSBuild to crash with OOM. We avoid compounding this issue with having to shell out for both build and restore by restoring once into Core_Root and not restoring each test project itself. As we progress on the test merging work into the "merge projects/assemblies" stage (after the "merge test runs into a single process" stage), we should be able to reduce the number of project files down enough to enable us to do more traditional project behaviors.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds fabulous!

@SingleAccretion
Copy link
Contributor Author

CI failure is #73040.

@SingleAccretion
Copy link
Contributor Author

This time the CI failure is #75244.

@dotnet/runtime-infrastructure - ping, are we ok to merge this change?

@agocke agocke merged commit eccbe90 into dotnet:main Feb 27, 2023
@SingleAccretion SingleAccretion deleted the NuGet-Imports-In-Tests branch March 28, 2023 19:49
@ghost ghost locked as resolved and limited conversation to collaborators Apr 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MSBuild warnings when building runtime test projects
4 participants