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

Use PackageReference for C++ dependencies #8195

Merged
merged 208 commits into from
Dec 4, 2021

Conversation

JunielKatarn
Copy link
Contributor

@JunielKatarn JunielKatarn commented Jul 8, 2021

See dotnet/project-system#2491 (comment)

MSBuild AND Visual Studio now support PackageReference dependencies for MSVC projects.
Using this will significantly reduce the project file clutter and largely simplify dependency management introducing the following enhancements:

  • Consolidate NuGet dependency declaration into the VCXPROJ file (drops packages.config.
  • Allows importing nugets directly from the GlobalPackagesFolder instead of creating local copies per repository clone, saving a fair amount of disk space (drops packages folder).
  • Aligns with the .NET restoring style (msbuild /t:restore).
    Bonus:
  • Allows to easily parameterize/interpolate package names and versions, as package references are merely MSBuild Item declarations.

Note, this change should be transparent while using Visual Studio but the MSBuild CLI and the CI restore steps will be affected.

Before:

NuGet.exe restore <solution>
MSBuild <solution>

After:

MSBuild /Restore <solution>

Pre-merge checklist

  • App and lib templates for C++ should be using PackageReference too (under discussion).
  • The CLI should be updated to deal with PackageReference.
    • Revise the logic in auto-linking which updates packages.config.
  • Make sure we bring in WinUI 2.x
    @asklar: (i see it's commented out now), and that opting into winui3 brings it in
  • Validate that apps can continue to override the WinUI 2 version by updating ExperimentalFeatures.props.
  • Validate creating new app and auto-linking works properly.
  • Review removal of RestoreLockedMode.
    Use PackageReference for C++ dependencies #8195 (comment)

Future work this PR will allow us to do after merge:

  • Leverage packages.props files
  • [ ] Remove the /p:RestorePackagesConfig=true once all packages.config files are gone.
    Useful for mixed PackageReference/packages.config projects when combined with RestoreProjectStyle=PackagesConfig.
  • Update website documentation to advertise this change to RNW users.
  • Clean up CI from redundant NuGet Restore workflows.
  • Consolidate most relevant package names and versions into shared props files.
Microsoft Reviewers: Open in CodeFlow

@JunielKatarn JunielKatarn requested review from dannyvv and asklar July 8, 2021 00:29
@JunielKatarn JunielKatarn requested a review from a team as a code owner July 8, 2021 00:29
@NickGerleman
Copy link
Collaborator

Love the overall change.

@JunielKatarn do you know the min version of VS this requires? Because end-users are buildings our source in addition to CI, there's some different versions out in the wild we need to support.

@asklar maintains some scripts and documentation for this that may need to be updated, if the min required version goes up.

This might require those manually doing a NuGet restore to change their logic, so I'm going to label this as breaking so a callout can be added to release notes?

@NickGerleman NickGerleman added the Breaking Change This PR will break existing apps and should be part of the known breaking changes for the release label Jul 8, 2021
@JunielKatarn JunielKatarn requested a review from acoates-ms July 8, 2021 00:35
@asklar
Copy link
Member

asklar commented Jul 8, 2021

Love the overall change.

@JunielKatarn Julio Cesar Rocha FTE do you know the min version of VS this requires? Because end-users are buildings our source in addition to CI, there's some different versions out in the wild we need to support.

@asklar Alexander Sklar FTE maintains some scripts and documentation for this that may need to be updated, if the min required version goes up.

This might require those manually doing a NuGet restore to change their logic, so I'm going to label this as breaking so a callout can be added to release notes?

from a functionality standpoint this should be transparent to users - if they need to run something differently, that'd be a problem. If they are using the CLI or VS, this should just work

@asklar asklar linked an issue Jul 8, 2021 that may be closed by this pull request
@JunielKatarn
Copy link
Contributor Author

Love the overall change.

@JunielKatarn do you know the min version of VS this requires? Because end-users are buildings our source in addition to CI, there's some different versions out in the wild we need to support.

@asklar maintains some scripts and documentation for this that may need to be updated, if the min required version goes up.

This might require those manually doing a NuGet restore to change their logic, so I'm going to label this as breaking so a callout can be added to release notes?

If I recall correctly, it is Visual Studio 16.9.
But MSBuild has had this capability for a while, according to @wjk (can you please confirm)?

Copy link
Collaborator

@NickGerleman NickGerleman left a comment

Choose a reason for hiding this comment

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

Chatted offline with Julio about the change. E.g. what would be good to validate.

We have manual + UT validation for autolinking looking good. As part of this change, Julio will add a document to our website to guide users on any migration they may need to do. run-windows will continue to restore NuGet packages for community modules using packages.config out of the box.

@JunielKatarn JunielKatarn merged commit bcdf9ad into microsoft:main Dec 4, 2021
NickGerleman added a commit to NickGerleman/react-native-windows that referenced this pull request Dec 18, 2021
This reverts microsoft#8195, since it looks like it added some pretty critical regressions to user scenarios.

The CLI relies on a system installed `nuget.exe` to restore, This causes the CLI to crash when the user has not separately installed `nuget.exe`. It also may call an incompatible version of `nuget.exe`, since the installed system version may not be compatible. This was not picked up in CI, since we have steps to manage the NuGet binary on the machine, for us to manually restore.

First restore is also failing locally. The error pointed to a GitHub issue around C++ PackageReference usage in VS. NuGet/Home#11392. The inability to restore on first usage is a known issue, that the VS team is not investing in, claiming they do not officially support C++ PackageReference.

On my own dev machine, this led to errrors both trying to restore via VS, or our CLI. This is a fairly serious regression, that may block work of other engineers, and prevent us from shipping the current branch.

Creating this PR to revert, unless we think we can forward-fix these issues in a short-term timeframe.
NickGerleman added a commit to NickGerleman/react-native-windows that referenced this pull request Dec 18, 2021
@JunielKatarn JunielKatarn deleted the pkgref-cpp branch January 11, 2022 02:21
@JunielKatarn JunielKatarn added the PackageReference Modern NuGet restore used in C++ projects label Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Build Infrastructure Breaking Change This PR will break existing apps and should be part of the known breaking changes for the release PackageReference Modern NuGet restore used in C++ projects
Projects
None yet
9 participants