Raise Transitive Project References to MSBuild elements #478

natidea · 2016-12-12T01:35:44Z

Raise closure of Transitive Project References to msbuild as ProjectReference elements. This closure is generated by NuGet in the assets file.

Fixes #200
See dotnet/project-system#199 (comment) for details on P2P plan.

…ojectReferences is called

…ectRefs

…ojects with ProducesNoOutput settings. (Fixes failures building xUnit.net.csproj)

natidea · 2016-12-12T01:51:24Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.PackageDependencyResolution.targets

+  <Target Name="IncludeTransitiveProjectReferences"
+          Condition="'$(ResolvePackageDependenciesForBuild)' == 'true' and '$(EnableResolvePackageDependencies)' == 'true' and Exists('$(ProjectAssetsFile)')"
+          DependsOnTargets="_ComputeTransitiveProjectReferences" 
+          BeforeTargets="AssignProjectConfiguration">


BeforeTargets="AssignProjectConfiguration" ensures ProjectReference elements are created before they are processed into ResolvedProjectReferences. The alternative is to add this to the list ResolvePackageDependenciesForBuildDependsOn, but that would result in IncludeTransitiveProjectReferences being called too late.

Choosing this approach means we have to copy the condition from ResolvePackageDependenciesForBuild to ensure we are not calling RunResolvePackageDependencies at inappropriate times. (E.g. Without this condition, the sdk build fails on building xUnit.net.csproj

Another alternative is to bypass ResolveProjectReferences and generate the _ResolvedProjectReferencePaths element directly, as is done in Microsoft.NuGet.targets. This currently does not work because of a bug calculating resolved paths (#479).

Could we add IncludeTransitiveProjectReferences to ResolvePackageDependenciesForBuildDependsOn, and then put BeforeTargets="AssignProjectConfiguration" on the ResolvePackageDependenciesForBuild target? It seems like that would prevent us from having to repeat the same condition twice.

In any case, I would like to see comments in the targets file itself explaining how the ordering works. IIUC, the key is that the AssignProjectConfiguration is the entry point that reads ProjectReference items, so this target needs to run before that, but only if there is an assets file.

Separately, what is the difference between the ResolvePackageDependenciesForBuild and EnableResolvePackageDependencies properties? They seem to only be used together.

It appears the two flag properties are redundant, and may have been added separately. I've removed EnableResolvePackageDependencies and also simplified the ordering as you suggested to avoid repeating the condition.

natidea · 2016-12-14T01:45:59Z

/cc @dotnet/project-system
@emgarten for NuGet fyi.

nguerrera · 2016-12-16T00:23:05Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.PackageDependencyResolution.targets

+          DependsOnTargets="RunResolvePackageDependencies"
+          Returns="_TransitiveProjectReferences">
+    <ItemGroup>
+      <ProjectDefs Include="@(PackageDefinitions->WithMetadataValue('Type', 'project'))" />


Nit: Let's be consistent on not abbreviating things. We have PackageDefinitions and ProjectDefs on the same line here. It should be ProjectDefinitions.

Item names are also supposed to be singular, but that's one Microsoft has gotten wrong in many places so I feel less strongly about enforcing it. #Resolved

dsplaisted · 2016-12-16T02:41:25Z

TestAssets/TestProjects/AppWithTransitiveProjectRefs/AuxLibrary/AuxLibrary.csproj

+<Project Sdk="Microsoft.NET.Sdk">
+
+  <PropertyGroup>
+    <Version>42.43.44.45-alpha</Version>


Is there any reason these projects need to have a version? If not, let's remove them from the project file.

dsplaisted · 2016-12-16T02:47:20Z

test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildAnAppWithTransitiveProjectRefs.cs

+        }
+
+        [Fact]
+        public void It_builds_the_project_successfully_twice()


Same as my comment on #489: Is there a reason we need to test building twice for this scenario?

dsplaisted · 2016-12-16T02:50:21Z

test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildAnAppWithTransitiveProjectRefs.cs

+                .HaveStdOutContaining("This string came from AuxLibrary!");
+
+            var appInfo = FileVersionInfo.GetVersionInfo(Path.Combine(outputDirectory.FullName, "TestApp.dll"));
+            appInfo.CompanyName.Should().Be("Test Authors");


My feeling is that we don't need to be testing the assembly versions and attributes here. Tests should generally be specific about what they are testing as opposed to testing a bunch of behaviors that don't seem to be related in a given scenario.

dsplaisted · 2016-12-16T03:11:31Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.PackageDependencyResolution.targets

+  <Target Name="IncludeTransitiveProjectReferences"
+          Condition="'$(ResolvePackageDependenciesForBuild)' == 'true' and '$(EnableResolvePackageDependencies)' == 'true' and Exists('$(ProjectAssetsFile)')"
+          DependsOnTargets="_ComputeTransitiveProjectReferences" 
+          BeforeTargets="AssignProjectConfiguration">


Could we add IncludeTransitiveProjectReferences to ResolvePackageDependenciesForBuildDependsOn, and then put BeforeTargets="AssignProjectConfiguration" on the ResolvePackageDependenciesForBuild target? It seems like that would prevent us from having to repeat the same condition twice.

In any case, I would like to see comments in the targets file itself explaining how the ordering works. IIUC, the key is that the AssignProjectConfiguration is the entry point that reads ProjectReference items, so this target needs to run before that, but only if there is an assets file.

Separately, what is the difference between the ResolvePackageDependenciesForBuild and EnableResolvePackageDependencies properties? They seem to only be used together.

dsplaisted · 2016-12-16T03:13:30Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.PackageDependencyResolution.targets

+          BeforeTargets="AssignProjectConfiguration">
+    <ItemGroup>
+      <!-- Add the references we computed -->
+      <ProjectReference Include="@(_TransitiveProjectReferences)" />


Is it possible for there to be a transitive reference from the assets file to a project that is also directly referenced? It would be good to have a test case to cover this, where B references C, and A references both B and C. #Resolved

…encies and remove redundant condition EnableResolvePackageDependencies

Use package dependencies (under "target") instead of package definitions (under "library") to compute transitive project references. Package dependencies could contain different information for different TFMs. This also simplifies determining which project references are transitive since we can do that all using information in the package dependency tree.

natidea · 2016-12-21T04:06:44Z

After chatting with @emgarten , I've made some changes. Previously we were computing transitive project references from ProjectDefinitions which comes from the asset file's "libraries" section. This is a deduped list of packages/projects. This is not correct because project transitivity could be conditional on the TFM. In this case, the project reference may be handled transitively in one TFM, but not the other.

Commit f3b057b changes to using PackageDependencies so computing transitive project references matches the information for the active target. This includes excluding projects marked with a placeholder compile time assembly _._

Additionally I added logic to exclude directly referenced projects to address the concern @dsplaisted raised. It turns out these duplicated projects were deduped by ResolveAssemblyReference because their output assemblies were identical, so the compiler only gets one copy. However the Build Hierarchy printed at the end of a diagnostic build contains more entries without this fix, so perhaps more redundant NoOp builds were happening.

I've tested this using the following project layout:

    A -> B -> C -> D
              C -> E
    A -> D

When building A:

C and E are treated as transitive references
B and D are direct references and hence are already included; they are not appended by the new target

I simulated making E a non-transitive reference in C, by modifying the assets file to insert a placeholder in E. With this change only C is treated as a transitive reference and E is not sent to the compiler. This compiles fine, but I get an error in the GenerateBuildDependencyFile target about missing project information. We will need to fix DependencyContextBuilder to handle this case. I'll file a bug on this.

natidea · 2016-12-21T04:07:23Z

/cc @srivatsn for Ask Mode approval

…ageDependencies task

natidea · 2016-12-21T20:16:56Z

Merging so we can get some insertions going. Additional feedback still welcome.

ripenko · 2017-03-02T08:31:01Z

When this fix will be available in VS 2017?

dsplaisted · 2017-03-02T14:54:24Z

It should already be in the latest RC of VS 2017.

KshitizGIT · 2017-06-23T16:02:57Z

Any way I can opt out of this?

Lets say, I have the following project layout.

A -> B -> C.

I don't want classes inside assembly C to be directly accessible inside assembly A.

srivatsn · 2017-06-23T16:38:53Z

Adding PrivateAssets=All to the ProjectReference stops the flow of the transitive ref. So, in this case. B.csproj would have this:

  <ProjectReference Include="C.csproj" PrivateAssets="All" />

nguerrera · 2017-06-23T16:45:18Z

@srivatsn, you mean ProjectReference, right?

srivatsn · 2017-06-23T16:46:16Z

Ah yes typo - fixed it the original comment.

KshitizGIT · 2017-06-23T16:59:18Z

Thanks a lot.. Worked like a charm 👍

Turns out that the .NET SDK lifts transitive project references as direct references (without any additional metadata), and this causes the second-level dependency from being built unexpectedly (see dotnet/sdk#478 and dotnet/project-system#199). Since we don't want to disrupt the IDE (BuildingInsideVisualStudio) and we only want to fix this for the very specific case of running from the CLI `dotnet pack --no-build`, we make the fix very constrained for that scenario. We check for `NoBuild` but ALSO for `_IsPacking`, which is passed by the `dotnet pack` command. This ensures minimal impact in all other scenarios, since we're essentially turning off a built-in behavior in the SDK that has explicit side-effects (by design and desirable) and we should preserve. #Fixes 501

natidea added 5 commits December 9, 2016 01:50

Add targets to include transitive project references before ResolvePr…

e0a174f

…ojectReferences is called

Add assets and scaffolding for test to build App With Transitive Proj…

69eabef

…ectRefs

Add condition to IncludeTransitiveProjectReferences to skip during pr…

d596f9e

…ojects with ProducesNoOutput settings. (Fixes failures building xUnit.net.csproj)

Fixup Transitive Project Ref tests

c07d029

Add Clean target test for AppWithTransitiveProjectRefs

f8b1a39

dnfclas added the cla-already-signed label Dec 12, 2016

natidea commented Dec 12, 2016

View reviewed changes

nguerrera reviewed Dec 16, 2016

View reviewed changes

dsplaisted reviewed Dec 16, 2016

View reviewed changes

natidea added 5 commits December 19, 2016 17:58

Remove irrelevant test scenarios

e7a8fe1

Simplify ordering of targets associated with resolving package depend…

057c7b6

…encies and remove redundant condition EnableResolvePackageDependencies

Merge remote-tracking branch 'dotnet/master' into transitiveProjectRef

48b27c4

Exclude top level project references from _TransitiveProjectReferences

4164546

Add unit test for transitive project reference changes in ResolvePack…

b8228ad

…ageDependencies task

srivatsn approved these changes Dec 21, 2016

View reviewed changes

srivatsn added the Approved to merge label Dec 21, 2016

natidea merged commit a700d03 into dotnet:master Dec 21, 2016

srivatsn mentioned this pull request Dec 22, 2016

ASP.NET core project build requires transitive project references. #547

Closed

rainersigwald mentioned this pull request Apr 18, 2017

Cannot transitively reference a csproj+project.json #1112

Closed

mmitche pushed a commit to mmitche/sdk that referenced this pull request Jun 5, 2020

Adding UsingToolNetFrameworkReferenceAssemblies to true (dotnet#478)

0240739

kzu mentioned this pull request Aug 8, 2024

Additional fix for --no-build in pack with transitive references devlooped/nugetizer#511

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise Transitive Project References to MSBuild elements #478

Raise Transitive Project References to MSBuild elements #478

natidea commented Dec 12, 2016

natidea Dec 12, 2016 •

edited

Loading

dsplaisted Dec 16, 2016

natidea Dec 20, 2016

natidea commented Dec 14, 2016

nguerrera Dec 16, 2016 •

edited by natidea

Loading

dsplaisted Dec 16, 2016

dsplaisted Dec 16, 2016

dsplaisted Dec 16, 2016

dsplaisted Dec 16, 2016

dsplaisted Dec 16, 2016 •

edited by natidea

Loading

natidea commented Dec 21, 2016

natidea commented Dec 21, 2016

natidea commented Dec 21, 2016

ripenko commented Mar 2, 2017

dsplaisted commented Mar 2, 2017

KshitizGIT commented Jun 23, 2017

srivatsn commented Jun 23, 2017 •

edited

Loading

nguerrera commented Jun 23, 2017

srivatsn commented Jun 23, 2017

KshitizGIT commented Jun 23, 2017

Raise Transitive Project References to MSBuild elements #478

Raise Transitive Project References to MSBuild elements #478

Conversation

natidea commented Dec 12, 2016

natidea Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

dsplaisted Dec 16, 2016

Choose a reason for hiding this comment

natidea Dec 20, 2016

Choose a reason for hiding this comment

natidea commented Dec 14, 2016

nguerrera Dec 16, 2016 • edited by natidea Loading

Choose a reason for hiding this comment

dsplaisted Dec 16, 2016

Choose a reason for hiding this comment

dsplaisted Dec 16, 2016

Choose a reason for hiding this comment

dsplaisted Dec 16, 2016

Choose a reason for hiding this comment

dsplaisted Dec 16, 2016

Choose a reason for hiding this comment

dsplaisted Dec 16, 2016 • edited by natidea Loading

Choose a reason for hiding this comment

natidea commented Dec 21, 2016

natidea commented Dec 21, 2016

natidea commented Dec 21, 2016

ripenko commented Mar 2, 2017

dsplaisted commented Mar 2, 2017

KshitizGIT commented Jun 23, 2017

srivatsn commented Jun 23, 2017 • edited Loading

nguerrera commented Jun 23, 2017

srivatsn commented Jun 23, 2017

KshitizGIT commented Jun 23, 2017

natidea Dec 12, 2016 •

edited

Loading

nguerrera Dec 16, 2016 •

edited by natidea

Loading

dsplaisted Dec 16, 2016 •

edited by natidea

Loading

srivatsn commented Jun 23, 2017 •

edited

Loading