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

Help find catkin python for install underlays #406

Merged
merged 1 commit into from
May 1, 2013

Conversation

stonier
Copy link
Contributor

@stonier stonier commented Apr 8, 2013

This is a follow on from #397

@stonier
Copy link
Contributor Author

stonier commented Apr 8, 2013

This is safe - side effects such as taking care of environment variables like ROS_PACKAGE_PATH and ROS_ROOT are not handled by this script. That is done by devel/env.sh and this script just computes the difference between environments. See the discussion in #397.

@dirk-thomas
Copy link
Member

If I understand the discussion right (sorry, has been a while) you extend the sys path to be able to find the following catkin.* imports. It is only relavant if the PYTHONPATH was not set before invoking it and it is not used to find python stuff from other packages, right?

If that is true, I would recommend we update the condition in line 8 to not look for a file "catkinConfig.cmake.in" (which does not get installed) but for a different file e.g. "toplevel.cmake" which is there in source and install space.

@stonier
Copy link
Contributor Author

stonier commented Apr 30, 2013

To the first question, yes - it is just looking for catkin's python path, nothing else.

To the second point - looking for an installed file like toplevel.cmake is moot since catkin's python does not reside at the relative ../python location (in the install space it is in /opt/ros/groovy/lib/python2.7/dist-packages/catkin. ../../../lib/python2.7/dist-packages/catkin is very different to ../python.

Hence the two separate search rules.

Having a look at that commit again, it should probably use an else argument so that it doesn't override the location if found in the source space. It could probably hunt in those workspace python paths specifically for catkin's package before adding it PYTHONPATH and break once it finds one too. That perhaps is just extra code for no valuable reason.

I'll update tonight.

@dirk-thomas
Copy link
Member

Oh, I didn't see that. That's why we check for catkinConfig.cmake.in. I can make a patch which uses your loop over the workspaces and adds the one specific folder which contains catkin.

@dirk-thomas
Copy link
Member

The following looks clean to me - what do you think?

# find the import relatively if available to work before installing catkin or overlaying installed version
if os.path.exists(os.path.join('@catkin_EXTRAS_DIR@', 'catkinConfig.cmake.in')):
    sys.path.insert(0, os.path.join('@catkin_EXTRAS_DIR@', '..', 'python'))
try:
    from catkin.environment_cache import generate_environment_script
except ImportError:
    # search for catkin package in all workspaces and prepend to path
    for workspace in "@CATKIN_WORKSPACES@".split(';'):
        python_path = os.path.join(workspace, '@CATKIN_GLOBAL_PYTHON_DESTINATION@'))
        if os.path.isdir(os.path.join(python_path, 'catkin'):
            sys.path.insert(0, python_path)
            break
    from catkin.environment_cache import generate_environment_script

@stonier
Copy link
Contributor Author

stonier commented May 1, 2013

Just tested it with a couple of erratic ')' bug fixes:

if os.path.exists(os.path.join('@catkin_EXTRAS_DIR@', 'catkinConfig.cmake.in')):
    sys.path.insert(0, os.path.join('@catkin_EXTRAS_DIR@', '..', 'python'))
try:
    from catkin.environment_cache import generate_environment_script
except ImportError:
    # search for catkin package in all workspaces and prepend to path
    for workspace in "@CATKIN_WORKSPACES@".split(';'):
        python_path = os.path.join(workspace, '@CATKIN_GLOBAL_PYTHON_DESTINATION@')
        if os.path.isdir(os.path.join(python_path, 'catkin')):
            sys.path.insert(0, python_path)
            break
    from catkin.environment_cache import generate_environment_script

Works great.

@stonier
Copy link
Contributor Author

stonier commented May 1, 2013

Updated and rebased the commit.

@@ -4,10 +4,19 @@ import os
import stat
import sys

# find the import relatively if available to work before installing catkin or overlaying installed version
# find the import for catkin's python module - either from devel space or from an installed underlay
Copy link
Member

Choose a reason for hiding this comment

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

Two minor formulation issues:

  • "module" -> "package" since there is no module "catkin" - only a package

  • "devel space" is not correct, it must be "source space" since the python files are not copied.

    find the import for catkin's python package - either from source space or from an installed underlay

@stonier
Copy link
Contributor Author

stonier commented May 1, 2013

Updated.

@dirk-thomas
Copy link
Member

Thank you, for taking the time to push on this.

dirk-thomas added a commit that referenced this pull request May 1, 2013
Help find catkin python for install underlays
@dirk-thomas dirk-thomas merged commit d5b0535 into ros:groovy-devel May 1, 2013
cwecht pushed a commit to cwecht/catkin that referenced this pull request Mar 20, 2018
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