Skip to content
This repository has been archived by the owner on Aug 30, 2020. It is now read-only.

ROS catkin support: replace the tmp catkin devel/include with the one from the workspace #106

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

fmauch
Copy link

@fmauch fmauch commented Jul 10, 2017

When developing with ROS and catkin_make files get deployed into a 'devel' folder during the build process. Apart from libraries and binaries, also auto-generated header files will be installed into this folder structure under 'devel/include'.
When doing a fake-build with YCM-Generator, this devel folder gets created in /tmp and this is then entered in the config. Up until now it is necessary to adapt this path manually after letting YCM-Generator run, which is necessary each time a new package is added to the workspace, as they bring their own new include directories.

This patch searches for the base directory by parsing the include paths for the keywords 'catkin_workspace' or 'catkin_ws'.

I am very well aware, that this patch is not yet ready to merge, for now I would only like to know whether a patch like this, which is very specific to ROS/catkin_make has a chance to get merged or not.

If you think that this is too specific to get merged, just decline this PR, then I'll try to maintain my own fork with those changes.

Cheers
Felix

@rdnetto
Copy link
Owner

rdnetto commented Jul 12, 2017

CMake is also affected by the same problem (the temp path being included in the resulting file). I'd prefer a solution that generalizes to all supported build systems, rather than just one.

In the case of cmake, there doesn't seem to be a good default to replace the build path with, since it may include files that are generated / not present in the source directory, and defaulting to the source root would also make it harder to tell which entries need to be updated.

What do you think about adding a flag that specifies a directory to replace the build path with in the config file? Then you'd be able to manually specify the workspace, and anyone using cmake could do the same thing.

@fmauch
Copy link
Author

fmauch commented Jul 12, 2017

Well, catkin_make uses cmake, so probably it's the same issue. However, the generated files are exactly, what I go for here. catkin_make takes so called message files (plain text definition files) and creates cpp-headers and python modules during the cmake run. I'm not sure how that would look with plain cmake, is this any different?

Passing this as a flag seems like a doable approach, but I'm not sure what to replace here. How and what does cmake actually add a tmp folder to the include path?

@rdnetto
Copy link
Owner

rdnetto commented Jul 13, 2017

The reason the temp path is appearing is because it's run as an out-of-tree build, to avoid polluting the project directory. The logic is equivalent to:

mkdir /tmp/tmp123
cd /tmp/tmp123
cmake $PROJECT_ROOT
make

except the compiler in PATH is a shell script that collects the compiler args. That means that if the build process generates a file, then includes it, it'll be including it from the target directory in /tmp/tmp123, which is how it makes it's way into the config file.

That temp path needs to be replaced by the path to the directory where the user is actually building their project, but since we can't detect that we need it to be provided via a flag.

@fmauch
Copy link
Author

fmauch commented Jul 13, 2017

Ah, thanks for the good explanation. So you would suggest a flag for defining the generated-include path?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants