Reference leaf's src from test project #42826

ViktorHofer · 2020-09-28T22:00:38Z

Fixes #42815
Unblocks #42814

ghost · 2020-09-29T11:32:06Z

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

src/libraries/Microsoft.CSharp/tests/Microsoft.CSharp.Tests.csproj

src/libraries/System.Diagnostics.Debug/tests/System.Diagnostics.Debug.Tests.csproj

ViktorHofer · 2020-10-07T10:52:25Z

/azp run runtime

azure-pipelines · 2020-10-07T10:52:52Z

Azure Pipelines successfully started running 1 pipeline(s).

ViktorHofer · 2020-10-07T14:59:37Z

@ericstj the PR is ready, do you have some more feedback? We already have the problem that we need to keep src and test project's targetframeworks in sync, today. We could either add validation for that or think of better ways to reference src from test that would not involve these additional test builds.

ericstj · 2020-10-07T18:27:31Z

better ways to reference src from test that would not involve these additional test builds.

See my comment in the issue. I think we're making these references very special already, it doesn't hurt to make them a bit more special.

Were you able to look into incremental build for tests and make sure that no changes to src/ref still result in a reasonably fast inner-loop build for tests? I would think that "reasonable" here means we aren't doubling the time to build a test project when there are no changes to the src/ref.

ViktorHofer · 2020-10-08T15:01:27Z

...tworkInformation/tests/FunctionalTests/System.Net.NetworkInformation.Functional.Tests.csproj

@@ -2,7 +2,7 @@
  <PropertyGroup>
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
    <StringResourcesPath>../../src/Resources/Strings.resx</StringResourcesPath>
-    <TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
+    <TargetFrameworks>$(NetCoreAppCurrent)-Windows_NT;$(NetCoreAppCurrent)-Linux;$(NetCoreAppCurrent)-Browser;$(NetCoreAppCurrent)-OSX;$(NetCoreAppCurrent)-iOS;$(NetCoreAppCurrent)-tvOS;$(NetCoreAppCurrent)-FreeBSD</TargetFrameworks>
    <IgnoreForCI Condition="'$(TargetOS)' == 'Browser'">true</IgnoreForCI>


@ericstj with what we discussed, how would I express that Browser is not compatible when using NetCoreAppCurrent? In such cases I think I would still need to mimic the the src's configurations and remove Browser.

This thing wasn't compiling for Browser before, I would expect that condition to work the same way.

Why do you think this project wasn't compiling for Browser before locally?

Because TargetFrameworks was $(NetCoreAppCurrent) not $(NetCoreAppCurrent)-Browser so this would build during the browser vertical, but compile for $(NetCoreAppCurrent) not $(NetCoreAppCurrent)-Browser. I'm missing what changes here would require us to target $(NetCoreAppCurrent)-Browser

What I'm trying to achieve here is to avoid the unnecessary Browser build completely and remove the IgnoreForCI proeprty but I can't express that with $(NetCoreAppCurrent) as the TFM as that is compatible with Browser.

jep, from above:

In such cases I think I would still need to mimic the the src's configurations and remove Browser.

Can you use a placeholder TargetFramework? $(NetCoreAppCurrent);_$(NetCoreAppCurrent)-Browser

No, we removed the concept and implementation of placeholders in the configuration system as it caused issues during restore and because it wasn't necessary anymore with ProjectReferences in OOBs.

I see, then an exclusion?

runtime/src/libraries/tests.proj

Line 109 in 40b8e3d

<ItemGroup Condition="'$(TargetOS)' == 'Browser' and '$(RunDisabledWasmTests)' != 'true'">

IMHO we should try to minimize the way we disable tests. My preference would be some indication in the source, so that we get reporting over the number of tests that are disabled and still test what happens when code that uses the type is JIT'ed.

I see, then an exclusion?

We use project exclusions for temporary issues only... Similar to ActiveIssue in code. This doesn't solve exactly what I was asking for, which is a mechanism to not compile the Browser configuration at all which would still happen when you invoke the project directly.

ericstj · 2020-10-08T20:34:27Z

Should this be part of the monthly rollout and/or be accompanied with some docs that tell folks how to speed up the build if they don’t change src/ref?

ViktorHofer · 2020-10-08T21:40:26Z

Should this be part of the monthly rollout and/or be accompanied with some docs that tell folks how to speed up the build if they don’t change src/ref?

Merging this as part of the monthly rollout shouldn't be necessary. There isn't a change that requires developers to change their workflow. Developers won't need to manually rebuild the source project anymore but that doesn't mean that the existing process wouldn't work anymore.

On my 2 core notebook I didn't notice any perf hit as most inbox projects don't use P2Ps yet and hence there aren't any transitive P2Ps (not even the ref project as that's only referenced if it isn't built yet). When/If we go down the rabbit hole and use P2Ps everywhere, we should definitely document the --no-dependencies option and advertise it when only the invoked project changed. As said, I don't think this is necessary as part of this PR.

ericstj · 2020-10-08T23:24:00Z

most inbox projects don't use P2Ps yet and hence there aren't any transitive P2Ps (not even the ref project as that's only referenced if it isn't built yet)

I see I hadn't realized that we didn't always rebuild ref. Makes sense. I'm OK with this.

ericstj · 2020-10-08T23:29:03Z

Build is failing with:

/__w/1/s/.packages/microsoft.dotnet.build.tasks.targetframework.sdk/6.0.0-beta.20508.4/build/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk.targets(79,5): error : Not able to find a compatible supported target framework for net6.0-Unix in Project System.Net.Http.csproj. The Supported Configurations are net6.0-Windows_NT, net6.0-Linux, net6.0-OSX, net6.0-FreeBSD, net6.0-iOS, net6.0-tvOS, net6.0-Browser [/__w/1/s/src/libraries/System.Net.Http/tests/EnterpriseTests/System.Net.Http.Enterprise.Tests.csproj]
/__w/1/s/.packages/microsoft.dotnet.build.tasks.targetframework.sdk/6.0.0-beta.20508.4/build/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk.targets(79,5): error : Not able to find a compatible supported target framework for net6.0-Unix in Project System.Net.Http.csproj. The Supported Configurations are net6.0-Windows_NT, net6.0-Linux, net6.0-OSX, net6.0-FreeBSD, net6.0-iOS, net6.0-tvOS, net6.0-Browser [/__w/1/s/src/libraries/System.Net.Http/tests/UnitTests/System.Net.Http.Unit.Tests.csproj]

ericstj · 2020-10-08T23:39:54Z

eng/references.targets

+        <Private>false</Private>
+        <ReferenceOutputAssembly Condition="!$(NetCoreAppLibraryNoReference.Contains('%(Filename);')) and
+                                            '%(ProjectReference.SkipUseReferenceAssembly)' != 'true'">false</ReferenceOutputAssembly>
+        <ReferringTargetFramework Condition="'$(TargetFrameworkSuffix)' == ''">$(TargetFramework)-$(TargetOS)</ReferringTargetFramework>


This isn't handling the case when referencing project is a less specific RID than referenced. Nor does it handle the case where TFM is less specific (though we may not use this any more).

Suggested change

<ReferringTargetFramework Condition="'$(TargetFrameworkSuffix)' == ''">$(TargetFramework)-$(TargetOS)</ReferringTargetFramework>

<ReferringTargetFramework>$(BuildTargetFramework)-$(TargetOS)</ReferringTargetFramework>

Nor does it handle the case where TFM is less specific (though we may not use this any more).

Right, we don't care about that anymore as test projects targeting ie .NETStandard aren't supported.

This isn't handling the case when referencing project is a less specific RID than referenced.

Right, but tbh I don't understand your proposal below. BuildTargetFramework is the same as TargetFramework if the project is invoked individually (outside of a root build).

This isn't handling the case when referencing project is a less specific RID than referenced.

I thought about this a bit more and I'm unsure how we would make this ever work. I'm ok with supporting a rid-less TargetFramework for the sake of less configurations but I can't get my head around how we would support less specific RIDs without loosing the ability in test projects to not support some configurations.

Maybe we should chat about this offline as this confuses me... In the meantime I adjusted the two affected test projects for the sake of getting this in.

Binplacing will still make the right decision. The purpose of the project reference here is just to make a convenient build. That convenient build shouldn't really be chosen based on the test's configuration, but what the vertical build would have put in the test runtime, which is decided by the things I suggested.

… into RefSrcFromTest

ViktorHofer · 2020-10-10T21:24:26Z

