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 instead of packages.config #1242

Closed
wants to merge 17 commits into from

Conversation

wjk
Copy link
Contributor

@wjk wjk commented Aug 30, 2019

This PR attempts to do what the title says. The basic algorithm is derived from my Sunburst.VisualCpp.PackageReference NuGet package, modified to remove any need for precompiled DLLs.

I am thinking about writing a follow-up PR containing targets files that can be included in a downstream project to ease the use of WinUI. I am planning use the same technique there to resolve the NuGet package assets. However, I thought that the removal of packages.config was worth adding to the main project. I wish that the vcxproj system supported this natively, but several requests for its addition have not received a response (see microsoft/BuildXL#719 (comment) for details).

Note that I had to change the YAML CI build instructions to ensure that the project is properly restored before build; otherwise, the PackageReference targets will detect that the packages were missing and fail the build. I don't know how to properly ensure that the CI will build this PR before it is merged. I have manually verified that the Microsoft.UI.Xaml project does build after it has been restored, however.

@wjk wjk requested a review from a team as a code owner August 30, 2019 01:45
@jevansaks
Copy link
Member

I, too, wish that vcxproj supported the more modern syntax -- but I'm wary of trying to make the magic work for fear that something in the project system may get introduced down the line that breaks our custom logic. @tara-raj is vcxproj supporting PackageReference on the horizon? If so, then I wouldn't feel so bad about having a workaround in the interim.

@wjk
Copy link
Contributor Author

wjk commented Aug 30, 2019

@jevansaks If/when vcxproj adds native support for PackageReference, I honestly don't think we'll need to change anything. As I understand it, the lack of PackageReference support in C++ projects is purely a UI issue. The targets and build system already support it 100% (via the restore support for non-SDK-style csproj, which is included indirectly via Microsoft.Common.targets). The worst that I can foresee is that my custom targets will introduce what would then be a duplicate check for restored packages, on top of one that vcxproj performs itself.

@jevansaks
Copy link
Member

I am thinking about writing a follow-up PR containing targets files that can be included in a downstream project to ease the use of WinUI.

Can you elaborate on what problem you would be looking to solve? Make sure to file an issue so we can discuss what the problem/solution might be first.

As I understand it, the lack of PackageReference support in C++ projects is purely a UI issue. The targets and build system already support it 100% (via the restore support for non-SDK-style csproj, which is included indirectly via Microsoft.Common.targets).

Hmmm ... then can you help us understand what the custom targets here are doing? And why were root directory.props changes required if this is just for the dev\dll project?

@wjk
Copy link
Contributor Author

wjk commented Aug 30, 2019

Regarding the follow-up PR I mentioned, noted about filing an issue. I might not actually file one at all, as I have since discovered CommunityToolkit/Microsoft.Toolkit.Win32#147, which provides some 80% of what I would have suggested here.

As for the targets I added here, if the package references are not restored before the build begins, the build would blindly continue, not noticing that required NuGet imports are missing, and weird build failures would result. Furthermore, if the set of PackageReferences changes (or the version on one or more of them changes), unless the Restore target is manually run again, the build would blindly forge ahead with the old set of NuGet imports, and more weird failureswould result.

I put the targets in the root of the repo because I didn't know at the time that dev\dll would be the only project needing them (and because that's where I keep them in my personal projects that use this technique). Keeping them there does have a benefit, however: If we want to convert any other vcxproj files in the repo to use PackageReference, the targets will automatically be picked up in those projects. If there are no PackageReferences in a project, my added targets will do nothing (and they won't even be imported in a csproj or vbproj file).

@jevansaks
Copy link
Member

We discussed this, and we don't feel comfortable taking this change.

Perhaps if PackageReference was on the VS roadmap for vcxproj then this might be an acceptable detour. But I tried to follow up with the VS team but they don't seem to be investing in the VC project nuget support at this time, and PackageReference is not on their roadmap.

In addition it sounds like this would be a step backwards for using the Visual Studio Nuget Package Manager UI to upgrade/install packages.

We appreciate the thought and effort. Sorry!

@jevansaks jevansaks closed this Sep 13, 2019
@wjk wjk deleted the VCPackageReference branch September 13, 2019 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants