Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for symlinked directories in private devel space #377

Merged
merged 1 commit into from
Jan 31, 2017

Conversation

dominiquehunziker
Copy link
Contributor

Build / Run Issue

Symlinks in the package devel space linking to directories are not properly forwarded in the shared linked devel space layout, i.e., an empty directory is created instead of a symlink.

Use case: I symlink a data directory to the devel space like this to make use of package data for Python packages in the devel space here.

Sample workspace: https://github.com/dominiquehunziker/catkin_tools_test_ws

. /opt/ros/indigo/setup.sh
catkin clean -y
catkin config --link-devel
catkin build
. devel/setup.bash
rosrun py_dummy_pkg runner.py

Suggested fix

Basically, the code/logic for files is reused (maybe this should be refactored?). However, I'm uncertain if the check for collisions is sufficient and if they should be handled differently from the files.

@remod
Copy link

remod commented Jun 28, 2016

I have the same issue. An easy fix which works for me is to replace

os.walk(source_devel_path)

with

os.walk(source_devel_path, followlinks=True)

Instead of cloning the symlink this will symlink its content.

@dominiquehunziker
Copy link
Contributor Author

dominiquehunziker commented Jun 28, 2016

Yes, that works too.
However, the behavior is not exactly the same. In your case you would have to rerun catkin if you add a file to the symlinked directory. In my case this is transparent and, therefore, reflects the Python behavior in the devel space better.

@NikolausDemmel
Copy link
Member

This change looks good to me. Haven't tested though.

@wjwwood
Copy link
Member

wjwwood commented Jan 31, 2017

I just tested this and I was able to reproduce the issue and see that this pr fixes the issue. The code changes are pretty involved, but they make sense to me. @jbohren if you ever have time, it would be good for you to review this change too. However, I'm going to merge it for now as it seems to work and not break anything I can see.

Thanks @dominiquehunziker and everyone else who chipped in on the discussion. Sorry it took so long to merge.

@wjwwood wjwwood merged commit 5c73dd4 into catkin:master Jan 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants