generate all ament index markers into <build>/ament_index_preinstall

[wjwwood]

The benefit to this approach is that the location can be used before install for tests which need the ament_index to be in place.

The "preinstall prefix" is exposed through a new cmake function.

Connects to ros2/rclpy#13

[dirk-thomas]

Why did you change this? This is not how the API is supposed to be used. See

ament_cmake/ament_cmake_core/cmake/index/ament_index_register_resource.cmake

Lines 19 to 33 in ebeb599

	# Register a package resource of a specific type with the index.
	#
	# :param resource_type: the type of the resource
	# :type resource_type: string
	# :param CONTENT: the content of the marker file being installed
	# as a result of the registration (default: empty string)
	# :type CONTENT: string
	# :param CONTENT_FILE: the path to a file which will be used to fill
	# the marker file being installed as a result of the registration.
	# The file can either be a plain file or a template (ending with
	# '.in') which is expanded using configure_file() with @ONLY.
	# (optional, conflicts with CONTENT)
	# :type CONTENT_FILE: string
	# :param PACKAGE_NAME: the package name (default: ${PROJECT_NAME})
	# :type PACKAGE_NAME: string

[wjwwood]

A few lines above it errors out if there is anything in ARGN. I don't see how this could ever pass something along. You're right that it won't work as is though. Should I just add the PACKAGE_NAME keyword argument or should I remove the check about ARGN?

[dirk-thomas]

The check above is wrong. The CMake function currently doesn't allow to pass a package name. Instead the check should test for "unused arguments".

The call to ament_index_register_resource can either just pass ${ARGN} as before or explicitly "PACKAGE_NAME" "${ARG_PACKAGE_NAME}".

[dirk-thomas]

I don't think extending the ament index into the build space ("preinstalled") is a good way. I think sticking to the single responsibility principle here is better.

Since all this information is only available within one CMake context of a single package the information can easily be passed within CMake without the need of the file system.

[wjwwood]

I don't think extending the ament index into the build space ("preinstalled") is a good way. I think sticking to the single responsibility principle here is better.

Since all this information is only available within one CMake context of a single package the information can easily be passed within CMake without the need of the file system.

That makes testing something that uses the ament index impossible (you instead have to have a second, side channel for relaying the information). If you want to veto this approach, please look at ros2/rclpy#13 and suggest an alternative. I'll implement it if it is a better idea, but I cannot think of a better way to do it.

I don't see how the "single responsibility" principle applies here. Can you explain why you think that is the case?

[dirk-thomas]

Maybe "single responsibility" wasn't a good term. The problem I see is that the concept of having an index in the build space e.g. doesn't apply to Python. Also the fact that the current ament index files are being generated into the build space is an implementation detail. They could very well be generated during the install step. Also none of the other CMake API is currently returning results including the files from the build space. But I do see the use case for this.

The first thing to clarify is that this will be a CMake-only thing. E.g. Python won't have that kind of use case.

If we then guarantee that the resources are actually created at configure (or build ?) time then I would expect that all CMake API returns information including that source (actually all functions should take an optional prefix path which has a reasonable default). Then CMake code can use the index the same way as the proposed Python test. Otherwise it looks kind of inconsistent to me.

I can work on a pull request for that if we have a consensus on this.

[wjwwood]

Sounds good.

[dirk-thomas]

I have created the following branch with the patch: https://github.com/ament/ament_cmake/tree/import_rmw_that_have_python_only2

If it looks good you could move the changes over to this branch in order to test it together with the rclpy patch.

[wjwwood]

I merged your branch in (or reset to your branch) and fixed a small thing. I had to make some changes in rclpy, but they've been pushed as well. I think they are both ready for review. I'll start CI and post on the other PR.

[dirk-thomas]

The custom prefix path parameter has been added in ab1397b

[wjwwood]

Ok I now have ros2/rclpy@0f1c31e and 6c84658 that work for me locally. I'm going to kick off a Windows CI to test those out as the short term solution.

[wjwwood]

It fails still, I think because CMake is still somehow converting it into a list at some point:

http://ci.ros2.org/job/ci_windows/1224/testReport/(root)/projectroot/rclpytests/

I'll have to spin up a build on my Windows VM to test and iterate locally.

[wjwwood]

I'm still unable to use multi item "PATH"'s when passing arguments. So I've just fallen back to sending one item until we can figure out a good way to do that.

[wjwwood]

I created #66 to track the issue of not being able to pass multiple elements.