search

colcon / colcon-cmake

Extension for colcon to support CMake packages
http://colcon.readthedocs.io
Apache License 2.0
16 stars 25 forks source link

fix: Fixed has target check taking ages for large projects #138

Open jmachowinski opened 3 months ago

jmachowinski commented 3 months ago

The old implementation took up to 30 seconds for packages, were a lot of targets existed. This is a common case for interface packages containing a lot of messages.

wjwwood commented 3 months ago

I tested this on macos and I didn't notice or see any issues building the ros2.repos up to rclcpp and a little past that.

jmachowinski commented 2 months ago

I had some issues with this patch, it does not seem to work in special situations. I currently don't have time to look into it, therefore I will convert this PR into a draft.

cottsay commented 2 months ago

What sort of behavior does this exhibit if the CMake generator is Visual Studio? Also, what if the system's language is not set to English - is make's output localized?

jmachowinski commented 2 months ago

What sort of behavior does this exhibit if the CMake generator is Visual Studio?

Unchanged

Also, what if the system's language is not set to English - is make's output localized?

AFAIK make is not localized, at least it is not on my german localized ubuntu

jmachowinski commented 3 weeks ago

After reading https://stackoverflow.com/questions/23849953/how-to-check-existence-of-the-target-in-a-makefile, I'm wondering if we shouldn't also use --dry-run and maybe use the strategy suggested there where we check the output of the data base with --print-data-base? I'm not certain it provides more benefits, but maybe it would help to avoid unintended (and in some cases expensive time-wise) side effects.

--dry-run generated a lot of not needed output, which is not done by the question mode. As I don't check on the return code, but parse the std_err instead, we should be fine

jmachowinski commented 3 weeks ago

This version is working, and tested. Additional tests would be appreciated though, as I only have a linux machine.

cottsay commented 3 weeks ago

Beyond the linting errors that CI identified, I am also concerned that this change is coupled to GNU Make. The -q flag has a different (and incompatible) meaning on BSD: https://man.freebsd.org/cgi/man.cgi?make(1)

That said, BSD isn't an explicitly supported platform, but it still doesn't feel good to break it just for an optimization. I'll do a little more poking around to see if I can see another solution.