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

Adds vcpkg item to project settings in Visual Studio #4361

Merged
merged 23 commits into from
Jun 15, 2020

Conversation

Neumann-A
Copy link
Contributor

@Neumann-A Neumann-A commented Sep 27, 2018

To solve #4359

Problem currently:
<PropertyPageSchema Include="C:\Sources\Extern\vcpkg\scripts\buildsystems\msbuild\vcpkg-general.xml">
is an absolute path (replacing with ([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), .vcpkg-root))\scripts\buildsystems\msbuild\vcpkg-general.xml did not work)

and

Property $(VcpkgRoot) is currently blank

edit: All fields are blank; I just had a *.props file added to my project to define some of the values. So we need to included those into vcpkg.system.props to define defaults.

To solve this: Changes in commands.integrate.cpp may be necessary?

@Neumann-A
Copy link
Contributor Author

Neumann-A commented Sep 27, 2018

Another nice feature would be to scan the triplet folder on integration and make the VcpkgTriplet selection an enum based on that.

Example how to do enums in the xml are given in C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\VC\VCTargets\1033\cl.xml or whereever your Visual Studio installation lies

@Neumann-A
Copy link
Contributor Author

All issues resolved? At least seems to work now. Are there more settings which should be added?
vcpkgprops

Copy link
Contributor

@yuehuang010 yuehuang010 left a comment

Choose a reason for hiding this comment

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

Overall, looks good. Please address the space/tabs to be consistant with the rest of the documents.
The $(Platform) is almost always defined and is (almost) never empty.

I only review the goal and changes to achieve the goal. I don't validate correctness and testing. Please make sure that each field in the property page works as indented.

</PropertyGroup>

<PropertyGroup>
<VcpkgTripletPrefix Condition="'$(VcpkgTripletPrefix)' == ''">$(PlatformTarget)</VcpkgTripletPrefix>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this work with older Visual Studio versions? Only checked for latest version of Visual Studio 2017. If it does not generally work I can change it back to the old conditional.

@ras0219-msft ras0219-msft self-assigned this Sep 28, 2018
@ras0219-msft
Copy link
Contributor

ras0219-msft commented Sep 28, 2018

Thanks for the PR! This is really cool!

Testing to be done before this can be merged:

  • vcpkg integrate project (with and without vcpkg integrate install on another repo)
  • vcpkg export --nuget (with and without vcpkg integrate install)
  • Multiple packages from vcpkg export --nuget (should either choose one or fail understandably)

Additionally, this will break all current users who have run integrate install when they git pull. Could the .props file be pulled in at the top of the .targets instead, so the appdata shortcut would not need to be modified?

@Neumann-A
Copy link
Contributor Author

Additionally, this will break all current users who have run integrate install when they git pull. Could the .props file be pulled in at the top of the .targets instead, so the appdata shortcut would not need to be modified?

Tried that first. The fields in the property page are empty then. The defaults can only be set in a *.props file. I also tried to import a *.props file in the available applocal *.target file but this also does not work. Since bootstrap must be run anyway to recompile vcpkg you could make a call to vcpkg integrate install mandatory if system wide user integration is detected after recompiling.

@Neumann-A
Copy link
Contributor Author

Note for people who want to play with this and change settings/properties/layout:
If you want to change anything in the vcpkg.props, vcpkg.targets or vcpkg-general.xml close Visual Studio completly after or before the changes and then reload your project. Just closing the project seems to be not enough. VS somehow remembers/caches the previous files without reloading them....(at least I was wondering more than once why my changes would not become visible or do anything.)

@yuehuang010
Copy link
Contributor

Yes, VS caches all .targets, .props, and property page xml. This is huge perf gain when loading multiple projects.
FYI, Build will always read from disk because it spawns msbuild out of proc.

@PhoebeHui
Copy link
Contributor

@Neumann-A, thanks for contributing to vcpkg! All triplets build successfully.

@JackBoosY JackBoosY added the category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed label Jun 11, 2020
@Neumann-A
Copy link
Contributor Author

@BillyONeal: looks ok. I hope you tested it?

@BillyONeal
Copy link
Member

@BillyONeal: looks ok. I hope you tested it?

I tested that vcpkg itself works and that our ports that rely on the msbuild integration to find dependencies work. I did not test the VS interaction here because I don't really understand that. (I just didn't want you to have to do work to merge this since it's been here a long time :) )

@BillyONeal
Copy link
Member

BillyONeal commented Jun 12, 2020

Remaining todos:

  • Test vcpkg integrate project without vcpkg integrate install on another repo @ras0219
  • Test vcpkg integrate project with vcpkg integrate install on another repo @ras0219
  • Test vcpkg export --nuget with vcpkg integrate install @ras0219
  • Test vcpkg export --nuget without vcpkg integrate install @ras0219
  • Test multiple packages from vcpkg export --nuget (should either choose one or fail understandably) @ras0219
  • Test that this doesn't break users who used vcpkg integrate install on git pull or fix the changes such that this works. @BillyONeal

@BillyONeal BillyONeal self-assigned this Jun 12, 2020
@BillyONeal
Copy link
Member

@Neumann-A Thanks very much for your contribution!

@BillyONeal
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BillyONeal BillyONeal merged commit 1451450 into microsoft:master Jun 15, 2020
@Neumann-A Neumann-A deleted the vcpkgsettings branch June 15, 2020 23:05
JangBoo pushed a commit to JangBoo/vcpkg that referenced this pull request Jun 18, 2020
…t#4361)

Co-authored-by: Curtis J Bezault <curtbezault@gmail.com>
Co-authored-by: Billy Robert O'Neal III <bion@microsoft.com>
Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
@aggieNick02
Copy link
Contributor

This PR ends up breaking vcpkg clients who used VcpkgTriplet in .vcxproj files to specify the desired triplet. See #12062

@BillyONeal
Copy link
Member

@strega-nil How close is your manifests PR to merging? I didn't want to address @aggieNick02 's point above yet because a fix here is going to cause another merge mess for you but if you expect that needs more than ~a day further I need to do that....

@strega-nil
Copy link
Contributor

strega-nil commented Jun 23, 2020

It's this friday close. Fix it and I'll merge mine back in.

penumbra23 pushed a commit to codespace-dev/vcpkg that referenced this pull request Aug 5, 2020
…t#4361)

Co-authored-by: Curtis J Bezault <curtbezault@gmail.com>
Co-authored-by: Billy Robert O'Neal III <bion@microsoft.com>
Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
strega-nil pushed a commit to strega-nil/vcpkg that referenced this pull request May 5, 2021
…t#4361)

Co-authored-by: Curtis J Bezault <curtbezault@gmail.com>
Co-authored-by: Billy Robert O'Neal III <bion@microsoft.com>
Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.