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

DEE CMake Refresh #21099

Closed
2 tasks
svenevs opened this issue Mar 5, 2024 · 2 comments · Fixed by RobotLocomotion/drake-external-examples#295
Closed
2 tasks

DEE CMake Refresh #21099

svenevs opened this issue Mar 5, 2024 · 2 comments · Fixed by RobotLocomotion/drake-external-examples#295
Assignees
Labels
component: distribution Nightly binaries, monthly releases, docker, installation

Comments

@svenevs
Copy link
Contributor

svenevs commented Mar 5, 2024

Outcropping of RobotLocomotion/drake-external-examples#285 (python 3.12 update) so as not to block that issue. There are a number of warnings about deprecated CMake behaviours we are relying on.

The objective of DEE is to give users a template recipe to copy. The recipe should be as durable as possible for what actual users would experience. How it operates in CI is really secondary to that goal. Ideally CI would yell when users are going to be sad, but the most important thing we want is to have something that is the best idea to copy + paste.

It doesn't really matter which python version we end up with. Either 3.11 or 3.12 are supposed to work. What we probably want is to always use $(homebrew)/bin/python3 on macOS, whatever that resolves to. So anytime homebrew upgrades, the user will follow suit. We should not try to pin the minor versions, we should just let homebrew decide.

RobotLocomotion/drake-external-examples#285 (review)

The goal of this ticket should be to have somebody revisit and fix all of the CMake warnings in drake external examples, including updating to find_package(Python3). The other warnings may also relate to drake changes, I did not look closely.

  • Update python find_package usage.

     CMake Warning (dev) at CMakeLists.txt:14 (find_package):
      Policy CMP0148 is not set: The FindPythonInterp and FindPythonLibs modules
    
  • Resolve warnings about drake-config.cmake and lcm-config.cmake.

     CMake Deprecation Warning at /opt/drake/lib/cmake/drake/drake-config.cmake:9     (cmake_policy):
      Compatibility with CMake < 3.5 will be removed from a future version of
      CMake.
     CMake Deprecation Warning at /opt/drake/lib/cmake/lcm/lcm-config.cmake:9   (cmake_policy):
    

Those were the ones that came up in the macOS build (any DEE nightly trigger log will help too), there may also be linux things.

CC @jwnimmer-tri @BetsyMcPhail @mwoehlke-kitware

@svenevs svenevs added the component: distribution Nightly binaries, monthly releases, docker, installation label Mar 5, 2024
@svenevs svenevs self-assigned this Mar 5, 2024
@svenevs
Copy link
Contributor Author

svenevs commented Mar 5, 2024

It doesn't really matter which python version we end up with. Either 3.11 or 3.12 are supposed to work. What we probably want is to always use $(homebrew)/bin/python3 on macOS, whatever that resolves to. So anytime homebrew upgrades, the user will follow suit. We should not try to pin the minor versions, we should just let homebrew decide.

I agree, unless we have a minimum supported version. Beyond that, really the user is responsible for configuring whether find_package(Python3) finds python 3.12 from brew or a custom version from spack, typically via the CMAKE_PREFIX_PATH environment variable, though there are other mechanisms (see: search order numbered out near the end of this section of find_package.

This being the real discussion point:

What we probably want is to always use $(homebrew)/bin/python3 on macOS, whatever that resolves to. So anytime homebrew upgrades, the user will follow suit.

I believe that the average homebrew user would have find_package(Python3) return 3.11 on one run, but if they updated and now it was 3.12, re-configuring from a clean build directory would result in find_package(Python3) giving 3.12. I'm not sure how to test this without tinkering with parts of brew they don't really want you to. I'm of the opinion whoever implements this just checks the results of find_package and confirms that it gives the default brew one. This should be testable by also installing [email protected] on the same machine.

@jwnimmer-tri
Copy link
Collaborator

I'm not sure how to test ...

I'm perfectly satisfied with no automated test for that. The idea is that DEE should call the boring vanilla CMake function which does it boring default thing, and then the project builds and passes its small suite of test cases. Since DEE isn't itself introducing any extra complexity / branching, we don't need automated testing of different circumstances. We just assume that CMake has defaults that work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: distribution Nightly binaries, monthly releases, docker, installation
Development

Successfully merging a pull request may close this issue.

4 participants