Skip to content

Conversation

@davkean
Copy link
Contributor

@davkean davkean commented Sep 8, 2020

Bug

This enables custom CPS projects including C++/MSIX projects to be restored.

Fix

This is easier to review commit by commit.

PR #3618 unblocked adding the CPS project to the VSSolutionManager cache and enable the Package Manager UI, it was, however, enough to unblock actual restore of these projects. After these changes, custom CPS projects can be restored to produce an assets file on disk [1].

Major changes:

  • No longer require a project to define the "TargetFramework/TargetFrameworks" properties. These are .NET SDK specific concepts that don't exist in other project types. TargetFrameworkMoniker, however, is still required. All projects have these due to MSBuild defaults.
  • No longer filter C++ out of the CPS check. C++ is CPS-based, but does not define the CPS capability (which indicates that CPS owns the "hierarchy").
  • Make sure the project actually defines the PackageReferences capabilities.

[1] There is a still a dependency on the .NET SDK to convert the assets file -> references, copy to bin items, etc which will be tracked separately.

Testing/Validation

TBD.

Tests Added: Yes/No
Reason for not adding tests:
Validation:

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.

@davkean Is this meant for 16.9?
16.8 is a bit tight.

@nkolev92 nkolev92 self-assigned this Sep 9, 2020
@nkolev92 nkolev92 added the Community PRs created by someone not in the NuGet team label Sep 9, 2020
@davkean
Copy link
Contributor Author

davkean commented Sep 9, 2020

@nkolev92 This is all hidden behind the ProjectReference capability for a project type; whether we enable C++/MSIX that for 16.8 or not, can be discussed. But we should merge this in to unblock testing across these project types.

@nkolev92
Copy link
Member

nkolev92 commented Sep 9, 2020

@davkean
That makes sense, let's just merge when PR is ready.

Can you see the tests?
https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=4049218&view=ms.vss-test-web.build-test-results-tab&runId=15614548&resultId=100237&paneView=debug

There are some failures.

