dotnet / msbuild

The Microsoft Build Engine (MSBuild) is the build platform for .NET and Visual Studio.
https://docs.microsoft.com/visualstudio/msbuild/msbuild
MIT License
5.22k stars 1.35k forks source link

Investigate and list ProjectReference and PackageReference behavioral divergences #8507

Closed JanKrivanek closed 1 year ago

JanKrivanek commented 1 year ago

Motivation

8398 needs investigation of what we can support (and how) in converting between the two.

There might be possible inconsitencies between PackageReference and ProjectReference items metadata (and even behavior with same metadata - e.g. SetTargetFramework has different behavior for the two: https://github.com/NuGet/Home/issues/12436, PrivateAssets as well: https://github.com/dotnet/msbuild/issues/4371#issuecomment-815793382, etc.).

Expected outputs

nkolev92 commented 1 year ago

At some point I did some digging about issues that contain this type of information and creating an issue for a future investigation. I don't think my issue has anything you don't know, but figured I'd share it:

PackageReference has PrivateAssets/ExcludeAssets/Include assets capabilities and those are respected. 
Those also easily translate into the dependencies in the generated package. 

ProjectReferences *do not* have the same equivalent experience. Realistically only the ReferenceOutputAssembly and PrivateAssets=all metadata work. 

Basically, there's absolutely no way to express a "runtime only" ProjectReference, the same way you can express a PackageReference. This feature covers the rethinking of all this metadata. 
Should project ref support all this metadata the same way PackageReference does? Should it? Gotchas/caveats?

See Re: How to express a package dependency without the weight of a P2P ref?

Project/PackageReference story

https://github.com/NuGet/Home/issues?q=is%3Aissue+ReferenceOutputAssembly+is%3Aopen
https://github.com/dotnet/sdk/search?q=ReferenceOutputAssembly&type=Issues
https://github.com/microsoft/msbuild/search?q=ReferenceOutputAssembly&type=Issues
nkolev92 commented 1 year ago

I forwarded the e-mail my write-up references :)

JaynieBai commented 1 year ago

PackageReference Metadata Lists

Tag Description Default Value
Name The display name of the reference Optional string
Version that's used when restoring packages Optional string, but if not set, it will have warning NU1604
Aliases When an alias is specified, all assemblies coming from the annotated package will need to be referenced with an alias. Optional string
NoWarn hide specific warnings, either project-wide or package-wide. Optional string
IncludeAssets These assets will be consumed all
ExcludeAssets These assets will not be consumed none
PrivateAssets These assets will be consumed but won't flow to the parent project contentfiles;analyzers;build
GeneratePathProperty It generates a variable for each package that contains the full path to the package version being used, when it's true. The variable format prepends Pkg to the package name and replaced any period (.) with an underscore (_). That makes the variable for Prism.Core into PkgPrism_Core. false

ProjectReference Metadata Lists

Tag Description Default Value
Aliases When an alias is specified, all references coming from the project will need to be referenced with an alias. Optional string
Name The display name of the reference Optional string
GlobalPropertiesToRemove Names of properties to remove when building the referenced project, for example RuntimeIdentifier;PackOnBuild Optional string[]
Project A GUID for the reference Optional string
OutputItemType Item type to emit target outputs into. Default is blank. If the Reference metadata is set to "true" (default) then target outputs will become references for the compiler. Optional string
ReferenceOutputAssembly If set to false, does not include the output of the referenced project as a Reference of this project, but still ensures that the other project builds before this one true
Private Specifies whether the reference should be copied to the output folder. This attribute matches the Copy Local property of the reference that's in the Visual Studio IDE. Optional Boolean
SetConfiguration Sets the global property Configuration for the referenced project, for example Configuration=Release. Optional string
SetPlatform Sets the global property Platform for the referenced project, for example Platform=AnyCPU. Optional string.
SetTargetFramework Sets the global property TargetFramework for the referenced project, for example TargetFramework=netstandard2.0. Optional string
SkipGetTargetFrameworkProperties If true, builds the referenced project without negotiating the most compatible TargetFramework value. false
PlatformLookupTable Some cases of ProjectReferences require a $(PlatformLookupTable) to correctly determine what a referenced project should build as. References between managed and unmanaged projects also get a default lookup table that can be opted out of by setting the property UseDefaultPlatformLookupTables to false. Details in References between managed and unmanaged projects Optional string
Targets Semicolon separated list of targets in the referenced projects that should be built. Default is the value of $(ProjectReferenceBuildTargets) which defaults to empty, indicating the default targets. Optional string[]
IncludeAssets These transitive reference assets will be consumed. all
ExcludeAssets These transitive reference assets will not be consumed none
PrivateAssets These direct and transitive reference assets will be consumed, but won't flow to the downstream project contentfiles;analyzers;build
JaynieBai commented 1 year ago

