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

RST-3172 Check that requirements file is locked #62

Merged
merged 7 commits into from May 28, 2020
Merged

Conversation

paulbovbel
Copy link
Member

@paulbovbel paulbovbel commented May 27, 2020

From discussion with @abencz, we probably don't want to blindly recommend INPUT_REQUIREMENTS, and instead just ensure that requirements are actually locked.

This actually satisfies the spirit of the RST-3172 better, since we can detect any insufficiently locked requirements.

Sneaks in a python3 port of catkin_virtualenv proper.

@paulbovbel paulbovbel force-pushed the check-venv branch 6 times, most recently from 56abf99 to ee9903a Compare May 27, 2020 19:30
@paulbovbel paulbovbel changed the title Check that lock file is locked RST-3172 Check that lock file is locked May 27, 2020
@paulbovbel paulbovbel force-pushed the check-venv branch 6 times, most recently from e3b94ae to 3997b66 Compare May 27, 2020 19:58
@paulbovbel paulbovbel requested review from abencz and ablakey May 27, 2020 19:59
@paulbovbel paulbovbel marked this pull request as ready for review May 27, 2020 19:59
@paulbovbel paulbovbel force-pushed the check-venv branch 2 times, most recently from 299ad4c to 1ed764a Compare May 27, 2020 20:05
@@ -1,4 +1,4 @@
#!/usr/bin/env python
#!/usr/bin/env python3
Copy link
Member Author

Choose a reason for hiding this comment

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

I really got tired of the python2 subprocess module. Onward!

@paulbovbel paulbovbel changed the title RST-3172 Check that lock file is locked RST-3172 Check that requirements file is locked May 27, 2020
catkin_virtualenv/scripts/venv_check Show resolved Hide resolved
catkin_virtualenv/src/catkin_virtualenv/venv.py Outdated Show resolved Hide resolved

args = parser.parse_args()

extra_pip_args = args.extra_pip_args[1:-1]
Copy link
Contributor

Choose a reason for hiding this comment

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

It's weird that args.extra_pip_args includes the wrapping "". Somewhere along the line extra quotes are being inserted?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it was some weird mess with passing args from cmake list->shell->python, where some of those args are prefixed with - but shouldn't be processed by argparse...

I won't pretend this is the best way - I mostly got there via trial and error until something worked.

Copy link
Contributor

@abencz abencz May 28, 2020

Choose a reason for hiding this comment

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

Maybe args.extra_pip_args.strip("\"\'") would be more robust if some of this pipeline changes.

catkin_virtualenv/scripts/venv_check Outdated Show resolved Hide resolved
catkin_virtualenv/scripts/venv_check Outdated Show resolved Hide resolved
catkin_virtualenv/scripts/venv_check Outdated Show resolved Hide resolved
parser.add_argument(
'--requirements', required=True, help="Requirements to check.")
parser.add_argument(
'--extra-pip-args', default="\"\"", type=str, help="Extra pip args for install.")
Copy link
Contributor

Choose a reason for hiding this comment

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

This any different from '""' ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think black will complain about '""' :(

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh. Dunno why I didn't think of that earlier, works for me.

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

Successfully merging this pull request may close these issues.

None yet

3 participants