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

Spec proposal for opt-in feature where PrivateAssets flag option indepdent from IncludeAssets/ExcludeAssets options #12313

Closed
Closed
Changes from 3 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
0864d28
Spec proposal for privateAssets not eclipsed by includeassets/exclude…
erdembayar Dec 7, 2022
5cd02a1
Improve example
erdembayar Dec 8, 2022
19bd26e
Address PR comment
erdembayar Dec 8, 2022
58f030a
Rename property to PrivateAssetIndependent
erdembayar Dec 9, 2022
d231ef3
Add project2project scenario
erdembayar Dec 9, 2022
b272b0e
Take Andy's suggestion
erdembayar Dec 9, 2022
fdf8ee8
Clarify flow table
erdembayar Dec 9, 2022
bf84bb4
P2P consumption example
erdembayar Dec 9, 2022
ed3370b
Update proposed/2022/PrivateAssetIndepdentFromExcludeAsset.md
erdembayar Dec 15, 2022
188b8dc
Update proposed/2022/PrivateAssetIndepdentFromExcludeAsset.md
erdembayar Dec 15, 2022
6bf5644
Update proposed/2022/PrivateAssetIndepdentFromExcludeAsset.md
erdembayar Dec 15, 2022
9563247
Update proposed/2022/PrivateAssetIndepdentFromExcludeAsset.md
erdembayar Dec 15, 2022
68f52a1
Update proposed/2022/PrivateAssetIndepdentFromExcludeAsset.md
erdembayar Dec 15, 2022
4cd21af
Update proposed/2022/PrivateAssetIndepdentFromExcludeAsset.md
erdembayar Dec 15, 2022
a1f9f46
Update proposed/2022/PrivateAssetIndepdentFromExcludeAsset.md
erdembayar Dec 15, 2022
3338e2c
Update proposed/2022/PrivateAssetIndepdentFromExcludeAsset.md
erdembayar Dec 15, 2022
dae50f7
Take AArnott suggestion
erdembayar Dec 15, 2022
c76de07
Address Fernando's comment
erdembayar Dec 15, 2022
e9a8465
Correct pack example
erdembayar Dec 15, 2022
148c65f
Add Alternative idea
erdembayar Dec 15, 2022
72bcb2c
Address Andy's comment
erdembayar Dec 19, 2022
2b37bc5
Rename property
erdembayar Jan 10, 2023
271e53b
Add example
erdembayar Feb 16, 2023
1f7b433
Address Nikolche's comments
erdembayar Apr 4, 2023
001fad0
PublicAssets control
erdembayar Apr 5, 2023
a36b5f0
Change to ExcludedAssetsFlow boolean metadata
erdembayar Apr 7, 2023
e24fbf5
Update proposed/2022/PrivateAssetIndepdentFromExcludeAsset.md
erdembayar Apr 7, 2023
3db6950
Update proposed/2022/PrivateAssetIndepdentFromExcludeAsset.md
erdembayar Apr 7, 2023
7311350
Update proposed/2022/PrivateAssetIndepdentFromExcludeAsset.md
erdembayar Apr 7, 2023
8fef1e9
Address remaining comment by Andrew
erdembayar Apr 7, 2023
1d3d357
Correct typos
erdembayar Apr 7, 2023
c9b9bc1
Address remaining comments by Nikolche
erdembayar Apr 7, 2023
df9cca9
Update proposed/2022/PrivateAssetIndepdentFromExcludeAsset.md
erdembayar Apr 10, 2023
0f4b052
Nikolche shorten comment
erdembayar Apr 10, 2023
8693e02
Update proposed/2022/PrivateAssetIndepdentFromExcludeAsset.md
erdembayar Apr 10, 2023
0d53d37
Other comments
erdembayar Apr 10, 2023
1af7f27
More feedback address
erdembayar Apr 10, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
147 changes: 147 additions & 0 deletions proposed/2022/PrivateAssetIndepdentFromExcludeAsset.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
# Opt-in `PrivateAssets` asset flow control option precedence over `IncludeAssets/ExcludeAssets` option

- Author Name <https://github.com/erdembayar>
- Start Date 2022-12-07
- GitHub Issue <https://github.com/NuGet/Home/issues/6938>
- GitHub PR <https://github.com/NuGet/NuGet.Client/pull/4976>

## Summary

<!-- One-paragraph description of the proposal. -->
Currently `IncludeAssets/ExcludeAssets` options take precedence over `PrivateAssets` asset control option when creating package, so in many cases `IncludeAssets/ExcludeAssets` options completely eclipse the `PrivateAssets` option so doesn't let assets flow to consuming parent project. This proposal introduces new a `PackPrivateAssetsFlow` opt-in property so it let `PrivateAssets` asset control option takes precedence over `IncludeAssets/ExcludeAssets` options to enable asset flow to consuming parent projects.
erdembayar marked this conversation as resolved.
Show resolved Hide resolved
erdembayar marked this conversation as resolved.
Show resolved Hide resolved
erdembayar marked this conversation as resolved.
Show resolved Hide resolved

## Motivation

<!-- Why are we doing this? What pain points does this solve? What is the expected outcome? -->
A package author should be able control which asset flow to parent consuming projects when this project is consumed as package. It enables parent projects to consume transitive packages without having to directly reference them, therefore it reduces the number of packages developers need to keep track of.
erdembayar marked this conversation as resolved.
Show resolved Hide resolved
We couldn't make this default experience because it'll break customers who rely on current behavior.
erdembayar marked this conversation as resolved.
Show resolved Hide resolved

## Explanation
erdembayar marked this conversation as resolved.
Show resolved Hide resolved

### Functional explanation