Running the end to end tests is not documented publicly (it's in a one note), it's one of those things I haven't had time to move yet.

I'll clean-up the doc to unblock you.

@zivkan
Copy link
Member

zivkan commented Sep 9, 2020

I'm a bit confused and concerned about the comment that this will enable PackageReference in C++ projects. Back in March, a C++ PM publicly stated that they don't intend on supporting PackageReference. This PR appears to contradict their statement, although I recognise that you're not in the C++ team.

This PR changes how NuGet works in VS, but if the customer tries to restore the C++ project from the command line, what behaviour are they going to get? One of the concerns about supporting PackageReference is that it might require changes not only in NuGet, but also the c++ project system, otherwise there are some bits and pieces that won't work. Are they committed to making changes to fix bugs from allowing initial/partial C++ PackageReference restore support?

I'm worried that if this enables C++ projects to restore PackageReference in Visual Studio, but that support is either incomplete (needing work from the C++ team), or doesn't work the same on the command line/CI agent, that there's going to be periodic bug reports from customers, that the NuGet and C++ teams will keep reassigning to the other team, before it eventually gets closed as By Design, without ever fixing the customer's problem. Personally, I'd rather not add half-working, half-broken C++ PackageReference support.

@davkean
Copy link
Contributor Author

davkean commented Sep 10, 2020

@zivkan I'm working on the C++ package reference support, hence the PR.

@nkolev92
Copy link
Member

A further clarification, it's not support for all C++ scenarios, but rather C++/CLI only.

@davkean
Copy link
Contributor Author

davkean commented Sep 10, 2020

I missed the context that probably makes this easier to understand:

  • C++ uses CPS under the covers for a variety of features.
  • C++ will be using CPS's PackageReference support under certain circumstances, that will at first limited to C++/CLI targeting .NET Core.
  • MSIX project is entirely based on CPS.
  • MSIX projects wants PackageReference support.
  • I'm working on enabling PackageReference support for every project system based on CPS.

@davkean davkean force-pushed the FixCPSProjects branch 2 times, most recently from 1adc686 to d404e25 Compare September 10, 2020 05:11
@davkean davkean marked this pull request as ready for review September 10, 2020 22:38
@davkean davkean requested a review from a team as a code owner September 10, 2020 22:38
@mingkuang-Chuyu
Copy link

I missed the context that probably makes this easier to understand:

  • C++ uses CPS under the covers for a variety of features.
  • C++ will be using CPS's PackageReference support under certain circumstances, that will at first limited to C++/CLI targeting .NET Core.
  • MSIX project is entirely based on CPS.
  • MSIX projects wants PackageReference support.
  • I'm working on enabling PackageReference support for every project system based on CPS.

I hope C++ Native can also support PackageReference.

I also use C#, but some scenarios require C++ Native. I have used vcpkg, but it is really troublesome to use, because we need to compile the corresponding library in vcpkg before we can use it. In NuGet I can use the code to make a source code package. When the project is compiled, it will automatically start to compile the source code in NuGet, and the project can be used directly. All of this is transparent.

@mingkuang-Chuyu
Copy link

mingkuang-Chuyu commented Sep 15, 2020

like TinyXML, this is its configuration.

This NuGet package will automatically compile TinyXML after being added to the project, and the project can use them directly. Is this experience better than the vcpkg experience? In addition, ExcludeAssets>contentfiles</ExcludeAssets> allows the package to provide header files only.

TinyXML.Static.nuspec

<?xml version="1.0" encoding="utf-8"?>
<package xmlns="http://schemas.microsoft.com/packaging/2013/01/nuspec.xsd">
  <metadata minClientVersion="2.5">
    <id>TinyXML.Static</id>
    <version>2.6.2.2</version>
    <title>TinyXML静态库</title>
    <summary>TinyXML is a simple, small, C++ XML parser that can be easily integrated into other programs.</summary>
    <authors>leethomason</authors>
    <requireLicenseAcceptance>true</requireLicenseAcceptance>
    <licenseUrl>https://sourceforge.net/directory/license:zlib/</licenseUrl>
    <projectUrl>https://sourceforge.net/projects/tinyxml/</projectUrl>
    <description>TinyXML is a simple, small, C++ XML parser that can be easily integrated into other programs.</description>
    <copyright>leethomason Copyright</copyright>
    <tags>native nativepackage</tags>
    <releaseNotes>
- Switched over to VC 2010
- Fixed up all the build issues arising from that. (Lots of latent build problems.)
- Removed the old, now unmaintained and likely not working, build files.
- Fixed some static analysis issues reported by orbitcowboy from cppcheck. 
- Bayard 95 sent in analysis from a different analyzer - fixes applied from that as well.
- Tim Kosse sent a patch fixing an infinite loop.
- Ma Anguo identified a doc issue.
- Eddie Cohen identified a missing qualifier resulting in a compilation error on some systems.
- Fixed a line ending bug. (What year is this? Can we all agree on a format for text files? Please? ...oh well.)
    </releaseNotes>
    <contentFiles>
      <files include="any\any\TinyXML.Static.txt" buildAction="Content" copyToOutput="false"/>
    </contentFiles>
  </metadata>
  <files>
    <file src=".\tinyxml\*.h" target="build\native\tinyxml"/>
    <file src=".\tinyxml\*.cpp" target="build\native\tinyxml" exclude=".\tinyxml\xmltest.cpp"/>
    <file src=".\TinyXML.Static.targets" target="build\native\TinyXML.Static.targets"/>
    <file src=".\TinyXML.Static.txt" target="contentFiles\any\any\"/>
    <file src=".\TinyXML.Static.txt" target="Content\any\any\"/>
  </files>
</package>

TinyXML.Static.targets

<?xml version="1.0" encoding="utf-8"?>
<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <PropertyGroup>
    <IncludePath>$(MSBuildThisFileDirectory)tinyxml;$(IncludePath)</IncludePath>
  </PropertyGroup>
  <Target Name="TinyXML_static_ClCompile" AfterTargets="BeforeClCompile">
    <CreateItem
      Condition="'%(Content.FileName)' == '$(MSBuildThisFileName)'"
      Include="$(MSBuildThisFileDirectory)tinyxml\*.cpp"
      AdditionalMetadata="PrecompiledHeader=NotUsing;SDLCheck=false;ConformanceMode=false;ObjectFileName=$(IntDir)$(MSBuildThisFileName)\;NuGetPackageId=$(MSBuildThisFileName);NuGetPackageVersion=%(Content.NuGetPackageVersion)">
      <Output TaskParameter="Include" ItemName="ClCompile"/>
    </CreateItem>

    <!--传统Nuget会插入到 Text 节点中,我们做个兼容处理-->
    <CreateItem
      Condition="'%(Text.FileName)' == '$(MSBuildThisFileName)'"
      Include="$(MSBuildThisFileDirectory)tinyxml\*.cpp"
      AdditionalMetadata="PrecompiledHeader=NotUsing;SDLCheck=false;ConformanceMode=false;ObjectFileName=$(IntDir)$(MSBuildThisFileName)\;NuGetPackageId=$(MSBuildThisFileName)">
      <Output TaskParameter="Include" ItemName="ClCompile"/>
    </CreateItem>
  </Target>
</Project>

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.

@NuGet/nuget-client Can I get a 2nd pair of eyes on this?

@nkolev92
Copy link
Member

@davkean there are some conflicts, can you please rebase?

@nkolev92
Copy link
Member

@mingkuang-Chuyu

As mentioned in #3145, the C++ native PackageReference support is not currently actively worked on.
These changes are specific to what Augustin mentioned in #3145 (comment)

For now, we are definitely committing to PackageReference support for C++/CLI projects targeting .NET Core, as there are cases there where developers are required to pull in NuGet packages in a workflow where packages.config no longer works at all.

@davkean davkean force-pushed the FixCPSProjects branch 2 times, most recently from 3d36986 to fc24a26 Compare September 24, 2020 08:37
@mingkuang-Chuyu
Copy link

mingkuang-Chuyu commented Sep 28, 2020

NuGet has long supported source code compilation just like vcpkg. I have never understood why Native C++ does not support PackageReference.

And on the VS platform, NuGet is 100 years ahead of vcpkg. Diligently everyone brings a better experience to our Native C++ users. I have tried many tools, NuGet, and the experience on the VS platform is the best.

If you are under-staffed, I have also studied NuGet code and functions, and I am willing to contribute my strength.

@nkolev92 @zivkan @davkean

@davkean
Copy link
Contributor Author

davkean commented Sep 28, 2020

@mingkuang-Chuyu Please file a suggestion bug via Developer Community and I will route it to the C++ team. NuGet alone cannot drive PackageReference support for all C++ projects.

This better describes what these types do.
The CPS capability indicates that the outer hierarchy of a project is owned by CPS, not that the project uses CPS. C++ will be using CPS-based components to light up PackageReference support and hence should pass this check.
We were opt'ing CPS projects out of CpsProjectReferenceProjectProvider based on whether that had TargetFramework or TargetFrameworks properties, both are which are .NET-SDK specific.

C++/MSIX deployment and other custom projects don't have these properties.
@mingkuang-Chuyu
Copy link

@mingkuang-Chuyu Please file a suggestion bug via Developer Community and I will route it to the C++ team. NuGet alone cannot drive PackageReference support for all C++ projects.

It has been in the community for a long time (at least 5 years) and there are many people discussing this topic, but they simply ignore it. I'm re-feeding it. Thank you.

@davkean
Copy link
Contributor Author

davkean commented Sep 28, 2020

The feedback needs to go to the C++ team and they aren't on this thread. I understand you are very passionate about this, but you should be commenting/upvoting a Developer Community feedback item for this be seen by the right folks.

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

Development

Successfully merging this pull request may close these issues.

4 participants