Differences between PackageReference and ProjectReference

Tag PackageReference ProjectReference IsSame Comments
Name ☑️ ☑️
Aliases ☑️ ☑️
Version ☑️
NoWarn ☑️ ☑️ 1. NoWarn on a package reference does not apply transitively to its dependencies https://github.com/NuGet/Home/issues/5740 2. projectReference. https://github.com/NuGet/Home/issues/8093.
GeneratePathProperty ☑️
IncludeAssets ☑️ ☑️ 1. In package reference, IncludeAssets works for transitive and direct references 2. In project reference, IncludeAssets works for transitive references
ExcludeAssets ☑️ ☑️ 1. In package reference, ExcludeAssets works for transitive and direct references 2. In project reference, ExcludeAssets works for transitive references
PrivateAssets ☑️ ☑️
GlobalPropertiesToRemove ☑️
Project ☑️
OutputItemType ☑️
ReferenceOutputAssembly ☑️
Private ☑️
SetConfiguration ☑️ https://github.com/NuGet/Home/issues/7868#issuecomment-519277759
SetPlatform ☑️ https://github.com/NuGet/Home/issues/7868#issuecomment-519277759
SetTargetFramework ☑️ https://github.com/NuGet/Home/issues/7868#issuecomment-519277759 https://github.com/NuGet/Home/issues/12436
SkipGetTargetFrameworkProperties ☑️ https://github.com/NuGet/Home/issues/7868#issuecomment-519277759
PlatformLookupTable ☑️ https://github.com/NuGet/Home/issues/7868#issuecomment-519277759
Targets ☑️

Appart from those metadata differences - other group of differences is nuget support for additional assets - especially build, buildMultitargeting, buildTransitive, analyzers, that are not supported by ProjectReference and do not have existing available workaround analogy.

Attach some examples for ExcludeAssets IncludeAssets and PrivateAssets in excel InvestigationExamples.xlsx

nkolev92 commented 1 year ago

Hey @JaynieBai

Excellent list!

I think the NoWarn part for ProjectReference might not be 100% correct. ProjectReference do support NoWarn too. The issue linked is closed as a dup for transitive packages.

Are you seeing anything different in your tests?

JaynieBai commented 1 year ago

Hey @JaynieBai

Excellent list!

I think the NoWarn part for ProjectReference might not be 100% correct. ProjectReference do support NoWarn too. The issue linked is closed as a dup for transitive packages.

Are you seeing anything different in your tests?

Thanks for pointing it out. I didn't find any direct warnings about project reference currently. But it couldn't suppress transitive warning and referenced project warning now. So mark it not supported temporarily.

nkolev92 commented 1 year ago

Not sure what you mean by I didn't find any direct warnings about project reference currently

I think NoWarn is equivalent for ProjectReference and PackageReference.

The lack of transitive support is a big change and it applies to more than just NoWarn.

JaynieBai commented 1 year ago

