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

Provide more CMake flags to customize behaviour #16

Merged
merged 3 commits into from
May 3, 2018
Merged

Provide more CMake flags to customize behaviour #16

merged 3 commits into from
May 3, 2018

Conversation

paulbovbel
Copy link
Contributor

@paulbovbel paulbovbel commented May 2, 2018 •
edited
Loading

Add support for:

  • different python versions (default 2)
  • using system site packages (default true)
  • isolating requirements globbing to just the calling package (default false)

@paulbovbel paulbovbel changed the title Make flags more flexible to support disabling system site packages Provide more CMake flags to customize behaviour May 2, 2018
@paulbovbel
Copy link
Contributor Author

Supercede #13

@paulbovbel paulbovbel force-pushed the system-site-packages branch 3 times, most recently from 79ccee6 to 6de9c39 Compare May 2, 2018 21:30
tappan-at-git
tappan-at-git approved these changes May 3, 2018
View reviewed changes
catkin_virtualenv/cmake/build_venv.py Outdated
use_system_packages=True,
python=find_executable('python3') if args.python3 else find_executable('python2'),
use_system_packages=args.use_system_packages,
python=find_executable('python' + args.python_version),
Copy link

Choose a reason for hiding this comment

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

If I'm reading the docs right, find_executable will return None if you ask for a non-existent version of python (say, someone types it wrong or messes up dependencies for a future development version). While the default value for the python kwarg here is None, it doesn't look like Deployment handles that case at all -- it'll blindly attempt to call subprocess on [None, "-m", "venv"] , which I doubt will return a clean error. It's probably worth checking whether this returns None and warning the user appropriately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, good call

catkin_virtualenv/cmake/catkin_generate_virtualenv.cmake
@@ -17,14 +17,27 @@
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
function(catkin_generate_virtualenv)
cmake_parse_arguments(ARG "PYTHON3" "" "" ${ARGN})
set(oneValueArgs PYTHON_VERSION_MAJOR USE_SYSTEM_PACKAGES ISOLATE_REQUIREMENTS)
cmake_parse_arguments(ARG "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN} )
Copy link

Choose a reason for hiding this comment

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

"${multiValueArgs}" is just being explicit/future-proofing, or are you missing some args?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just being explicit that that's what cmake_parse_argument expects. Undefined args get subbed as "".

@paulbovbel paulbovbel merged commit a6c552e into devel May 3, 2018
@paulbovbel paulbovbel mentioned this pull request May 3, 2018
Closed
@paulbovbel paulbovbel deleted the system-site-packages branch March 18, 2019 21:20
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

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@paulbovbel @tappan-at-git