search

colcon / colcon-ros

Extension for colcon to support ROS packages
http://colcon.readthedocs.io
Apache License 2.0
13 stars 26 forks source link

Tests target is built by default #114

Closed mikepurvis closed 3 years ago

mikepurvis commented 3 years ago

Slightly related to #110, in my opinion this is wrong behaviour:

https://github.com/colcon/colcon-ros/blob/10130852e577dfaa0855e035d338f27689166fc7/colcon_ros/task/catkin/build.py#L71-L74

None of catkin_make, catkin_make_isolated, or catkin_tools has defaulted to building test binaries. It's very surprising as a user to run colcon build and have it be building targets other than what is expressed in the CMakeLists.txt as being part of ALL.

I'm very much in support of having "run tests" be a first-class action (it never was in catkin_tools, regrettably), but I feel that building the tests should be something done just ahead of running them, not as something which must always be explicitly disabled using a flag.

dirk-thomas commented 3 years ago

While the behavior is different to previous ROS 1 tools it is consistent across all package types in colcon which I would consider more important.

One advantage is that after calling build you can be sure that all code built - even if you never explicitly built the tests.

Also e.g. for CMake packages you can choose to not configure tests in the first place if you are not interested in them (and prefer to have a quicker build time).

mikepurvis commented 3 years ago

I don't think it's just a matter of diverging from ROS 1 tools— it's a divergence from most other build systems I've encountered, where the building of tests is always moved to a separate target which much be explicitly invoked.

Couldn't the desired consistency be achieved via a separate colcon test verb, which has a dedicated implementation for all package types, and maybe flags for --build-only/--run-only modes. Is there something under the hood which requires the two build parts to be connected like this?

dirk-thomas commented 3 years ago

Is there something under the hood which requires the two build parts to be connected like this?

I could ask the opposite: is there any reason they need to be separated?

Currently the semantic is very clean: build builds packages and test runs the tests of packages. And you do have the options to split the operations if desired:

it's a divergence from most other build systems I've encountered, where the building of tests is always moved to a separate target which much be explicitly invoked.

How would you do that in ROS 2? ament_cmake is just doing what CMake is doing - nothing special - none of the overhead from catkin to setup a whole hierarchy of test targets and hidden targets (prefixed with an underscore). I am not aware of a way in CMake to configure tests but skip them from building (if the CMake code doesn't explicitly mark the targets as exclude from all). And even then there is no way to explicitly build all test targets which are not part of the all target.

mikepurvis commented 3 years ago

Okay, hmm— so the argument is basically that conventional CMake (and therefore ament) calls for any test binaries to be built as part of ALL, with no way to exclude these, and therefore auto-building them for the catkin case is an exercise in conformity between the two? Certainly when it comes to other public CMake projects, it ends up being custom CMake args, and a mishmash of flags with defaults which may go either way, eg:

https://github.com/opencv/opencv/blob/8da1b9aafa33899559f75a3de7de1b7112cfe7a9/CMakeLists.txt#L450-L454

Now that said, there are potentially CMake features which could be leveraged here to mark targets as being tests without needing to hitch them all onto a master tests target— thinking particularly of the LABELS property, which it seems is used by CTest for pretty must this exact purpose. I wonder if it's worth reaching out to Kitware to understand if they would recommend a particular way of approaching this?

Anyway, feel free to leave this ticket open or close it; either way, it can be a record of why this is the way it is, for others to find. Our exploratory ROS 2 team is still very small, but when I brought this business up, one of my colleagues mentioned that they had already perma-enabled the flag via a shell alias months ago, which seems like a bad sign. :)

dirk-thomas commented 3 years ago

so the argument is basically that conventional CMake (and therefore ament) calls for any test binaries to be built as part of ALL, with no way to exclude these, and therefore auto-building them for the catkin case is an exercise in conformity between the two?

Yes, perfect summary.

I wonder if it's worth reaching out to Kitware to understand if they would recommend a particular way of approaching this?

Sounds reasonable. Please share the response here for future readers.

feel free to leave this ticket open or close it

I will go ahead and close then if there is currently no derived action item from it. Please feel free to continue commenting.

that they had already perma-enabled the flag via a shell alias

The defaults.yaml file seems to be the proper place for such a configuration (see https://colcon.readthedocs.io/en/released/user/configuration.html#defaults-yaml).