ros / resource_retriever

Package used to retrieve resources of different kinds, e.g. http://, file://, the ROS specific package://, etc.
8 stars 42 forks source link

ninja error #80

Closed jasonbeach closed 4 months ago

jasonbeach commented 1 year ago

I'm building rviz from source (I have a docker image of Ubuntu 18.02 with ros2 galactic built from source) which apparently depends on resource_retriever, however when I add resource_retriever to my workspace and try building, I get this error

ninja: error: build.ninja:161: bad $-escape (literal $ must be written as $$) 

In the build.ninja file there are two instances of $(MAKE) which seem to be at the heart of the issue. If I replace them with make -j then all works fine. I assume these come from these two lines in libcurl_vendor CMakeLists.txt:

      BUILD_COMMAND $(MAKE)
      INSTALL_COMMAND $(MAKE) install

https://github.com/ros/resource_retriever/blob/galactic/libcurl_vendor/CMakeLists.txt#L57-L58

If I replace $(MAKE) in those two lines with a simple /usr/bin/make -j, again all works as it should. I assume these statements are meant to replace MAKE with the actual path of make, but I don't see anywhere in the CMakeLists.txt that would define MAKE. I also haven't seen the $() notation in CMake before. I've seen it in bash for command substitution, but am not sure what it does in cmake. Since Ninja is the default generator for ROS2 I would assume this would cause an issue for anyone that's trying to build this, so I'm a little surprised there's no other issues logged. Am I doing something I shouldn't be?

clalancette commented 1 year ago

Since Ninja is the default generator for ROS2

Just to be clear, Ninja is not the default generator for ROS 2. That's why likely nobody else is seeing this.

That said, we'd be happy to take fixes for this particular issue.

jasonbeach commented 1 year ago

Just to be clear, Ninja is not the default generator for ROS 2. That's why likely nobody else is seeing this.

I coulda swore....well...it really should be 😉

It looks like cmake doesn't have a find module for make like it does for git...so the only thing I can think of would be to add: find_program(MAKE_PROGRAM make REQUIRED) and then change $(MAKE) to ${MAKE_PROGRAM} -j in the external project definition. It appears that make itself defines the MAKE variable which is why the CMakeLists.txt as it is currently written works. Note that the variable name MAKE_PROGRAM is an arbitrary name I picked here and shouldn't be confused with CMAKE_MAKE_PROGRAM which is defined by cmake and is terribly named...it refers to the program of the current generator, be it make, ninja, xcode or whatever)

Would that be acceptable?

clalancette commented 1 year ago

Honestly, what we really should do is to stop using the ./configure ; make ; make install dance in libcurl_vendor, and instead switch to just using CMake to build curl. That would a) get rid of this problem completely, and b) get rid of the special-casing for Windows (since we would always just use cmake for the build).

@jasonbeach Are you willing to try that?

jasonbeach commented 1 year ago

My naive attempt at a PR: https://github.com/ros/resource_retriever/pull/81

mjcarroll commented 4 months ago

We are now using ament_cmake_vendor_package and this builds fine with Ninja for me, closing this out.