Make changes to reference assembly rerun compile

[ericstj]

@geoffkizer noticed that making a change only to a ref wouldn't cause ApiCompat to fail. This was because the src project doesn't recompile, so ApiCompat doesn't rerun.

This won't completely solve the problem since there's a condition on the reference from src->ref, so it's still a race condition when you build the SLN. If the ref happens to build before src it will catch the change, otherwise it won't. We could remove that condition on src->ref, but then it would substantially grow the project graph for single project src build.

runtime/eng/resolveContract.targets

Lines 22 to 27 in ddcbc34

	<ResolvedMatchingContract Condition="Exists('$(ContractAssemblyPath)')" Include="$(ContractAssemblyPath)" />
	<!-- If the contract doesn't exist in the default contract output path add a project reference to the contract project to resolve -->
	<ProjectReference Condition="'@(ResolvedMatchingContract)' == ''" Include="$(ContractProject)">
	<ReferenceOutputAssembly>false</ReferenceOutputAssembly>
	<OutputItemType>ResolvedMatchingContract</OutputItemType>
	</ProjectReference>

I measured a no-op incremental build of src\System.Net.Sockets.csproj and today it is 2s on my machine.
If I remove the condition on the src->ref project reference that goes up to 3.7s. It would go up a lot more for things at the top of the stack.
For example, System.Net.Http.Json will go from ~5s to ~10s.

That said, this would only hit folks building the src csproj directly, which might be unlikely. Those folks can always do --no-dependencies if they want a faster build.

Let me know if folks think it's worthwhile to remove that fast-path for better incremental.

[safern]

Makes sense. I think the perf regression is worth it. I believe a couple of people have hit this and we spend more time investigating what's going on with them, than what they would have waited for the build to finished if it worked like this in the first place.

[ViktorHofer]

The perf regression that Eric mentioned is only noticeable if we lift the condition from the P2P. The fast path currently only works if the tfm between the ref and src match, which often isn't the case.

I think we should remove the condition instead...

[ericstj]

I think we should remove the condition instead...

We need both this and to remove that condition. I'll go ahead and do so.

[ViktorHofer]

Of course, as the reference assembly isn't referenced 🤔😅. Makes sense.

[ericstj]

Interesting, it turns out APICompat doesn't always work when using the ProjectReference. It works in most cases, but because the ProjectReference returns the TargetPath which doesn't contain the full assembly closure, unlike ContractAssemblyPath, which was based on NetCoreAppCurrentRefPath which has everything, I'm seeing some API compat errors due to missing references.

[ghost]

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

Issue Details

---

@geoffkizer noticed that making a change only to a ref wouldn't cause ApiCompat to fail. This was because the src project doesn't recompile, so ApiCompat doesn't rerun.

This won't completely solve the problem since there's a condition on the reference from src->ref, so it's still a race condition when you build the SLN. If the ref happens to build before src it will catch the change, otherwise it won't. We could remove that condition on src->ref, but then it would substantially grow the project graph for single project src build.

runtime/eng/resolveContract.targets

Lines 22 to 27 in ddcbc34

	<ResolvedMatchingContract Condition="Exists('$(ContractAssemblyPath)')" Include="$(ContractAssemblyPath)" />
	<!-- If the contract doesn't exist in the default contract output path add a project reference to the contract project to resolve -->
	<ProjectReference Condition="'@(ResolvedMatchingContract)' == ''" Include="$(ContractProject)">
	<ReferenceOutputAssembly>false</ReferenceOutputAssembly>
	<OutputItemType>ResolvedMatchingContract</OutputItemType>
	</ProjectReference>

I measured a no-op incremental build of src\System.Net.Sockets.csproj and today it is 2s on my machine.
If I remove the condition on the src->ref project reference that goes up to 3.7s. It would go up a lot more for things at the top of the stack.
For example, System.Net.Http.Json will go from ~5s to ~10s.

That said, this would only hit folks building the src csproj directly, which might be unlikely. Those folks can always do --no-dependencies if they want a faster build.

Let me know if folks think it's worthwhile to remove that fast-path for better incremental.

Author:	ericstj
Assignees:	-
Labels:	area-Infrastructure-libraries
Milestone:	-

[ericstj]

Interesting, it looks like

runtime/eng/resolveContract.targets

Lines 5 to 6 in 5c3c690

	<!-- Use implementation referencepath if no contract tfm is set. -->
	<ContractDependencyPaths Condition="'$(ContractDependencyPaths)' == ''">$(ContractDependencyPaths);@(ReferencePath->'%(RelativeDir)'->Distinct())</ContractDependencyPaths>

might have been trying to address this, however that's done statically and operating on @(ReferencePath) items which aren't normally available until ResolveReferences target is run. @ViktorHofer do you recall how this was meant to work?

[ericstj]

I debugged this and I see that this is resulting in delayed evaluation of that item transform until the target which runs and includes that property in an item. Cute.

The reason this isn't working for NETCoreAppCurrent, is because it doesn't include these:

runtime/eng/resolveContract.targets

Lines 3 to 6 in 5c3c690

	<ContractDependencyPaths Condition="'$(DisableImplicitFrameworkReferences)' == 'true' and
	'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">$(MicrosoftNetCoreAppRefPackRefDir)</ContractDependencyPaths>
	<!-- Use implementation referencepath if no contract tfm is set. -->
	<ContractDependencyPaths Condition="'$(ContractDependencyPaths)' == ''">$(ContractDependencyPaths);@(ReferencePath->'%(RelativeDir)'->Distinct())</ContractDependencyPaths>

I guess I can try to always include ReferencePath. I don't see why that would be a problem.

[ericstj]

All build failures are

Not able to find a compatible supported target framework for netstandard2.0 in Project System.Security.Cryptography.Pkcs.csproj. The Supported Configurations are net6.0, netcoreapp3.0, netstandard2.1, net461

So we're building an implementation for this but not a reference.

runtime/src/libraries/System.Security.Cryptography.Pkcs/pkg/System.Security.Cryptography.Pkcs.pkgproj

Line 10 in 0fb9c00

<HarvestIncludePaths Include="ref/netstandard2.0" />

That's interesting. I wonder what it was running APICompat against before 🤔

[danmoseley]

We could remove that condition on src->ref, but then it would substantially grow the project graph for single project src build.

Could it run deterministically if I did /t:rebuild on the project? It would not be hard to do that once before pushing my PR.

[ericstj]

If you built the ref at least once with the change, rebuild of the src would work. I think a rebuild of the sln would work as well, since it would clean the old ref. Only building the sln is a race.

[ericstj]

That's interesting. I wonder what it was running APICompat against before 🤔

runtime/src/libraries/System.Security.Cryptography.Pkcs/src/System.Security.Cryptography.Pkcs.csproj

Line 646 in b37f10a

<ResolvedMatchingContract Include="$(NuGetPackageRoot)$(MSBuildProjectName.ToLowerInvariant())\$(SystemSecurityCryptographyPkcsVersion)\ref\$(TargetFramework)\$(MSBuildProjectName).dll" />

Makes sense. I'll add a condition.

[ViktorHofer]

Makes sense. I'll add a condition.

Ah, I forgot about that one... For some reason we don't build the netstandard2.0 ref anymore.

[ViktorHofer]

Failure is #32805.