Skip to content

Adds vcpkg item to project settings in Visual Studio#4361

Merged
BillyONeal merged 23 commits intomicrosoft:masterfrom
Neumann-A:vcpkgsettings
Jun 15, 2020
Merged

Adds vcpkg item to project settings in Visual Studio#4361
BillyONeal merged 23 commits intomicrosoft:masterfrom
Neumann-A:vcpkgsettings

Conversation

@Neumann-A
Copy link
Copy Markdown
Contributor

@Neumann-A Neumann-A commented Sep 27, 2018 •
edited
Loading

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
Copy Markdown
Contributor Author

Neumann-A commented Sep 27, 2018 •
edited
Loading

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
Copy Markdown
Contributor Author

Neumann-A commented Sep 27, 2018

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

Neumann-A
Neumann-A commented Sep 27, 2018
View reviewed changes
yuehuang010
yuehuang010 reviewed Sep 27, 2018
View reviewed changes
yuehuang010
yuehuang010 reviewed Sep 27, 2018
View reviewed changes
yuehuang010
yuehuang010 reviewed Sep 27, 2018
View reviewed changes
yuehuang010
yuehuang010 reviewed Sep 27, 2018
View reviewed changes
yuehuang010
yuehuang010 reviewed Sep 27, 2018
View reviewed changes
yuehuang010
yuehuang010 reviewed Sep 27, 2018
View reviewed changes
yuehuang010
yuehuang010 approved these changes Sep 27, 2018
View reviewed changes
Copy link
Copy Markdown
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.

Neumann-A
Neumann-A commented Sep 28, 2018
View reviewed changes
scripts/buildsystems/msbuild/vcpkg.props Outdated
</PropertyGroup>

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

@Neumann-A Neumann-A Sep 28, 2018

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
Copy Markdown
Contributor

ras0219-msft commented Sep 28, 2018 •
edited
Loading

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
Copy Markdown
Contributor Author

Neumann-A commented Sep 28, 2018

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
Copy Markdown
Contributor Author

Neumann-A commented Sep 28, 2018

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
Copy Markdown
Contributor

yuehuang010 commented Sep 28, 2018

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.

@Neumann-A Neumann-A mentioned this pull request Oct 12, 2018
Closed
@degski degski mentioned this pull request Nov 12, 2018
Closed
@Neumann-A Neumann-A mentioned this pull request Mar 1, 2019
Open
@PhoebeHui
Copy link
Copy Markdown
Contributor

PhoebeHui commented Mar 8, 2019

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

@Neumann-A Neumann-A mentioned this pull request Apr 24, 2019
Closed
@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
Copy Markdown
Contributor Author

Neumann-A commented Jun 11, 2020

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

@BillyONeal
Copy link
Copy Markdown
Member

BillyONeal commented Jun 11, 2020

@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
Copy Markdown
Member

BillyONeal commented Jun 12, 2020 •
edited
Loading

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
BillyONeal reviewed Jun 12, 2020
View reviewed changes
@BillyONeal
Copy link
Copy Markdown
Member

BillyONeal commented Jun 15, 2020

@Neumann-A Thanks very much for your contribution!

@BillyONeal
Copy link
Copy Markdown
Member

BillyONeal commented Jun 15, 2020

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Jun 15, 2020

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
@Neumann-A @cbezault
@BillyONeal @ras0219-msft @JangBoo
[vcpkg] Add vcpkg item to project settings in Visual Studio (microsof…
256f096 
…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
Copy Markdown
Contributor

aggieNick02 commented Jun 22, 2020

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

@BillyONeal
Copy link
Copy Markdown
Member

BillyONeal commented Jun 22, 2020

@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
Copy Markdown
Contributor

strega-nil commented Jun 23, 2020 •
edited
Loading

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

@gix gix mentioned this pull request Jun 30, 2020
Closed
@gix gix mentioned this pull request Aug 3, 2020
Closed
penumbra23 pushed a commit to codespace-dev/vcpkg that referenced this pull request Aug 5, 2020
@Neumann-A @cbezault
@BillyONeal @ras0219-msft @penumbra23
[vcpkg] Add vcpkg item to project settings in Visual Studio (microsof…
92c597d 
…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
@Neumann-A @cbezault
@BillyONeal @ras0219-msft
[vcpkg] Add vcpkg item to project settings in Visual Studio (microsof…
f1c0292 
…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>
@Neumann-A Neumann-A mentioned this pull request Aug 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BillyONeal BillyONeal BillyONeal approved these changes

@ras0219-msft ras0219-msft Awaiting requested review from ras0219-msft

@dan-shaw dan-shaw Awaiting requested review from dan-shaw

+3 more reviewers

@ras0219 ras0219 ras0219 approved these changes

@yuehuang010 yuehuang010 yuehuang010 approved these changes

@JackBoosY JackBoosY JackBoosY approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@BillyONeal BillyONeal

@ras0219-msft ras0219-msft

@PhoebeHui PhoebeHui

@JackBoosY JackBoosY

Labels

category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.