Support VCS requirements
Closed this issue · 17 comments
Consider a package defining a requirement in editable format (as documented here):
-e git+https://github.com/timtadh/pyflwor.git@f3861f73b2bf6106a0363c15d5de040d8b01d50f#egg=pyflwor
catkin_virtualenv (0.3.0) fails with a parse error as follows:
Scanning dependencies of target catkin_virtualenv_editable_error_generate_virtualenv
[ 33%] Generating generated_requirements.txt
Traceback (most recent call last):
File "/home/nlimpert/dev/rosin_ws/src/catkin_virtualenv/catkin_virtualenv/scripts/combine_requirements", line 71, in <module>
sys.exit(combine_requirements(**vars(args)))
File "/home/nlimpert/dev/rosin_ws/src/catkin_virtualenv/catkin_virtualenv/scripts/combine_requirements", line 43, in combine_requirements
requirement = Requirement(requirement_string)
File "/usr/lib/python2.7/dist-packages/packaging/requirements.py", line 94, in __init__
requirement_string[e.loc:e.loc + 8]))
packaging.requirements.InvalidRequirement: Invalid requirement, parse error at "'-e git+h'"
CMakeFiles/catkin_virtualenv_editable_error_generate_virtualenv.dir/build.make:70: recipe for target 'generated_requirements.txt' failed
make[2]: *** [generated_requirements.txt] Error 1
make[2]: *** Deleting file 'generated_requirements.txt'
CMakeFiles/Makefile2:163: recipe for target 'CMakeFiles/catkin_virtualenv_editable_error_generate_virtualenv.dir/all' failed
make[1]: *** [CMakeFiles/catkin_virtualenv_editable_error_generate_virtualenv.dir/all] Error 2
Makefile:138: recipe for target 'all' failed
make: *** [all] Error 2
Note: The particular line was taken from executing pip freeze - (re-)installing it via pip install -r requirements.txt works.
Please find a small example package here.
I'd love to be able to support VCS sources in catkin_virtualenv (https://pip.pypa.io/en/stable/reference/pip_install/#vcs-support). Happy to take patches!
Editable mode (as well as local sources) is a bit more esoteric - where would you imagine that being useful? We primarily use catkin_virtualenv as a way to easily bundle pip dependencies into debian packages.
Editable mode (as well as local sources) is a bit more esoteric - where would you imagine that being useful? We primarily use catkin_virtualenv as a way to easily bundle pip dependencies into debian packages.
I agree that an editable mode doesn't make sense for catkin_virtualenv (I was basically pointing at VCS support - sorry for the noise).
@paulbovbel: do you have any idea why this isn't working right now? I've not looked at this myself, but would you have any idea what is throwing that parsing error?
the combine_requirements script tries to parse the entry into a packaging.Requirement (https://github.com/locusrobotics/catkin_virtualenv/blob/devel/catkin_virtualenv/scripts/combine_requirements#L43), which clearly doesn't know how to deal with a VCS requirement.
It may be as simple as providing a ducktyped VCSRequirement namedtuple as a complement to (https://github.com/locusrobotics/catkin_virtualenv/blob/devel/catkin_virtualenv/scripts/combine_requirements#L31) and inserting it into the combined_requirements requirements dict for later serialization (https://github.com/locusrobotics/catkin_virtualenv/blob/devel/catkin_virtualenv/scripts/combine_requirements#L58).
Ah, so it seems we were (or at least I was) conflating pip functionality with virtualenv/catkin_virtualenv.
Thanks for the hints.
You have the right idea, since catkin_virtualenv uses pip under the hood, and tries to emulate pip's functionality when merging the pip_requirements from a catkin package's dependency chain.
You have the right idea, since
catkin_virtualenvuses pip under the hood, and tries to emulatepip's functionality when merging the pip_requirements from a catkin package's dependency chain.
Could we not use pips parsing infrastructure for that?
(apologies for the many questions, perhaps I should start reading the code)
No problem. We could, and after delving through pip's source, I thought the packaging library would be sufficient (it's vendored into pip). More than happy to go one level up if necessary.
Apparently there is a way to do this by using pip's own parser: Using Python's pip Module Programmatically. Unfortunately we're apparently not supposed to do this, and the pip developers have moved the req module to _internal.req: a clear sign that this could change at any time (and thus break catkin_virtualenv at any time).
I don't expect there to be many changes to requirements.txt processing that would break catkin_virtualenv though, as in the end combine_requirements seems to just convert (potentially many) requirements.txt files into a single large one. It's pip requirements in -> pip requirements out, and any changes to the pip requirements.txt file format should be transparent to us (as long as we can detect whether several requirements.txt files state the same dependency).
@paulbovbel: how "ok" would it be to depend on a 'random' other module that implements requirements.txt parsing (such as requirements-parser)?
It isn't available as a deb though, so that would complicate things -- again.
It looks like all of pip's API is considered internal. I'm open to either approach - the deb availability is a pain (which is why this package exists!), so a reasonable approach would be to vendor pip._internal.req or requirements-parser. You can see that I've already had to do this with dh_virtualenv.
pip._internal.req depends on many other pip._internal modules, so that is probably not an option (we'd essentially be including the entirety of pip).
I'll see how "good" requirements-parser is.
I needed this capability so I threw together something that works. https://github.com/locusrobotics/catkin_virtualenv/compare/dev-support-vc
I don't think this is necessarily the right solution. Feels like a lot of duplicated effort here as I stole directly from pipenv and also noticed that our Requirement class doesn't even get used in favour of packaging. Requirement.
But maybe it helps someone in the meantime.
Thanks for making this available @ablakey.
Adding support for this got sidelined as the VCS dependency "disappeared" in our case.
No problem. Thank you and @nlimpert for starting the conversation. I'll probably take the torch from here. I think it's a worthwhile feature to support, particularly as I use custom branches all the time from VCS and I'm sure others do too.
Looks like a perfectly reasonable solution to me, short of vendoring all of pip :) Would you mind opening a PR? PRed myself for expediency
Notable that you can actually extract self.name from a regex capture group (https://github.com/locusrobotics/catkin_virtualenv/compare/dev-support-vc#diff-4867c3deece8624b92fb57319f84c089R105)
I've tested #40 and it does work (in that the code is checked out and installation is started).
Something I've ran into is that if the VCS requirement has a requirements.txt itself stored in the repository, those deps are not considered when merging all the requirements files. Makes sense of course, as the various scripts used for that in catkin_virtualenv don't know about that file (yet).
This is not necessarily a problem with the approach in #40. Just something that seems to complicate using the VCS entries.