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

Document references peculiarities #8494

Merged
merged 7 commits into from
Apr 23, 2023

Conversation

JanKrivanek
Copy link
Member

Relates to:
#4795
#8405

Context

Documenting some aspect of tailoring behavior of references. Capturing as well resolution/workarounds for the listed comunity reported issues

@JanKrivanek JanKrivanek force-pushed the doc/dependencies-behavior branch from 435ed6c to aa7b9c8 Compare February 23, 2023 17:10
Copy link
Member

@rainersigwald rainersigwald 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 writing this! There are many subtle behaviors and interconnected systems here so I have a bunch of nitpicky comments and clarifications but am very excited to have this.

cc @ghogen -- some of this could be customer facing at some point, though I think it's a bit too complex/confusing/detail-oriented at the moment.

documentation/README.md Outdated Show resolved Hide resolved
documentation/wiki/Controlling-Dependencies-Behavior.md Outdated Show resolved Hide resolved
documentation/wiki/Controlling-Dependencies-Behavior.md Outdated Show resolved Hide resolved
documentation/wiki/Controlling-Dependencies-Behavior.md Outdated Show resolved Hide resolved
Comment on lines 46 to 47
// This is allowed unless DisableTransitiveProjectReferences=true is passed into build.
// private Domain.PersonTable _tbl;
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately this is kind of a bad example; the C# compiler should see the full closure of dependencies in any case--and as analyzers and nullability analysis increase, is getting more persnickety about this. At some point in the future this type of library reference may be required to be transitive--but all of the MSBuild stuff and motivations will stay good.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does this still feel as bad example after this #8494 (comment)?

documentation/wiki/Controlling-Dependencies-Behavior.md Outdated Show resolved Hide resolved
documentation/wiki/Controlling-Dependencies-Behavior.md Outdated Show resolved Hide resolved
documentation/wiki/Controlling-Dependencies-Behavior.md Outdated Show resolved Hide resolved
documentation/wiki/Controlling-Dependencies-Behavior.md Outdated Show resolved Hide resolved

**Notes:**

This will properly enforce the framework for the dependency chain. The output folder will contain proper version of the direct dependency - Repository Layer. The transitive dependencies might overbuild, and output folder of current project (Service Layer) might contain both versions of the transitive project dependency (Domain-net48.dll and Domain-netstd20.dll). This limitation can be workarounded by switching of the transitive project dependencies via `DisableTransitiveProjectReferences` (same as shown in [Access to transitive project references](#access-to-transitive-project-references))
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand this. Is it something like "once the first-level reference with overridden TF is hit, its references get normal TF resolution", so you might get different TFs on a transitive reference and have them both build?

Copy link
Member Author

Choose a reason for hiding this comment

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

Based on this and above feedback I attempted to reformulate this: https://github.com/dotnet/msbuild/pull/8494/files#diff-b5dec84f84d1f113f1c8d8369daf2a8b4958736d3e02764f3bfbd42e42c4a917R243-R246

Review would be appreciate - to make sure it's accurate and understandable

documentation/wiki/Controlling-Dependencies-Behavior.md Outdated Show resolved Hide resolved

For [.NET SDK projects](https://learn.microsoft.com/dotnet/core/project-sdk/overview) restore operation by default makes all transitive references accessible as if they were direct references.

This is required by the compiler and analyzers to be able to properly inspect the whole dependency or/and inheritance chain of types when deciding about particular checks.
Copy link
Member

Choose a reason for hiding this comment

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

Technically the full closure isn't required by the compiler today, but the compiler team wishes it was, and new analysis can cause not-required-today assembly references to be required tomorrow.

Copy link
Member Author

@JanKrivanek JanKrivanek Mar 28, 2023

Choose a reason for hiding this comment

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

Good point. I reworded (required by -> provided for), to prevent incorrect statement.

documentation/wiki/Controlling-Dependencies-Behavior.md Outdated Show resolved Hide resolved
}
```

## Access to transitive package references

The transitive access to dependencies works by default for package dependencies as well. This can be opted out via `PrivateAssets=compile` on the `PackageReference` of the concern. (More details on [Controlling package dependency assets](https://learn.microsoft.com/en-us/nuget/consume-packages/package-references-in-project-files#controlling-dependency-assets))
The transitive access to references works by default for package references as well. This can be opted out via `PrivateAssets=compile` on the `PackageReference` of the concern. (More details on [Controlling package dependency assets](https://learn.microsoft.com/nuget/consume-packages/package-references-in-project-files#controlling-dependency-assets))
Copy link
Member

Choose a reason for hiding this comment

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

Is there actually a way to reference a package and not its transitive closure? PrivateAssets affects only things that reference you, so you would have to change the package you want to reference.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. It was ment so in the example, but the description was very unclear and ambiguous.

I attempted to clarify it and explicitly call out the behavior.

JanKrivanek and others added 2 commits March 28, 2023 09:36
@rainersigwald rainersigwald added the Documentation Issues about docs, including errors and areas we should extend (this repo and learn.microsoft.com) label Mar 28, 2023
@JanKrivanek JanKrivanek added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Apr 21, 2023
@JaynieBai JaynieBai merged commit fbbcaf8 into dotnet:main Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Issues about docs, including errors and areas we should extend (this repo and learn.microsoft.com) merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants