search

ament / uncrustify_vendor

CMake shim over the uncrustify library: https://github.com/uncrustify/uncrustify
Apache License 2.0
0 stars 9 forks source link

Use explicit INSTALL_COMMAND to suppress DESTDIR #21

Closed cottsay closed 3 years ago

cottsay commented 3 years ago

The DESTDIR environment variable is used on UNIX systems during an invocation of 'make install' to specify an alternate directory prefix for installation paths. Since this isn't a cross-platform mechanism, the CMAKE_INSTALL_PREFIX CMake variable can also be used.

Unfortunately, if both of these are specified, they will both be applied. This isn't typically a problem, but can cause problems for the installation phase of ExternalProject_Add if the external project is built during an invocation of 'make install' with DESTDIR set.

Since the CMAKE_INSTALL_PREFIX method is employed by the call to ExternalProject_Add instead of DESTDIR, we should specifically suppress DESTDIR if it is set so that it doesn't interfere with the installation of the external project to the staging directory.

In this package, there is something subtle happening related to using git as an upstream source combined with a patching step that causes the external project to be re-built during the install phase. This is triggering the behavior described above. Each issue alone does not result in the misbehavior, but when both occur, the result is that the install phase of the external project installs the project to the wrong location - $ENV{DESTDIR}/${CMAKE_INSTALL_PREFIX} - instead of just ${CMAKE_INSTALL_PREFIX}, which corresponds to the staging directory.

nuclearsandwich commented 3 years ago

As far as I can tell from reading the only Bad Stuff:tm: that results from this change is that customizing a ROS 2 build using DESTDIR alone wouldn't affect the vendor packages since the variable would be unset during installation.

Is that actually the case? Are there other caveats to this approach? It seems like the best solution given that it doesn't interfere with the usual workflow.

cottsay commented 3 years ago

So we never want DESTDIR to be set during the installation of an external project to a staging directory if we're already using CMAKE_INSTALL_PREFIX. The installation phase of the vendor package that takes the stuff from the staging directory and installs it to the actual target installation directory will continue to use DESTDIR and/or CMAKE_INSTALL_PREFIX normally.

The problem isn't the use of DESTDIR in and of itself, it's that we currently allow that behavior to extend into the external project where we aren't expecting it to be used.

I'm not aware of any circumstances where we'd want the current behavior to continue.

Actually, if the external project is never built without a DESTDIR value, then the bug will always result in a build failure because the staging directory will never be populated by the external project's install phase.

I'm going to open an issue to discuss how colcon should handle an externally supplied DESTDIR environment variable, so this might not work in the future, but you can reproduce this right now:

$ rm build/uncrustify_vendor/ -rf
$ DESTDIR=/tmp/foobar colcon build --merge --install-base /opt/ros/rolling --packages-select uncrustify_vendor
Starting >>> uncrustify_vendor
--- stderr: uncrustify_vendor                                
Cloning into 'uncrustify-0.68.1'...
HEAD is now at 45b836cf Merge pull request #2308 from emersonknapp/md5-armhf
CMake Error at cmake_install.cmake:54 (file):
  file INSTALL cannot find
  "/opt/ros_src/rolling/build/uncrustify_vendor/uncrustify_vendor_install":
  No such file or directory.

---
Failed   <<< uncrustify_vendor [12.9s, exited with code 1]

Summary: 0 packages finished [13.9s]
  1 package failed: uncrustify_vendor
  1 package had stderr output: uncrustify_vendor
$ find /tmp/foobar
/tmp/foobar
/tmp/foobar/opt
/tmp/foobar/opt/ros_src
/tmp/foobar/opt/ros_src/rolling
/tmp/foobar/opt/ros_src/rolling/build
/tmp/foobar/opt/ros_src/rolling/build/uncrustify_vendor
/tmp/foobar/opt/ros_src/rolling/build/uncrustify_vendor/uncrustify_vendor_install
/tmp/foobar/opt/ros_src/rolling/build/uncrustify_vendor/uncrustify_vendor_install/bin
/tmp/foobar/opt/ros_src/rolling/build/uncrustify_vendor/uncrustify_vendor_install/bin/uncrustify

In this example, the staging directory should be /opt/ros_src/rolling/build/uncrustify_vendor/uncrustify_vendor_install. That's where the vendor package is expecting the external package to install to, but DESTDIR caused it to prefix the install paths with /tmp/foobar. Since the staging directory should be a build directory, it certainly shouldn't end up anywhere in the final install location, so making CMake anticipate the external project's use of DESTDIR isn't the right thing to do either. The best option is to suppress the behavior.

cottsay commented 3 years ago

Here's that colcon issue I promised: colcon/colcon-cmake#103