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

Transitive references #199

Closed
BillHiebert opened this issue May 25, 2016 · 30 comments
Closed

Transitive references #199

BillHiebert opened this issue May 25, 2016 · 30 comments
Labels
Feature Request Request for a new feature, or new scenario to be handled. Parity-XProj Missing features from xproj/project.json project system from Visual Studio 2015.

Comments

@BillHiebert
Copy link
Contributor

◦xproj you only need to declare top level dependencies and others are pulled in automatically
◦This also has an impact on the publish artifacts. csproj only pulls from referenced assemblies and has special logic to exclude assemblies which are not actually used in code.

@BillHiebert BillHiebert added the Parity-XProj Missing features from xproj/project.json project system from Visual Studio 2015. label May 25, 2016
@srivatsn srivatsn added the Feature Request Request for a new feature, or new scenario to be handled. label May 26, 2016
@davkean
Copy link
Member

davkean commented Jun 7, 2016

We think the cost here is going to be in two places:

  • MSBuild work to actually flow the references
  • UI work in the ReferencesSubTreeProvider to show inherited dependencies (xproj shows them under the project that it was inherited from)

We think that the language service will basically get this for free if MSBuild raises them as normal <Reference/> and <ProjectReference>.

@srivatsn srivatsn added this to the 1.0 RC milestone Jun 16, 2016
@davkean davkean modified the milestones: 1.1, 1.0 RC Jul 5, 2016
@srivatsn srivatsn modified the milestones: 1.1, 1.0 RC Aug 26, 2016
@srivatsn srivatsn modified the milestones: 1.0 RTM, 1.0 RC Sep 16, 2016
@brthor
Copy link

brthor commented Oct 4, 2016

Any way we could schedule this for RC? We're running into issues migrating projects with nested project dependencies, including cli repo itself.

@srivatsn
Copy link
Contributor

srivatsn commented Oct 5, 2016

Unlikely. This is a fairly large work item that has compat implications. Can the migration tool flatten the hierarchy?

@joelverhagen
Copy link
Member

@brthor, @srivatsn, running into the same issue. Could the whole closure of framework references be include when migrate writes the csproj? Similar to project references.

@joelverhagen
Copy link
Member

Lacking transitivity for projects is a problem for dotnet pack3 as well.

For example, in NuGet, one of our "entry point" packages is NuGet.CommandLine.XPlat (on NuGet.org). The project.json for this package has one project reference (NuGet.Commands) and several package references. dotnet pack on the project.json results in a .nupkg with symmetrical package dependencies.

After migrating this project to .csproj and using pack3, the resulting .nupkg has these additional package dependencies:

NuGet.Client
NuGet.Common
NuGet.Configuration
NuGet.ContentModel
NuGet.DependencyResolver
NuGet.DependencyResolver.Core
NuGet.Frameworks
NuGet.LibraryModel
NuGet.Packaging
NuGet.Packaging.Core
NuGet.Packaging.Core.Types
NuGet.ProjectModel
NuGet.Protocol.Core.Types
NuGet.Protocol.Core.v3
NuGet.Repositories
NuGet.RuntimeModel
NuGet.Versioning

@davkean
Copy link
Member

davkean commented Oct 14, 2016

I don't think that's related - this seems like a pack issue. Whether a project reference is transitive or not shouldn't affect when it ends up in a dependency in the nupkg.

@davkean
Copy link
Member

davkean commented Oct 14, 2016

Actually, I think it's a conversion work item we should mark the references so that they don't turn up as refs during the conversion.

@davkean
Copy link
Member

davkean commented Oct 14, 2016

@brthor ^

@brthor
Copy link

brthor commented Oct 14, 2016

So is migration supposed to promote the refs or not promote them for pack?

We have this issue open for the migration work to promote refs and project refs,

https://github.com/dotnet/cli/issues/4414

Should the promoted refs just be marked PrivateAssets All? will that fix the pack scenario? @joelverhagen

@davkean
Copy link
Member

davkean commented Oct 14, 2016

Still promote the refs - should mark them with the metadata to opt them out of refs for pack. Work with NuGet.

@brthor
Copy link

brthor commented Oct 14, 2016

Talked with @joelverhagen offline, PrivateAssets should do the trick

@srivatsn
Copy link
Contributor

srivatsn commented Nov 17, 2016

This is the current proposal for how we could implement this:

  • References to Projects that produce packages can be represented in the project as <PackageReference Include="PathToProject.csproj">. Lack of the version attribute could be used to statically determine the distinction between a package referenced by name\version and a package referenced by path (i.e a Project reference to a package).
  • NuGet will write to the assets file all the projects that are resolved this way.
  • The targets in the SDK should read the lock file and raise the closure of P2Ps as ResolvedProjectReferences so that they can be passed to the compiler.
  • Adding a Reference to a project from the UI should write a PackageReference to the project file if the target project can produce a package - this will be determined by checking if the target project has a PackageId.
  • Pack needs to understand this new pattern so that it can write the proper dependencies in the project. Currently pack reads attributes on a ProjectReference and that will have to be switched to read from the PackageReference instead.
  • The solution build manager will not see the transitive P2Ps but that should be fine since it builds the solution in dependency order. However solution build manager needs to be told about the first level PackageReference that's a projectref.

Tagging @kieranmo @davidfowl @davkean @yigalataz @rrelyea. Let me know if I missed anything from yesterday's discussion.

@srivatsn
Copy link
Contributor

srivatsn commented Nov 17, 2016

Distilling the workitems from the above proposal:

