Skip to content

[vcpkg] Use IncludePath and LibraryPath properties#4608

Merged
strega-nil merged 6 commits intomicrosoft:masterfrom
FrankHeimes:master
Jul 5, 2020
Merged

[vcpkg] Use IncludePath and LibraryPath properties#4608
strega-nil merged 6 commits intomicrosoft:masterfrom
FrankHeimes:master

Conversation

@FrankHeimes
Copy link
Copy Markdown
Contributor

@FrankHeimes FrankHeimes commented Oct 30, 2018

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

msftclas commented Oct 30, 2018 •
edited
Loading

CLA assistant check
All CLA requirements met.

@FrankHeimes
Copy link
Copy Markdown
Contributor Author

FrankHeimes commented Oct 30, 2018

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

@Neumann-A
Copy link
Copy Markdown
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
Copy Markdown
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
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.

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

NancyLi1013 commented Jan 14, 2020

Could you please solve the conflicts in this PR?

@FrankHeimes
Copy link
Copy Markdown
Contributor Author

FrankHeimes commented Jan 14, 2020

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

NancyLi1013 commented May 14, 2020

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

@strega-nil
Copy link
Copy Markdown
Contributor

strega-nil commented May 15, 2020

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

@yuehuang010
Copy link
Copy Markdown
Contributor

yuehuang010 commented May 15, 2020

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

NancyLi1013 commented Jun 22, 2020

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

FrankHeimes commented Jun 22, 2020

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