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.

Sign up for GitHub

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

Jump to bottom

Overhaul virtualenv generation and add Python 3 support #1

Merged
merged 13 commits into from
Nov 22, 2017
Merged

Overhaul virtualenv generation and add Python 3 support #1

merged 13 commits into from
Nov 22, 2017

Conversation

paulbovbel
Copy link
Member

@paulbovbel paulbovbel commented Nov 21, 2017 •
edited
Loading

  • Dropped relocatable virtualenv since it's not python3 compatible
  • Pulled in lightly modified dh_virtualenv, which is great at 'moving' a virtualenv
  • Properly handling devel and install space virtualenv generation in cmake extras

The intent is to release this module publicly, so please be pedantic.

paulbovbel
paulbovbel commented Nov 21, 2017
View reviewed changes
catkin_virtualenv/cmake/catkin_install_python.cmake

file(READ ${program_file} program_contents)
Copy link
Member Author

Choose a reason for hiding this comment

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

This has gotten significantly simpler because of the bash-based python script loaders.

paulbovbel
paulbovbel commented Nov 21, 2017
View reviewed changes
catkin_virtualenv/package.xml
@@ -3,7 +3,7 @@
<name>catkin_virtualenv</name>
<version>0.0.0</version>
<description>Bundle a virtualenv with a catkin package.</description>
<license>BSD</license>
<license>GPL</license>
Copy link
Member Author

@paulbovbel paulbovbel Nov 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

linking with dh_virtualenv, so must inherit license

@paulbovbel paulbovbel force-pushed the python3-support branch 3 times, most recently from 47b2eef to 3abca2c Compare November 21, 2017 17:08
svwilliams
svwilliams approved these changes Nov 21, 2017
View reviewed changes
Copy link

@svwilliams svwilliams left a comment

Choose a reason for hiding this comment

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

I tried to find things to complain about. Also, you still have a Python3 unit test TODO. Don't know if you are planning to fix that up in this PR or not.

catkin_virtualenv/cmake/catkin_install_python.cmake Outdated
@@ -65,76 +8,65 @@ function(catkin_install_python)
message(FATAL_ERROR "catkin_install_python() called without required DESTINATION argument.")
endif()

catkin_generate_virtualenv()

# If this package doesn't need a virtualenv, bypass processing python scripts
Copy link

Choose a reason for hiding this comment

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

We don't seem to be "bypassing" any longer. Comment needs updating.

catkin_virtualenv/cmake/catkin_install_python.cmake Outdated

if(EXISTS ${program_file})
stamp(${program_file}) # Reconfigure when the python script changes. This mirrors upstream behaviour.
# # Use CMake templating to load virtualenv into all specified python scripts
Copy link

Choose a reason for hiding this comment

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

Double "#' in comment

test_catkin_virtualenv_py3/test/test_virtualenv_script.test Outdated
@@ -0,0 +1,4 @@
<?xml version="1.0"?>
<launch>
<test test-name="test_virtualenv_script_py3" pkg="test_catkin_virtualenv_py3" type="test_virtualenv_script" />
Copy link

Choose a reason for hiding this comment

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

I'm not sure what the "best practices" are for naming things, but I find the ROS generated test names to be frustrating. They throw in an extra "test" in places.

Outputting test results to .../rostest-test_test_virtualenv_script.xml                                                                                      
[Testcase: testtest_virtualenv_script] ... ok

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

README.md Outdated

Make sure your python executables are installed using `catkin_install_python`:
# Generate the virtualenv, potentially in PYTHON3 mode
Copy link

Choose a reason for hiding this comment

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

Inconsistent spacing between # and text

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@paulbovbel
Copy link
Member Author

I'm not planning to sink any more time into python3 rostest at the moment, I'll come back to it in a sprint or if it's in demand.

@paulbovbel
Copy link
Member Author

paulbovbel commented Nov 21, 2017 •
edited
Loading

Will copyrightify after PR is merged

@paulbovbel paulbovbel force-pushed the python3-support branch 6 times, most recently from e5b2388 to 212ac36 Compare November 21, 2017 19:30
tappan-at-git
tappan-at-git requested changes Nov 21, 2017
View reviewed changes
Copy link

@tappan-at-git tappan-at-git left a comment

Choose a reason for hiding this comment

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

If we're releasing this into the public sphere, I'd suggest adding explicit expectations about issues/support requests. e.g. An issue template which just says in big letters "If you can't figure out how X works, consult ROS answers first " might be valuable.
(Mostly because I don't love getting notifications for the repos I follow when someone sends a confused message about, e.g., how to compile.)

catkin_virtualenv/cmake/catkin_generate_virtualenv.cmake Outdated
DEPENDS ${requirements_list}
)

# Build a virtualenv in CMAKE_BINARY_DIR, with internal file structure set up for in devel or installspace.
Copy link

Choose a reason for hiding this comment

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

