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

find_package macro redefinition conflicts with vcpkg macro on Windows. #648

Closed
ftpronk opened this issue Jan 30, 2020 · 8 comments
Closed
Labels
Needs Discussion To be discussed in the technical steering committee
Milestone

Comments

@ftpronk
Copy link

ftpronk commented Jan 30, 2020

Trying to configure openexr on Windows fails when using dependencies managed by vcpkg.

Command:

cd .\build
cmake -DCMAKE_TOOLCHAIN_FILE=C:\vcpkg\scripts\buildsystems\vcpkg.cmake -DVCPKG_TARGET_TRIPLET=x64-windows-static ..

Result:

-- Configure ILMBASE Version: 2.4.1 Lib API: 24.0.0
CMake Error at C:/vcpkg/scripts/buildsystems/vcpkg.cmake:237 (string):
  Maximum recursion depth of 1000 exceeded
Call Stack (most recent call first):
  C:/vcpkg/scripts/buildsystems/vcpkg.cmake:286 (_find_package)
  C:/vcpkg/scripts/buildsystems/vcpkg.cmake:286 (_find_package)

[...]

  C:/vcpkg/scripts/buildsystems/vcpkg.cmake:286 (_find_package)
  C:/vcpkg/scripts/buildsystems/vcpkg.cmake:286 (_find_package)
  CMakeLists.txt:33 (_find_package)
  IlmBase/config/IlmBaseSetup.cmake:136 (find_package)
  IlmBase/CMakeLists.txt:35 (include)
-- Configuring incomplete, errors occurred!

Vcpkg version:

Vcpkg package management program version 2020.01.17-nohash-external

Openexr commit: 1cbf6b9

@meshula
Copy link
Contributor

meshula commented Jan 30, 2020

It would probably be a good idea to post an issue on the vcpkg github site. It looks like they have their own methods for detecting OpenEXR and patching the files under various circumstances. They probably have not yet got support for OpenEXR 2.4.x.

@ftpronk
Copy link
Author

ftpronk commented Jan 31, 2020

I'm no using vcpkg for OpenEXR. I'm compiling OpenEXR from source, pulled from github. Commit number: 1cbf6b9. I'm only using vcpkg for the dependencies: zlib and boost.

@meshula
Copy link
Contributor

meshula commented Jan 31, 2020

I see. I understand that you are building OpenEXR from source from github. OpenEXR first builds IlmBase, and then it builds OpenEXR (IlmImf). It's possible that the vcpkg toolchain file doesn't affect things at all, but I do see this code:

https://github.com/microsoft/vcpkg/blob/master/ports/openexr/vcpkg-cmake-wrapper.cmake

which makes me feel like there might be something complicated going on, since that scripts seems like it might be modifying cmake behavior to use vcpkg's _find_package instead of the cmake find_package.

Your initial message lists this -

Maximum recursion depth of 1000 exceeded
Call Stack (most recent call first):
  C:/vcpkg/scripts/buildsystems/vcpkg.cmake:286 (_find_package)

which says that the recursion is occurring in the _find_package, in vcpkg, not in find_package.

This happens in the OpenEXR cmake script when it is trying to find IlmBase.

 IlmBase/config/IlmBaseSetup.cmake:136 (find_package)

I'm afraid I'm not familiar enough with how vcpkg works to comment further. It's hard for me to tell if their toolchain file is compatible with OpenEXR 2.4 at all. They definitely mention compatibility with OpenEXR 2.3 in their code -

https://github.com/microsoft/vcpkg/tree/master/ports/openexr

That's why I was thinking it might be an idea to ask for help from the vcpkg team. If there's some incompatibility with our cmake scripts, it would be great to know what it is. I can see in their sources that they have to patch our 2.3 scripts for compatibility, it would be much better to know if it's possible to modify things here, in this repo, to completely eliminate the need for patches at their end.

If you were to open an issue there, and mention it here, maybe a fix can be created in cooperation with the vcpkg authors.

Hope this helps!

@ftpronk
Copy link
Author

ftpronk commented Feb 3, 2020

Thank you for the update! My take on it with my limited understanding of CMake, was that there might be some conflict between the vcpkg redefinition of find_package, and the redefinition of OpenEXR 2.4 defined in CMakeLists.txt:

set(as_subproject IlmBase OpenEXR)
macro(find_package)
  if(NOT "${ARGV0}" IN_LIST as_subproject)
    _find_package(${ARGV})
  endif()
endmacro()

If I'm not mistaken, the underscore _ in front of find_package is an undocumented CMake behaviour trying to call the previously defined command with the same name. Is there maybe a limit in how many versions of a command CMake can track? And that would lead to the wrong find_package being called, and an infinite recursion? 🤔

I'll try to open an issue with vcpkg as you suggested.

Edit:
Found here:
https://crascit.com/2018/09/14/do-not-redefine-cmake-commands/

When function() or macro() is called to define a new command, if a command already exists with that name, the undocumented CMake behavior is to make the old command available using the same name except with an underscore prepended. This applies whether the old name is for a built-in command, a custom function or a macro. If a command is only ever overridden once, techniques like in the example above appear to work, but if the command is overridden again, then the original command is no longer accessible. The prepending of one underscore to “save” the previous command only applies to the current name, it is not applied recursively to all previous overrides. This has the potential to lead to infinite recursion, as the following contrived example demonstrates

@meshula
Copy link
Contributor

meshula commented Feb 3, 2020

Good find ~~~~ I did not know about that!! What a can of worms!!

@kdt3rd This could come up anywhere another project redefines findpackage. I wonder if there might be another way to signal being a subproject?

@cary-ilm
Copy link
Member

We discussed this in the steering committee. This is almost certainly a result of the complication of the top-level "super-build" setup that builds the submodules IlmBase, OpenEXR, PyIlmBase, OpenEXR_Viewers. Our intention with the 3.0 release later in the year is to split these projects into separate github repos, creating proper dependencies between them, which will mean an entire re-vamp of the cmake setup. At this point, this need for a special find_package will change drastically or go away entirely. We'd like to minimize any investment in maintaining the existing setup.

@cary-ilm
Copy link
Member

We discussed this again in the steering committee. There doesn't seem to be a simple solution, and as mentioned above, this is slated to be replaced entirely in the 3.0 release. Closing the issue for now.

@ftpronk
Copy link
Author

ftpronk commented Feb 28, 2020

I'll just add a link to the issue I opened on the vcpkg side: microsoft/vcpkg#9890

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Discussion To be discussed in the technical steering committee
Projects
None yet
Development

No branches or pull requests

3 participants