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

PrivateAssets for central transitive dependencies should flow regardless whether the parent node is a project or a package #4952

Conversation

marcin-krystianc
Copy link
Contributor

@marcin-krystianc marcin-krystianc commented Nov 29, 2022

Bug

Fixes: NuGet/Home#12276

Regression? Last working version: no

Description

When we check whether a node directly belongs to a root node, we need to look at rootNode.Item.Data.Dependencies instead of targetFrameworkInformation.Dependencies. The former contains projects, but the latter does not.

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A
  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@ghost ghost added the Community PRs created by someone not in the NuGet team label Nov 29, 2022
@marcin-krystianc marcin-krystianc changed the title PrivateAssets for central transitive dependencies should flow regardless whether the parent is a project or a package PrivateAssets for central transitive dependencies should flow regardless whether the parent node is a project or a package Nov 29, 2022
@marcin-krystianc
Copy link
Contributor Author

Next one is #4953

@jeffkl jeffkl self-assigned this Nov 29, 2022
@zivkan
Copy link
Member

zivkan commented Nov 29, 2022

Does this also solve NuGet/Home#10907 by any chance? (non CPM scenarios)

@marcin-krystianc
Copy link
Contributor Author

Does this also solve NuGet/Home#10907 by any chance? (non CPM scenarios)

No, very unlikely. The only significant change here is in the GetLibraryDependenciesForCentralTransitiveDependencies which is relevant only for CPM scenarios and transitively pinned dependencies.

{
LibraryDependency parentDependency = targetFrameworkInformation.Dependencies.FirstOrDefault(i => i.Name.Equals(parentNode.Item.Key.Name, StringComparison.OrdinalIgnoreCase));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

targetFrameworkInformation.Dependencies doesn't contain projects.

@marcin-krystianc marcin-krystianc marked this pull request as ready for review December 6, 2022 09:25
@marcin-krystianc marcin-krystianc requested a review from a team as a code owner December 6, 2022 09:25
@jeffkl
Copy link
Contributor

jeffkl commented Dec 6, 2022

@marcin-krystianc it looks like some unit tests are failing, please take a look.

@marcin-krystianc
Copy link
Contributor Author

@marcin-krystianc it looks like some unit tests are failing, please take a look.

Apologies for that, it is fixed now.

@jeffkl
Copy link
Contributor

jeffkl commented Dec 7, 2022

@marcin-krystianc it looks like some unit tests are failing, please take a look.

Apologies for that, it is fixed now.

No apology necessary, just wanted to make sure you were aware!

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Some test feedback before approving.

LibraryDependency parentDependency = rootNode.Item.Data.Dependencies.FirstOrDefault(i => i.Name.Equals(parentNode.Item.Key.Name, StringComparison.OrdinalIgnoreCase));

// A transitive dependency that is a few levels deep won't be a top-level dependency so skip it
if (parentDependency == null || parentDependency.ReferenceType != LibraryDependencyReferenceType.Direct)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This extra check is necessary, because rootNode.Item.Data.Dependencies contains also centrally managed transitive references which should be ignored here.

@marcin-krystianc
Copy link
Contributor Author

LGTM.

Some test feedback before approving.

Thanks for the heads up, I've simplified the test code a little bit,

@marcin-krystianc marcin-krystianc requested review from nkolev92 and jeffkl and removed request for nkolev92 December 8, 2022 08:48
Copy link
Contributor

@jeffkl jeffkl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

@jeffkl jeffkl merged commit 31f2a1c into NuGet:dev Dec 9, 2022
@marcin-krystianc marcin-krystianc deleted the marcink-20221128-assets_flow_for_project_and_packages branch December 12, 2022 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PRs created by someone not in the NuGet team
Projects
None yet
4 participants