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

CMake and ext package upgrades #1061

Merged

Conversation

michdolan
Copy link
Collaborator

@michdolan michdolan commented Jul 13, 2020

A variety of updates to ext package versions and CMake find modules:

  • Upgrade to CMake 3.12
  • Favor third-party CMake configs for finding packages
  • Adopt standard CMake _ROOT variables for overriding package locations
  • Upgrade Half version to match VFX Reference Platform CY2020
  • Upgrade other packages to take advantage of upstream CMake upgrades and configs
  • Match find module variable namespace to target name, and target names to third-party CMake configs
  • Move ext find package calls to project root to guarantee imported targets are global

See #1016 for discussion around other changes. This branch contains my commits cherry-picked from that branch. @scoopxyz will submit a follow-up PR with his changes.

Signed-off-by: Michael Dolan [email protected]

@michdolan
Copy link
Collaborator Author

NOTE: I did some refactoring of the OpenGL and headless rendering messaging section of the main CMakeLists.txt file. It will now not check or warn about headless rendering unless GPU rendering can be enabled. Worth a sanity check from @hodoulp and @ChinYing-Li . Thanks!

@michdolan
Copy link
Collaborator Author

Closing and reopening to re-trigger CI

@michdolan michdolan closed this Jul 13, 2020
@michdolan michdolan reopened this Jul 13, 2020
@michdolan michdolan closed this Jul 13, 2020
@michdolan michdolan reopened this Jul 13, 2020
@hodoulp
Copy link
Member

hodoulp commented Jul 13, 2020

@michdolan Without the fixes from @scoopxyz any builds with python 2.x fail!

@michdolan
Copy link
Collaborator Author

Our CI builds and tests Python 2 and 3 and everything built and passed Python tests. Where are you seeing a build failure?

The intent is for Sean to submit a follow-up PR, which in a worst case scenario could be merged immediately after this PR. We had a lot of issues around DCO failing when collaborating on the previous PR.

@hodoulp
Copy link
Member

hodoulp commented Jul 13, 2020

Here is my result (macOS, python 2.7.16):

git fetch origin pull/1061/head:pr-1061
git checkout pr-1061
rm -rf *
cmake ../.
-- Found PythonInterp: /usr/bin/python (found version "2.7.16") 
-- Found PythonLibs: /System/Library/Frameworks/Python.framework/Versions/2.7/lib/libpython2.7.dylib
-- Found pybind11: /usr/local/include (found suitable version "2.5.0", minimum required is "2.4.3") 

-- Found PythonInterp: /usr/bin/python (found suitable version "2.7.16", minimum required is "2.7") 
CMake Error at /usr/local/Cellar/cmake/3.16.5/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:146 (message):
  Could NOT find PythonLibs (missing: PYTHON_INCLUDE_DIRS) (Required is at
  least version "2.7")
Call Stack (most recent call first):
  /usr/local/Cellar/cmake/3.16.5/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:393 (_FPHSA_FAILURE_MESSAGE)
  /usr/local/Cellar/cmake/3.16.5/share/cmake/Modules/FindPythonLibs.cmake:310 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  src/bindings/python/CMakeLists.txt:8 (find_package)


-- Configuring incomplete, errors occurred!

@michdolan
Copy link
Collaborator Author

try adding to cmake call:
-DPYTHON_EXECUTABLE=$(which python)

@michdolan
Copy link
Collaborator Author

We can try to merge @scoopxyz commits into this PR again if needed. It should be clean now since the branch is new. It gets complicated when needing to rebase to amend commit messages when there are multiple contributors.

@scoopxyz
Copy link
Contributor

Yeah, quickly finding how nasty it gets if we don't validate DCO before creating any PRs... maybe there is a post-commit hook we could enforce it with?

@hodoulp
Copy link
Member

hodoulp commented Jul 13, 2020

try adding to cmake call:
-DPYTHON_EXECUTABLE=$(which python)

It does not work.

Yeah, quickly finding how nasty it gets if we don't validate DCO before creating any PRs...

Something to talk at the TSC. Many contributors (first contribution) get stuck with their pull request because of that 'double' validation i.e. always one missing.

@michdolan
Copy link
Collaborator Author

@scoopxyz if you make a PR against this new branch we can give it a whirl again.

@zachlewis
Copy link
Collaborator

@hodoulp try additionally setting 'PYTHON_INCLUDE_DIRS', e.g:
-DPYTHON_INCLUDE_DIRS=$(python -c "from distutils import sysconfig; print(sysconfig.get_python_inc())"

@michdolan michdolan merged commit 346b11c into AcademySoftwareFoundation:master Jul 20, 2020
@michdolan michdolan deleted the cmake_3_12_upgrade branch October 21, 2020 12:42
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.

5 participants