I think you want to strike either "for" or "in". Technically, "devel-" should be hyphenated if you're shorthanding "develspace or installspace".

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

catkin_virtualenv/cmake/catkin_generate_virtualenv.cmake Outdated
DEPENDS ${generated_requirements}
)

# Symlink virtualenv to the destination - this really only has an effect in develspace.
Copy link

Choose a reason for hiding this comment

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

"-" should be an em-dash ("--").

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

catkin_virtualenv/scripts/build_venv Outdated
parser.add_argument('--requirements', required=True, help="Packages to install into the virtual environment.")
parser.add_argument('--venv-dir', required=True, help="Directory where to generate the virtual environment.")
parser.add_argument('--install-dir', help="Directory where the virtual environment will be installed.")
parser.add_argument('--python3', action='store_true', help="Python interpreter")
Copy link

Choose a reason for hiding this comment

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

help="Use the Python 3 interpreter instead of Python 2."

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

catkin_virtualenv/scripts/build_venv Outdated
if __name__ == '__main__':
parser = argparse.ArgumentParser(description="Build a relocatable virtualenv.")
parser.add_argument('--requirements', required=True, help="Packages to install into the virtual environment.")
parser.add_argument('--venv-dir', required=True, help="Directory where to generate the virtual environment.")
Copy link

Choose a reason for hiding this comment

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

"Where to" -> "Directory in which to generate the virtual environment."

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

catkin_virtualenv/scripts/build_venv Outdated
parser = argparse.ArgumentParser(description="Build a relocatable virtualenv.")
parser.add_argument('--requirements', required=True, help="Packages to install into the virtual environment.")
parser.add_argument('--venv-dir', required=True, help="Directory where to generate the virtual environment.")
parser.add_argument('--install-dir', help="Directory where the virtual environment will be installed.")
Copy link

Choose a reason for hiding this comment

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

Confusing -- the venv is generated in one directory, then installed in another. Should --venv-dir be "Directory from which to generate a venv"? Or maybe this should be "Directory where requirements for the virtual environment will be installed"?

Copy link
Member Author

Choose a reason for hiding this comment

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

so it's generated in directory one, but the paths have to be rewritten for it to live in directory two, whether it's develspace, installspace, or /opt/ros

one = venv_dir
two = install_dir

any suggestions on how to clarify?

Copy link
Member Author

Choose a reason for hiding this comment

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

See comments since this is now hidden.

catkin_virtualenv/src/dh_virtualenv/deployment.py
import tempfile

ROOT_ENV_KEY = 'DH_VIRTUALENV_INSTALL_ROOT'
DEFAULT_INSTALL_DIR = '/opt/venvs/'
Copy link

Choose a reason for hiding this comment

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

Ditto.

catkin_virtualenv/src/dh_virtualenv/deployment.py Outdated
# self.virtualenv_install_dir = os.path.join(install_root, install_suffix)
# self.package_dir = os.path.join(self.debian_root, install_suffix)

# (pbovbel) slightly modified to allow specifiying exactly which dir houses the generated and installed venv
Copy link

Choose a reason for hiding this comment

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

If you wanted to preserve the old behavior in some format, you could pepper the old logic with appropriate if not package_dir and if not install_dir blocks.

catkin_virtualenv/src/dh_virtualenv/deployment.py
self.pip_args.extend(extra_pip_arg)

@classmethod
def from_options(cls, package, options):
Copy link

Choose a reason for hiding this comment

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

Not updated with your new options.

catkin_virtualenv/src/dh_virtualenv/deployment.py

@classmethod
def from_options(cls, package, options):
verbose = options.verbose or os.environ.get('DH_VERBOSE') == '1'
Copy link

Choose a reason for hiding this comment

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

May need to adjust if you want to use it in the script. (also I really thought os.environ would convert variables to integers, but it super doesn't.)

catkin_virtualenv/src/dh_virtualenv/deployment.py
self.setuptools = setuptools
self.python = python
self.builtin_venv = builtin_venv
self.sourcedirectory = '.' if sourcedirectory is None else sourcedirectory
Copy link

Choose a reason for hiding this comment

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

Upstream's disdain for underscores is sad. :(

@paulbovbel
Copy link
Member Author

@tappan-at-git I've eliminated the venv-dir arg entirely to simplify the build_venv usage. The venv is generated into the CWD, and the paths are rewritten for install_dir.

@paulbovbel paulbovbel force-pushed the python3-support branch 2 times, most recently from 8cea76a to 2bad526 Compare November 22, 2017 18:54
@paulbovbel paulbovbel merged commit 5abf06a into devel Nov 22, 2017
@paulbovbel paulbovbel deleted the python3-support branch November 22, 2017 19:02
@kronos30 kronos30 mentioned this pull request Jun 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tappan-at-git tappan-at-git tappan-at-git approved these changes

@svwilliams svwilliams svwilliams approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@paulbovbel @tappan-at-git @svwilliams