@davidfowl
Copy link
Member

davidfowl commented Nov 18, 2016

The reason it needs to be on PackageReference instead of project reference is because we want to enable swapping the package with a project without changing the references in the project file.

Imagine we started with this:

<PackageReference Include="Microsoft.AspNetCore.Mvc" Version="1.0.0" />

Then I want to swap the package for a project.

<PackageReference Include="Microsoft.AspNetCore.Mvc" Version="1.0.0" Project="$(Microsoft.AspNetCore.Mvc)" />

I would import a file in all of my projects that set the $(Microsoft.AspNetCore.Mvc) property appropriately when I did a specific gesture... It could even be inferred by name or something...

Now my packages automagically flip to projects and we can step in and debug 😄 . We do this trick today with ASP.NET Core to seamlessly work across solutions (we have like ~50 repositories).

@NTaylorMullen
Copy link

If i'm understanding this fix correctly we'll also need to stop migrating transitive framework assemblies in the CLI.

@dsplaisted
Copy link
Member

dsplaisted commented Nov 18, 2016

@davidfowl You could also use a target to replace a PackageReference with a ProjectReference. So they don't necessarily need to be the same tag in order to make the scenario work.

@davidfowl
Copy link
Member

@dsplaisted no, the point is I don't want to replace anything in the project at all. So explain to me how I would do the exact same thing? Where the presence of a property in an import automagically changes all of my references from package to project in a way the IDE is ok with?

@nguerrera
Copy link
Contributor

Ok, that's a good scenario. I like packagereference with project metadata for that. It is different from Sri's proposal, though. I'm pushing back against a package reference that has only a project path.

@srivatsn
Copy link
Contributor

We discussed both those syntaxes and the concern with the <PackageReference Include="Id of package" Project="Path to project" /> was that the PackageId is redundant information since the source of truth for that is in the target project. If this name differs and we ignore it that causes confusion.

Referencing something by name or path is also the existing pattern for <Reference> items

@srivatsn srivatsn modified the milestones: 1.0 RC3, 1.0 RTM Nov 18, 2016
@nguerrera
Copy link
Contributor

nguerrera commented Nov 18, 2016

I'm missing how that addresses @davidfowl's scenario. If the projects are spread across repos and I want to be able to build from packages sometimes and projects other times, the projects might not even be on my machine. So the ID is not redundant when I don't have the project in hand.

Or would he instead be expected to do <PackageReference Include="$(Microsoft.AspNetCore.Mvc) Version="$(Microsoft.AspNetCore.Mvc.Version)" /> and toggle $(Microsoft.AspNetCore.Mvc) between a csproj path and ID and toggle $(Microsoft.AspNetCore.Mvc.Version) between 1.0.0 and empty?

@dsplaisted
Copy link
Member

@davidfowl

no, the point is I don't want to replace anything in the project at all. So explain to me how I would do the exact same thing? Where the presence of a property in an import automagically changes all of my references from package to project in a way the IDE is ok with?

You don't modify the project file itself, the import has a target that modifies the set of ProjectReference and PackageReference items. A very simple one would look something like this:

<Target Name="ReplacePackageReferences" BeforeTargets="_LoadRestoreGraphEntryPoints;PrepareProjectReferences">
    <PackageReference Remove="Microsoft.AspNetCore.Mvc" />
    <ProjectReference Include="$(Microsoft.AspNetCore.Mvc)" />
</Target>

This removes any PackageReference to Microsoft.AspNetCore.Mvc and adds a ProjectReference before the rest of the targets consume those items. For a real solution, you would probably want to write a task that would help you only add the ProjectReference if the PackageReference for Microsoft.AspNetCore.Mvc existed, and perhaps other conditions.

@srivatsn
Copy link
Contributor

@nguerrera we can do what @dsplaisted suggests for PackageReference as well to switch. As for PackageReference vs ProjectReference the argument that was made in favour of PackageReference was that ProjectReferences are references to projects producing assemblies and that's an existing concept and those are not transitive. Referencing a package is a new concept and packages are transitive regardless of whether they are referenced by name\version or a path to a project that produces one

@nguerrera
Copy link
Contributor

I don't think everyone who wants transitive P2P refs would think of their projects as "producing nuget packages". I can imagine lots of customers with big solutions that never package their stuff, but still want the convenience of transitive P2P.

natidea added a commit to natidea/roslyn-project-system that referenced this issue Dec 7, 2016
…et so that it can write them (and their closures) to the assets file. This is to partly address transitive project references dotnet#199
@romerod
Copy link

romerod commented Dec 8, 2016

@davidfowl when swapping a package with a project should the project be open in the solution or do you work in another visualstudio instance? And will the UI take care not removing the Project="$(Microsoft.AspNetCore.Mvc)" part when updating a project?

@srivatsn
Copy link
Contributor

srivatsn commented Jan 6, 2017

This is now done.

@srivatsn srivatsn closed this as completed Jan 6, 2017
eric62369 pushed a commit to eric62369/project-system that referenced this issue Jul 10, 2020
…115.1 (dotnet#199)

- Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19565.1
kzu added a commit to devlooped/nugetizer that referenced this issue Aug 8, 2024
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
kzu added a commit to devlooped/nugetizer that referenced this issue Aug 8, 2024
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
kzu added a commit to devlooped/nugetizer that referenced this issue Aug 8, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Request for a new feature, or new scenario to be handled. Parity-XProj Missing features from xproj/project.json project system from Visual Studio 2015.
Projects
None yet
Development

No branches or pull requests

10 participants