Skip to content

[vcpkg] Use IncludePath and LibraryPath properties #4608

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jul 5, 2020
Merged

[vcpkg] Use IncludePath and LibraryPath properties #4608

merged 6 commits into from
Jul 5, 2020

Conversation

FrankHeimes
Copy link
Contributor

These tool agnostic properties allow to configure ClCompile and ResourceCompile without repeating the code. This change includes my changes from #4454.

@FrankHeimes
Use IncludePath and LibraryPath properties
e830065 
These tool agnostic properties allow to configure ClCompile and ResourceCompile without repeating the code.
This change includes my changes from microsoft#4454.
@msftclas
Copy link

msftclas commented Oct 30, 2018 •
edited
Loading

CLA assistant check
All CLA requirements met.

@FrankHeimes
Copy link
Contributor Author

Signing the CLA apparently faces some technical difficulties: 400 - Request Header Or Cookie Too Large

@Neumann-A
Copy link
Contributor

Neumann-A commented Oct 30, 2018 •
edited
Loading

Does the change from AdditionalIncludeDirectories to IncludePath and AdditionalLibraryDirectories to LibraryPath change the search order?
Maybe add a new property which decides where to put the include and lib dirs.
I like the code reduction.

@FrankHeimes
Copy link
Contributor Author

FrankHeimes commented Oct 30, 2018 •
edited
Loading

Does the change from AdditionalIncludeDirectories to IncludePath and AdditionalLibraryDirectories to LibraryPath change the search order?

The search precedence is

  1. C/C++ / Additional Include Directories
  2. VC++ Directories / Include Directories

Since the vcpkg paths are appended to the variables, the effective search precedence changed from

  1. C/C++ / Additional Include Directories
  2. vcpkg
  3. VC++ Directories / Include Directories

to

  1. C/C++ / Additional Include Directories
  2. VC++ Directories / Include Directories
  3. vcpkg

So - should the vcpkg paths be prepended to the variables instead?
I didn't notice any malicious side effects in my own projects, though.

@ras0219-msft ras0219-msft requested a review from yuehuang010 June 20, 2019 22:56
@ras0219-msft ras0219-msft self-assigned this Jun 20, 2019
yuehuang010
yuehuang010 approved these changes Jun 22, 2019
View reviewed changes
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.

Please make sure that $(VcpkgRoot) is defined before this point. You are moving the evaluation of VcpkgRoot earlier in MSBuild evalution order.

@NancyLi1013 NancyLi1013 changed the title Use IncludePath and LibraryPath properties [vcpkg] Use IncludePath and LibraryPath properties Jan 14, 2020
@NancyLi1013
Copy link
Contributor

Could you please solve the conflicts in this PR?

@FrankHeimes
Copy link
Contributor Author

Please make sure that $(VcpkgRoot) is defined before this point. You are moving the evaluation of VcpkgRoot earlier in MSBuild evalution order.

I'm not sure if I get your point right. $(VcpkgRoot) is defined before being used (line 57). It is only used if $(VcpkgEnabled) is true.

@NancyLi1013
Copy link
Contributor

@strega-nil
Could you please help review this PR?

@strega-nil
Copy link
Contributor

@ras0219-msft @dan-shaw I don't know anything about MSBuild; could you look into this?

@yuehuang010
Copy link
Contributor

MSBuild evaluation order:

  1. Under <Project>, <PropertyGroup>
  2. Under <Project>, then <ItemDefinitionGroup>
  3. Under <Project>, then <ItemGroup>
  4. Under <Target>, in order.

Your change moved $(VcpkgRoot) from #2 to #1. There is a chance where $(VcpkgRoot) could have changed by another property group.

@FrankHeimes
Copy link
Contributor Author

FrankHeimes commented May 16, 2020 •
edited
Loading

Your change moved $(VcpkgRoot) from #2 to #1. There is a chance where $(VcpkgRoot) could have changed by another property group.

You're right. After importing vcpkg.targets, another PropertyGroup might be evaluated that changes $(VcpkgRoot) again. So vcpkg isn't supposed to modify $(IncludePath) and $(LibraryPath) at all, or is it?

@NancyLi1013
Copy link
Contributor

@FrankHeimes
Could you please remove the conflicts?

@NancyLi1013 NancyLi1013 added requires:author-response category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed labels Jun 22, 2020
@FrankHeimes
Copy link
Contributor Author

@NancyLi1013 I removed the conflict and addressed @yuehuang010's objection against modifying IncludePath and LibraryPath using the contents of $(VcpkgRoot); i.e. I reverted some of my changes.

@strega-nil
Copy link
Contributor

strega-nil commented Jul 2, 2020 •
edited
Loading

@FrankHeimes could you merge in PR FrankHeimes#2?

@strega-nil strega-nil merged commit c21893b into microsoft:master Jul 5, 2020
Jimmy-Hu added a commit to Jimmy-Hu/vcpkg that referenced this pull request Jul 5, 2020
@Jimmy-Hu
Merge pull request #246 from microsoft/master
69e89f4 
[vcpkg integrate] Clean up vcpkg.target file (microsoft#4608)
E452003 added a commit to E452003/vcpkg that referenced this pull request Jul 6, 2020
@E452003
Merge branch 'master' of https://github.com/microsoft/vcpkg
723ee5b 
* 'master' of https://github.com/microsoft/vcpkg: (1418 commits)
  [vcpkg integrate] Clean up vcpkg.target file (microsoft#4608)
  [vcpkg_from_sourceforge] Add retry mirror function (2/2) (microsoft#12018)
  [pcre2] Restore the https://ftp.pcre.org/ mirror in addition to the SourceForge mirrors. (microsoft#12233)
  [xercesc] rename feature from xmlch_wchar to xmlch-wchar (microsoft#12205)
  [safeint] Update to 3.24 (microsoft#12217)
  [vcpkg] Remove the tombstones and 'ignore' baseline concepts. (microsoft#12197)
  [msbuild] Revert the importance to Normal (microsoft#12212)
  [vtk] Added opengl feature. (microsoft#11399)
  [span-lite] Update to 0.7.0 (microsoft#12206)
  [cmocka libarchive libiconv libpq libxml2 plibsys] fix drive-by error in vcpkg-cmake-wrappers (microsoft#12196)
  [azure-iot-sdk-c] Fix feature name and enable to build (microsoft#12209)
  [vcpkg] Improve performance of compiler tracking by suppressing aspects of CMake's compiler detection. (microsoft#12203)
  [vcpkg] Remove all uses of Foo::Foo() noexcept = default; to fix microsoft#9955 (microsoft#12201)
  [sqlite3] update to 3.32.3 to deal with security issues (microsoft#12185)
  [infoware] Bump version to 0.5.4 (microsoft#12167)
  [imgui] Update to 1.77 (microsoft#12155)
  [vcpkg] Update message in bootstrap.ps1 (microsoft#12145)
  [vcpkg] Enable NuGet-based binary caching via mono (microsoft#12170)
  Don't change manifest root when manifest isn't enabled. (microsoft#12191)
  Fix sourceparagraph:BooleanField (microsoft#12192)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@yuehuang010 yuehuang010 yuehuang010 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@strega-nil strega-nil

@ras0219-msft ras0219-msft

@NancyLi1013 NancyLi1013

Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@FrankHeimes @msftclas @Neumann-A @NancyLi1013 @strega-nil @yuehuang010 @vicroms @ras0219-msft