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

Fix setting CATKIN_GLOBAL_LIBEXEC_DESTINATION to libexec. #713

Closed
wants to merge 2 commits into from

Conversation

aballier
Copy link
Contributor

@aballier aballier commented Feb 2, 2015

I tried to set CATKIN_GLOBAL_LIBEXEC_DESTINATION to libexec, as described as possible in doc/user_guide/variables.rst, since I find this location more natural for nodes & co.
However, catkin_find could not find any node after that. This patch makes it possible to find nodes there, while maintaining the translation libexec -> lib.

@dirk-thomas
Copy link
Member

The order of search_dirs determines where resources are looked up first. Appending search_dirs_extra at the end would change the order and can therefore not be done as it is in this patch.

Also changing the variable CATKIN_GLOBAL_LIBEXEC_DESTINATION to anything else would not work since the Python logic does not know about that.

I would recommend to not override the libexec destination since it is currently not well supported. May be a note should be added to the documentation to make this clear.

@aballier
Copy link
Contributor Author

The order of search_dirs determines where resources are looked up first. Appending search_dirs_extra at the end would change the order and can therefore not be done as it is in this patch.

Right, didn't think about that.
search_dirs.insert(search_dirs.index('libexec'), 'lib') should do it then (and be simpler)

Also changing the variable CATKIN_GLOBAL_LIBEXEC_DESTINATION to anything else would not work since the Python logic does not know about that.

Could you please be more specific ? I've been using this for months without a single issue.

@dirk-thomas
Copy link
Member

If you would set CATKIN_GLOBAL_LIBEXEC_DESTINATION to e.g. foo the script catkin_find would still not work since it doesn't know about that path.

@aballier
Copy link
Contributor Author

Ah, yes indeed. Thanks.
Will update the PR with the above fix + a doc patch.

I tried to set CATKIN_GLOBAL_LIBEXEC_DESTINATION to libexec, as described as possible in doc/user_guide/variables.rst, since I find this location more natural for nodes & co.
However, catkin_find could not find any node after that. This patch makes it possible to find nodes there, while maintaining the translation libexec -> lib.
@aballier
Copy link
Contributor Author

Updated

@@ -71,6 +71,8 @@ They only contain relative paths and are supposed to be relative to the ``${CMAK

This is set to ``lib``.
On non-Debian distributions it could be set to ``libexec``.
Note that setting this variable to anything but ``lib`` or ``libexec`` will
not work as expected since the logic in the code expects one of these values.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of:

since the logic in the code expects one of these values.

I would write it like:

since tools like ``catkin_find`` will only consider these two locations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, updating it

…LIBEXEC_DESTINATION to values unknown to the Python code.
@dirk-thomas
Copy link
Member

Thank you. Squashed and merged in a4b3a23.

@dirk-thomas
Copy link
Member

ros/ros#83 indicates a regression due to this PR. Extending search_dirs should not be done in the workspaces loop but outside.

The duplicated return in the updated test should have been a strong indicator of that:

'baz/lib/foo/foopath',
'baz/lib/foo/foopath',

I have created #739 to address it.

@aballier
Copy link
Contributor Author

aballier commented Jun 3, 2015

oh, indeed, sorry & thanks for fixing it

i assumed the duplicated return was due to that search_dirs already contained 'lib', guess i was wrong

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

Successfully merging this pull request may close these issues.

2 participants