I have absolutely no idea why this is failing on Browser and iOS :( cc @akoeplinger

akoeplinger · 2020-10-12T11:40:24Z

For System.Runtime.Tests in Browser I can see this:

[17:23:17] info: Initializing.....
[17:23:17] info: WASM-ERR:
[17:23:17] info: WASM-ERR: Unhandled Exception:
[17:23:17] info: WASM-ERR: System.IO.FileNotFoundException: Could not load file or assembly 'System.Collections, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies.
[17:23:17] info: WASM-ERR: File name: 'System.Collections, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'
[17:23:17] info: Exception:System.IO.FileNotFoundException: Could not load file or assembly 'System.Collections, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies.
[17:23:17] info: File name: 'System.Collections, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'
[17:23:17] info: 17:23:17.6701579 Process exited with 1

Probably System.Collections is no longer referenced?

Dotnet-GitSync-Bot added the area-Meta label Sep 28, 2020

ViktorHofer force-pushed the RefSrcFromTest branch from f90d65d to d296160 Compare September 29, 2020 08:10

ViktorHofer added area-Infrastructure-libraries and removed area-Meta labels Sep 29, 2020

ViktorHofer force-pushed the RefSrcFromTest branch 2 times, most recently from 3406ed7 to 2459897 Compare September 29, 2020 12:27

ViktorHofer marked this pull request as ready for review September 29, 2020 14:26

ViktorHofer requested review from jozkee, layomia and steveharter as code owners September 29, 2020 14:26

ViktorHofer requested review from Anipik and ericstj and removed request for steveharter, layomia and jozkee September 29, 2020 14:26

ericstj reviewed Sep 29, 2020

View reviewed changes

This was referenced Sep 30, 2020

Mono System.Text.Json.Tests on Windows timing out #42677

Open

Builds legs getting abandoned due to agent notification issues #35223

Closed

Reference leaf's src from test project

06fc0d5

ViktorHofer force-pushed the RefSrcFromTest branch from 8a9c530 to 982ba10 Compare October 4, 2020 17:12

Make inbox P2Ps not reference their output assembly

c219229

ViktorHofer force-pushed the RefSrcFromTest branch from 982ba10 to c219229 Compare October 5, 2020 10:16

runfoapp bot mentioned this pull request Oct 5, 2020

Inability to unzip assets during build on Unix x64 #32805

Closed

Add private inbox assemblies to noreference section

2e97ec9

ViktorHofer commented Oct 8, 2020

View reviewed changes

ViktorHofer mentioned this pull request Oct 8, 2020

Add extension point to override TargetFramework in consuming project dotnet/arcade#6361

Merged

ViktorHofer added 2 commits October 8, 2020 22:24

PR feedback and update TargetFramework.Sdk

4bc4851

Merge branch 'master' into RefSrcFromTest

e363e87

Update global.json

a57f1b7

ericstj approved these changes Oct 8, 2020

View reviewed changes

ericstj reviewed Oct 8, 2020

View reviewed changes

ViktorHofer added 4 commits October 9, 2020 15:05

Update configurations in two test projects

2d252d2

Merge branch 'RefSrcFromTest' of https://github.com/ViktorHofer/runtime…

2b5ab61

… into RefSrcFromTest

Merge branch 'master' into RefSrcFromTest

712ae85

Merge branch 'master' into RefSrcFromTest

be179d8

This was referenced Oct 9, 2020

OSX deprovision jaredpar/runfo#41

Closed

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

Merge branch 'master' into RefSrcFromTest

6daf9eb

Merge remote-tracking branch 'upstream/master' into RefSrcFromTest

5d57920

ViktorHofer requested a review from ahsonkhan as a code owner November 12, 2020 20:35

This was referenced Nov 12, 2020

SelfContained_NetCoreApp3_CompatMode_SingleFile_APITests failed in CI #43917

Closed

Testing out the new system jaredpar/runfo#71

Open

ViktorHofer closed this Feb 8, 2021

ghost locked as resolved and limited conversation to collaborators Mar 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reference leaf's src from test project #42826

Reference leaf's src from test project #42826

ViktorHofer commented Sep 28, 2020

ghost commented Sep 29, 2020

ViktorHofer commented Oct 7, 2020

azure-pipelines bot commented Oct 7, 2020

ViktorHofer commented Oct 7, 2020

ericstj commented Oct 7, 2020

ViktorHofer Oct 8, 2020

ericstj Oct 8, 2020

ViktorHofer Oct 8, 2020

ericstj Oct 8, 2020

ViktorHofer Oct 8, 2020

ViktorHofer Oct 8, 2020

ericstj Oct 8, 2020

ViktorHofer Oct 8, 2020

ericstj Oct 8, 2020 •

edited

Loading

ViktorHofer Oct 8, 2020

ericstj commented Oct 8, 2020

ViktorHofer commented Oct 8, 2020

ericstj commented Oct 8, 2020

ericstj commented Oct 8, 2020

ericstj Oct 8, 2020 •

edited

Loading

ViktorHofer Oct 9, 2020

ViktorHofer Oct 9, 2020 •

edited

Loading

ericstj Oct 13, 2020

ViktorHofer commented Oct 10, 2020

akoeplinger commented Oct 12, 2020

	<ReferringTargetFramework Condition="'$(TargetFrameworkSuffix)' == ''">$(TargetFramework)-$(TargetOS)</ReferringTargetFramework>
	<ReferringTargetFramework>$(BuildTargetFramework)-$(TargetOS)</ReferringTargetFramework>

Reference leaf's src from test project #42826

Reference leaf's src from test project #42826

Conversation

ViktorHofer commented Sep 28, 2020

ghost commented Sep 29, 2020

ViktorHofer commented Oct 7, 2020

azure-pipelines bot commented Oct 7, 2020

ViktorHofer commented Oct 7, 2020

ericstj commented Oct 7, 2020

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 Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericstj commented Oct 8, 2020

ViktorHofer commented Oct 8, 2020

ericstj commented Oct 8, 2020

ericstj commented Oct 8, 2020

ericstj Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer commented Oct 10, 2020

akoeplinger commented Oct 12, 2020

ericstj Oct 8, 2020 •

edited

Loading

ericstj Oct 8, 2020 •

edited

Loading

ViktorHofer Oct 9, 2020 •

edited

Loading