Skip to content

Windows CMake directory install fix #1434

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

Merged
merged 2 commits into from
Nov 25, 2022

Conversation

themarpe
Copy link
Contributor

Hi @yhirose @sum01

Thanks for your initial work on #470

There is a comment which seems to be the rational behind the different CMake installation directory, but I don't think it is relevant (issue: #287)

I therefore assume that this initial split in installation directory was done "from spec", not from practical issue bubbling up - but it's not (was not) correct.

Proposed are two commits.

  1. First one fixes the original behavior - it was close, but that means of searching for CMake config files was only introduced in CMake version 3.25, predating the date of original PR. See: https://cmake.org/cmake/help/latest/command/find_package.html#id3
  2. Simplification, which removes the differentiation altogether, and settles for lib/cmake/[project]/ path - I've integrated many CMake dependencies to date in various projects (one bigger being https://github.com/luxonis/depthai-core) and have never seen this installation path used by any dependency on Windows. Never had issues before.

Issue popped up during integration of the library using Hunter package manager.

@sum01
Copy link
Contributor

sum01 commented Nov 20, 2022

That distinction was born from my experience trying to get Cmake to find installed things on Windows. Admitedly it was something from a while ago, so maybe it changed by now.

Though I don't think I can really depend on Cmake v3.25 this early. I usually use slower releases like Ubuntu (v3.22) or Debian (v3.18) as a barometer. But if you say it works, I'll plan on testing it on Windows then, although I'd want it to work with the declared minimum Cmake version (v3.14 currently).

@themarpe
Copy link
Contributor Author

Though I don't think I can really depend on Cmake v3.25 this early. I usually use slower releases like Ubuntu (v3.22) or Debian (v3.18) as a barometer.

I agree, this is what the first commit addresses. The current state is cmake/[package]/ which is a 3.25 feature (as linked), the commit changing it to: cmake/ is pre 3.25, which is what i think you intended in the first place

But if you say it works, I'll plan on testing it on Windows then, although I'd want it to work with the declared minimum Cmake version (v3.14 currently).

Great, do let me know, the latter commit (simplifyed one) is the one I'm in favor of if it works as expected in various cases.

@yhirose yhirose merged commit 5758769 into yhirose:master Nov 25, 2022
@yhirose
Copy link
Owner

yhirose commented Nov 25, 2022

Thanks for the contribution!

ExclusiveOrange pushed a commit to ExclusiveOrange/cpp-httplib-exor that referenced this pull request May 2, 2023
* Fixed install convention pre 3.25

* Simplified CMAKEDIR installation directory
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.

3 participants