Taks an example from https://github.com/JanKrivanek/msbuild/blob/doc/dependencies-behavior/documentation/wiki/Controlling-Dependencies-Behavior.md#access-to-transitive-project-references Repository Project defination:


  <PropertyGroup>
    <TargetFramework>net7.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Newtonsoft.Json"/>
  </ItemGroup>

  <ItemGroup>
    <ProjectReference Include="..\Domain\Domain.csproj" />
  </ItemGroup>

</Project>
namespace Repository
{
    public class Persona
    {
        private Domain.PersonTable _tbl;
    }
}

Build it and there will generate following warnings.
image

In Service Project definition.

  <PropertyGroup>
    <TargetFramework>net7.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <ItemGroup>
      <ProjectReference Include="..\Repository\Repository.csproj">
          <NoWarn>NU1701;NU1602</NoWarn>
      </ProjectReference>
  </ItemGroup>

</Project>

Not sure what you mean by I didn't find any direct warnings about project reference currently

NU1701;NU1602 is transitive reference warning. I can't find a warning that is generated from projectReference. And in https://learn.microsoft.com/en-us/nuget/reference/errors-and-warnings. It specifies NoWarn scope as Project, PackageReference

JaynieBai commented 1 year ago

Attach some examples for ExcludeAssets IncludeAssets and PrivateAssets in excel InvestigationExamples.xlsx

nkolev92 commented 1 year ago

If you take a project that targets net5.0 and add a project reference to a project that targets net472, you'll get a NU1702 which should be suppressible.

I've created an issue to fix the doc to make it clearer: https://github.com/NuGet/docs.microsoft.com-nuget/pull/3038.

nkolev92 commented 1 year ago

OK, so the bug for ProjectReference NoWarn is https://github.com/NuGet/Home/issues/8093.

JaynieBai commented 1 year ago

Discussed offline with @JanKrivanek about transitive assembly references.

From @JaynieBai When project A has an assembly reference. And project B references project A. But assembly reference is not in the project B dependency lists, B can't reference the transitive assembly. But for package reference and project reference, B can reference the transitive references. How to let projectB reference projectA assembly without setting the assembly in the project B again?

From @JanKrivanek Hi Jenny - transitive project and package references were added within the .net sdk style project as a feature primarily for the compiler. It might be that it was never intended to be extended to Assembly References as well - we'll check with @rainersigwald - as he likely has the background info.

rainersigwald commented 1 year ago

Assembly references in .NET are generally required to be transitive. At runtime, if you call code from B that uses a type from A, the .NET assembly loader will fail if A cannot be loaded.

At build time, the compiler may or may not need the full transitive closure--it will definitely need to see A if the public API of B returns a type from A, but if the use of A is all internal implementation details of B, the compiler may be able to succeed without access to A. However, the contract of the compiler is that you should pass the full closure, and if you don't and a new compiler requires more assemblies to do deeper analysis on the same code, that is not considered a breaking change in the compiler.

All of that has been the same for a very long time; there's no change introduced in .NET Core or the .NET SDK compared to (for example) a set of VS2010 projects.

Prior to SDK-enabled transitive project references, the closure was computed entirely within ResolveAssemblyReferences per-project. RAR will attempt to find the transitive closure of references unless instructed otherwise.

In a fully old-school project what would happen is:

  1. A builds, and produces A.dll in a folder, say A\bin\debug\A.dll
  2. B builds, referencing A.dll from that folder and copying it to B\bin\debug\A.dll
  3. Something that references A builds a. ResolveProjectReferences returns the path to B\bin\debug\B.dll b. RAR looks at B.dll and sees that it has a reference to A c. RAR looks "next to" B.dll and finds A.dll d. MSBuild gives both A.dll and B.dll to the compiler e. this project then copies both to its bin directory so they're available at runtime.

A big difference between this approach and the SDK's default transitive ProjectReferences is how A.dll is presented to RAR and whether it's marked as a direct reference.

JanKrivanek commented 1 year ago

@JaynieBai - do you plan any other work on this task? Or can this be closed? (Feels complete from my point of view. And thanks again for the help!)

JaynieBai commented 1 year ago

I don't have any plan for this work now. So, I think we can close that.