Skip to content

Conversation

@AntoninRousset
Copy link

Make ament_cmake_python compilation respect DESTDIR.

@cottsay
Copy link
Contributor

cottsay commented May 30, 2023

DESTDIR is typically not set during the compilation phase. Both Fedora and Debian packaging specify DESTDIR during make install but not the primary make invocation.

In what scenario do you feel this is needed?

@cottsay cottsay self-assigned this Jun 1, 2023
@gentoo90
Copy link

gentoo90 commented Sep 25, 2023

@cottsay, It's pythons bytecode compilation and it IS called during install. I just stepped on this landmine on gentoo when it tried to compile the bytecode directly into /opt/ros/humble instead of /var/tmp/portage/ros-humble/<PACKAGE>/image/opt/ros/humble which was denied by the sandbox.

Please merge.

@cottsay
Copy link
Contributor

cottsay commented Oct 11, 2023

How will this change behave on Windows, where CMAKE_INSTALL_PREFIX may be an absolute path which may include a drive letter?
I believe that VS-based builds currently ignore DESTDIR, but this would change that. If I'm not mistaken, DESTDIR is a Makefile-specific paradigm, not a CMake concept.

I'm not saying that we can't find a solution here, especially if the Makefiles aren't acting how they should, but I'm cautious about merging this as-is.

A question I have is how this is currently behaving in our deb and RPM builds on the buildfarm, which are currently using DESTDIR successfully.

@gentoo90
Copy link

A question I have is how this is currently behaving in our deb and RPM builds on the buildfarm, which are currently using DESTDIR successfully.

@cottsay, ros-humble-xacro_2.0.8-1jammy.20230919.194745_amd64.deb for example doesn't have *.pyc files in it, so compiling them is probably disabled on the buildfarm (e.g. ARG_SKIP_COMPILE is ON)

if(NOT ARG_SKIP_COMPILE)
get_executable_path(python_interpreter Python3::Interpreter CONFIGURE)
# compile Python files
install(CODE
"execute_process(
COMMAND
\"${python_interpreter}\" \"-m\" \"compileall\"
\"${CMAKE_INSTALL_PREFIX}/${destination}/${module_file}\"
)"
)
endif()

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.

3 participants