<!-- Explain the proposal as if it were already implemented and you're teaching it to another person. -->
<!-- Introduce new concepts, functional designs with real life examples, and low-fidelity mockups or pseudocode to show how this proposal would look. -->
The new ` PackPrivateAssetsFlow` property only affects pack operation (more specifically nuspec in nupkg file), but doesn't affect restore experience for current project so there would be no change in `project.assets.json` lock file.
erdembayar marked this conversation as resolved.
Show resolved Hide resolved
Experience for parent consuming project would be affected by which asset from `compile, runtime, contentFiles, build, buildMultitargeting, buildTransitive, analyzers, native` are flowing into them.

For the following table assume `PackPrivateAssetsFlow` is set `true` when creating package, iterating possible scenarios (not full list) for consuming parent project.

| Asset flowing to parent project | New feature enabled | Possible downside |
erdembayar marked this conversation as resolved.
Show resolved Hide resolved
|-----------------------|--------------|-----------------|
| compile | Expose new code, auto complete, intellicode, intellisense | Compile error due to naming ambiguity, run time error, NU1605 error |
erdembayar marked this conversation as resolved.
Show resolved Hide resolved
| runtime | Let provide new runtime dependency | Run time error |
| build | Enable msbuild imports | Build fails due to property/target value change |
| analyzers | Code analyzers work | n/a |

#### Examples

##### Case 1

Package reference in csproj file.

```.net
<PropertyGroup>
<TargetFramework>netstandard2.0</TargetFramework>
<VersionSuffix>beta</VersionSuffix>
erdembayar marked this conversation as resolved.
Show resolved Hide resolved
<PackPrivateAssetsFlow>True</PackPrivateAssetsFlow>
</PropertyGroup>
<ItemGroup>
<PackageReference Include="Microsoft.Windows.CsWin32" Version="0.2.138-beta" PrivateAssets="none" ExcludeAssets="build" />
</ItemGroup>

Before change nuspec file:

```.net
<dependencies>
<group targetFramework=".NETStandard2.0">
<dependency id="Microsoft.Windows.CsWin32" version="0.2.138-beta" exclude="Runtime,Build,Native,Analyzers,BuildTransitive" />
erdembayar marked this conversation as resolved.
Show resolved Hide resolved
erdembayar marked this conversation as resolved.
Show resolved Hide resolved
</group>
</dependencies>
```

After change nuspec file:

```.net
<dependencies>
<group targetFramework=".NETStandard2.0">
<dependency id="Microsoft.Windows.CsWin32" version="0.2.138-beta" include="All" />
zivkan marked this conversation as resolved.
Show resolved Hide resolved
erdembayar marked this conversation as resolved.
Show resolved Hide resolved
</group>
</dependencies>
```

##### Case 2

Package reference in csproj file.

```.net
<PropertyGroup>
<TargetFramework>netstandard2.0</TargetFramework>
<VersionSuffix>beta</VersionSuffix>
erdembayar marked this conversation as resolved.
Show resolved Hide resolved
<PackPrivateAssetsFlow>True</PackPrivateAssetsFlow>
</PropertyGroup>
<ItemGroup>
<PackageReference Include="Microsoft.Windows.CsWin32" Version="0.2.138-beta" PrivateAssets="none" IncludeAssets="none" />
</ItemGroup>
```

Before change nuspec file:

```.net
<dependencies>
<group targetFramework=".NETStandard2.0" />
</dependencies>
```

After change nuspec file:

```.net
<dependencies>
<group targetFramework=".NETStandard2.0">
<dependency id="Microsoft.Windows.CsWin32" version="0.2.138-beta" include="All" />
</group>
</dependencies>
```

### Technical explanation

We already have a [logic](hhttps://github.com/NuGet/NuGet.Client/blob/380415d812681ebf1c8aa0bc21533d4710514fc3/src/NuGet.Core/NuGet.Commands/CommandRunners/PackCommandRunner.cs#L577-L582) that combines `IncludeAssets/ExcludeAssets` options with `PrivateAssets`, we just change precedence on that logic depending on opt-in property.

## Drawbacks

<!-- Why should we not do this? -->
- Might break some consuming projects, that is why it's opt-in feature. See table above for more details.

## Rationale and alternatives
erdembayar marked this conversation as resolved.
Show resolved Hide resolved

<!-- Why is this the best design compared to other designs? -->
<!-- What other designs have been considered and why weren't they chosen? -->
<!-- What is the impact of not doing this? -->
- We could make it per package level control metadata. But could be hard to use if the customer want to opt-in for all package references in current project.
erdembayar marked this conversation as resolved.
Show resolved Hide resolved

`<PackageReference Include="contoso" Version="1.1.1" PackPrivateAssetsFlow="true"/>`

- We could make it opt-in option in `nuget.config` file, but it doesn't give customer to option to opt in/out for per project level.
erdembayar marked this conversation as resolved.
Show resolved Hide resolved
erdembayar marked this conversation as resolved.
Show resolved Hide resolved

```.net
<config>
<add key="PackPrivateAssetsFlow" value="true" />
erdembayar marked this conversation as resolved.
Show resolved Hide resolved
</config>
```

## Prior Art

<!-- What prior art, both good and bad are related to this proposal? -->
<!-- Do other features exist in other ecosystems and what experience have their community had? -->
<!-- What lessons from other communities can we learn from? -->
<!-- Are there any resources that are relevant to this proposal? -->

## Unresolved Questions

<!-- What parts of the proposal do you expect to resolve before this gets accepted? -->
<!-- What parts of the proposal need to be resolved before the proposal is stabilized? -->
<!-- What related issues would you consider out of scope for this proposal but can be addressed in the future? -->
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this will work with CPM enabled projects? See #12270

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeffkl
Could you be able to check if my changes affect any of CPM scenarios?


## Future Possibilities

<!-- What future possibilities can you think of that this proposal would help with? -->