drake icon indicating copy to clipboard operation
drake copied to clipboard

DEE CMake Refresh

Open svenevs opened this issue 1 year ago • 2 comments

Outcropping of https://github.com/RobotLocomotion/drake-external-examples/pull/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.

https://github.com/RobotLocomotion/drake-external-examples/pull/285#pullrequestreview-1912327105

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 avatar Mar 05 '24 23:03 svenevs

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.

svenevs avatar Mar 05 '24 23:03 svenevs

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.

jwnimmer-tri avatar Mar 05 '24 23:03 jwnimmer-tri