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

Add support for Python 3 in CMake build #10377

Merged
merged 1 commit into from
Jan 10, 2019
Merged

Add support for Python 3 in CMake build #10377

merged 1 commit into from
Jan 10, 2019

Conversation

jamiesnape
Copy link
Contributor

@jamiesnape jamiesnape commented Jan 9, 2019

EDIT (eric): Towards #8352.

This change is Reviewable

@jamiesnape
Copy link
Contributor Author

+@EricCousineau-TRI for feature review.

Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass done; if possible, would prefer to just use -DPYTHON_EXECUTABLE rather than potentially redundant / non-standard -DWITH_PYTHON_VERSION

Reviewed 5 of 5 files at r1.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee ericcousineau-tri, platform LGTM missing


CMakeLists.txt, line 162 at r1 (raw file):

  find_package(PythonInterp MODULE REQUIRED)
else()
  set(WITH_PYTHON_VERSION 2 CACHE STRING

I'm not a huge fan of adding WITH_PYTHON_VERSION...
Can we instead just rely on PYTHON_EXECUTABLE - if it's not set, just use Python 2, since that's what Bazel currently does?


CMakeLists.txt, line 163 at r1 (raw file):

else()
  set(WITH_PYTHON_VERSION 2 CACHE STRING
    "Choose the version of Python to use, options are 2 2.7 3 3.6 3.7"

nit (if this stays) Can this expand ${SUPPORTED_PYTHON_VERSIONS}?


CMakeLists.txt, line 175 at r1 (raw file):

    # Ensure that the python2 executable or link is used in preference to the
    # python and python2.7 executables or links.
    find_program(PYTHON_EXECUTABLE NAMES python2)

BTW Could this mention that 2 is only needed b/c Mac does not ship Python3?


doc/python_bindings.rst, line 68 at r1 (raw file):

*   ``-DWITH_MOSEK={ON, [OFF]}`` - Build with MOSEK enabled.
*   ``-DWITH_SNOPT={ON, [OFF]}`` - Build with SNOPT enabled.
*   ``-DWITH_PYTHON_VERSION={[2], 3}`` - Build with a specific version of Python.

Would much rather show / use -DPYTHON_EXECUTABLE here, since that's what other projects tend to use (like pybind11).

Copy link
Contributor Author

@jamiesnape jamiesnape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 unresolved discussions, LGTM missing from assignee ericcousineau-tri, platform LGTM missing


CMakeLists.txt, line 162 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

I'm not a huge fan of adding WITH_PYTHON_VERSION...
Can we instead just rely on PYTHON_EXECUTABLE - if it's not set, just use Python 2, since that's what Bazel currently does?

It's what most CMake projects do and the philosophy of CMake and Bazel are very different so it is not a good comparison. PYTHON_EXECUTABLE is really an advanced setting or fallback if the find_package fails.


CMakeLists.txt, line 163 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit (if this stays) Can this expand ${SUPPORTED_PYTHON_VERSIONS}?

SUPPORTED_PYTHON_VERSIONS is a list, so it would need transforming, but I guess so.


doc/python_bindings.rst, line 68 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Would much rather show / use -DPYTHON_EXECUTABLE here, since that's what other projects tend to use (like pybind11).

No, pybind11 has PYBIND11_PYTHON_VERSION (https://github.com/pybind/pybind11/blob/085a29436a8c472caaaf7157aa644b571079bcaa/tools/pybind11Tools.cmake#L11-L13), but that aside, pybind11 is a really bad example to follow as uses a non-standard find-module.

Copy link
Contributor Author

@jamiesnape jamiesnape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is a very un-user-friendly and very un-CMake solution.

Reviewable status: 3 unresolved discussions, LGTM missing from assignee ericcousineau-tri, platform LGTM missing

Copy link
Contributor Author

@jamiesnape jamiesnape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Also PYTHON_EXECUTABLE is changing in the new FindPython. There are Python_EXECUTABLE, Python2_EXECUTABLE, and Python3_EXECUTABLE and they behave differently.)

Reviewable status: 2 unresolved discussions, LGTM missing from assignee ericcousineau-tri, platform LGTM missing


CMakeLists.txt, line 163 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

SUPPORTED_PYTHON_VERSIONS is a list, so it would need transforming, but I guess so.

Done


CMakeLists.txt, line 175 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

BTW Could this mention that 2 is only needed b/c Mac does not ship Python3?

Done

Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: - thanks!
+@soonho-tri for platform review, please.

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all discussions resolved, platform LGTM from [ericcousineau-tri]


CMakeLists.txt, line 181 at r2 (raw file):

    # system version of Python installed on macOS. There is no system python3
    # as of macOS Mojave 10.14.
    find_program(PYTHON_EXECUTABLE NAMES python2)

BTW Per f2f, would be nice to check for redundant (and possibly mismatched configs) if user specifies both -DWITH_PYTHON_VERSION and -DPYTHON_EXECUTABLE. But that may be rare.

@EricCousineau-TRI EricCousineau-TRI mentioned this pull request Jan 10, 2019
21 tasks
Copy link
Member

@soonho-tri soonho-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, we have

[9:04:30 AM] CMake Error at CMakeLists.txt:166:
[9:04:30 AM]   Parse error.  Expected "(", got newline with text "
[9:04:30 AM] 
[9:04:30 AM]   ".
[9:04:30 AM] 
[9:04:30 AM] 

Reviewed 3 of 5 files at r1.
Reviewable status: all discussions resolved, LGTM missing from assignee soonho-tri, platform LGTM from [ericcousineau-tri]

Copy link
Contributor Author

@jamiesnape jamiesnape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Reviewable status: all discussions resolved, LGTM missing from assignee soonho-tri, platform LGTM from [ericcousineau-tri]

Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3.
Reviewable status: all discussions resolved, LGTM missing from assignee soonho-tri, platform LGTM from [ericcousineau-tri]

Copy link
Member

@soonho-tri soonho-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 5 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! all discussions resolved, platform LGTM from [ericcousineau-tri, soonho-tri]


CMakeLists.txt, line 157 at r2 (raw file):

# The supported Python major/minor versions should match those listed in both
# doc/developers.rst and tools/workspace/python/repository.bzl.
set(SUPPORTED_PYTHON_VERSIONS 2 2.7 3 3.6 3.7)

BTW, is it OK to have 3.7 here?

Copy link
Member

@soonho-tri soonho-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, platform LGTM from [ericcousineau-tri, soonho-tri]

Copy link
Contributor Author

@jamiesnape jamiesnape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, platform LGTM from [ericcousineau-tri, soonho-tri]


CMakeLists.txt, line 157 at r2 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

BTW, is it OK to have 3.7 here?

Yes, once #10378 merges.

@jamiesnape jamiesnape merged commit ec580be into RobotLocomotion:master Jan 10, 2019
@jamiesnape jamiesnape deleted the cmake-python-3 branch January 10, 2019 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants