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.

Sign up for GitHub

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
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/tests/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,10 @@
<BuildAsStandalone Condition="'$(BuildAsStandalone)' == ''">true</BuildAsStandalone>
<OutputType Condition="$(BuildAsStandalone)">Exe</OutputType>
<TestFramework>GeneratedRunner</TestFramework>

<!-- Prevent project-specific NuGet props/targets from clashing with the ones we manually import below. -->
<ImportProjectExtensionProps>false</ImportProjectExtensionProps>
<ImportProjectExtensionTargets>false</ImportProjectExtensionTargets>
Comment on lines +200 to +202
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 •
edited
Loading

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!

</PropertyGroup>

<Import Project="$(RepositoryEngineeringDir)testing\tests.props" Condition="'$(IsTestsCommonProject)' != 'true'" />
Expand Down