-
Notifications
You must be signed in to change notification settings - Fork 171
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
Support for installing virtual packages (Debian) #521
Support for installing virtual packages (Debian) #521
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes lgtm, but as you may imagine this has the chance to break the build farm. So I might need to do a rosdep release for the other changes, then do another release with just this shortly after so we can keep this change somewhat isolated and so we can test deploy it.
@tfoote can you review this too? I'm a little out of my depth with the more complex apt stuff.
The logic looks correct. The comment about using python-apt makes me wonder if we should reconsider using it. When we tried it early in the development cycle it wasn't mature enough. But maybe it is now. |
I tried to migrate the comments. The implementation should be straight-forward. |
Yeah, I think the challenge is that python-apt may not be made available on all systems. Since it really only makes sense on apt based systems. Which means that it would need to be conditionally imported etc to support systems where it's not available. |
Ok, this lgtm, but as I described above, I want to release this separately. So I'll do a release of rosdep with any pending changes/pr's and then I'll merge this and do another release a short time after. |
src/rosdep2/installers.py
Outdated
@@ -540,22 +540,41 @@ def install_resolved(self, installer_key, resolved, simulate=False, interactive= | |||
if simulate: | |||
print("#[%s] Installation commands:"%(installer_key)) | |||
for sub_command in command: | |||
print(' '+' '.join(sub_command)) | |||
if isinstance(sub_command[0], list): | |||
len = len(sub_command) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wjwwood: did you intend to overwrite the built-in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No 😄.
src/rosdep2/installers.py
Outdated
print(' %s (alternative %d/%d)'%(' '.join(sub_command[i]), i+1,l)) | ||
if isinstance(sub_command[0], list): | ||
len = len(sub_command) | ||
for cmd, i in enumerate(l): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enumerate(sub_command)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, sorry, that's what I get for relying on my local editor so much and then trying to edit with the web ui.
rebased to current master. including #533 |
I have refactored the code to read all information from Can be tests with the following Dockerfile (put in top folder): # rosindustrial/ci:hydro-precise ros:indigo ros:kinetic ros:lunar
FROM ros:lunar
RUN apt-get -qq update && apt-get -qq -y install sudo git python-setuptools \
&& { test -d /etc/ros/rosdep/sources.list.d || rosdep init; } \
&& echo "yaml https://github.com/jspricke/rosdistro/raw/fix_curl/rosdep/base.yaml" > /etc/ros/rosdep/sources.list.d/10-libcurl.list \
&& rosdep update \
&& git clone https://github.com/ros/resource_retriever /tmp/resource_retriever
COPY . /tmp/rosdep/
RUN cd /tmp/rosdep && python setup.py install \
&& rosdep install --rosdistro $(basename /opt/ros/*) -y --from-paths /tmp/resource_retriever |
Just an update, we're doing some migrations on the build farm and I'm also EOL'ing Jade right now. My plan is to do this and merge it after all that settles down, hopefully in a few weeks. Sorry for the delay, thanks for your patience. |
read virtual package state and providers in one run from apt-cache showpkg
rebased to current master and squashed some commits. |
I have added batch processing as proposed by @jacobperron. |
The test failure seems to be unrelated. |
I reran it and it passed this time. This should be good to go. I'll go ahead and merge this, but I need to coordinate the release with @tfoote and/or @nuclearsandwich since it has a off-chance of being disruptive on the buildfarm as well I think. |
As long as we push this out away from a sync window I don't think there's much way to test it without the full deployment. |
This PR should close #274 for real.
The major issue with virtual packages is that
apt
cannot install them directly. The user has to choose the appropriate provider.To automate this I have enhanced the install process by supporing nested command lists.
So instead of a simple list
['sudo', 'apt-get', 'install', pkg-a']
,[['sudo', 'apt-get', 'install', pkg-a'], ['sudo', 'apt-get', 'install', pkg-b']]
is valid, too.All commands get run one after another until one succeeds, the rest gets skipped
This feature is then used by
AptInstaller
to return commands for all package providers.Special care was taken for the reinstall flag: In this case just the installed package is returned.
The functionality can be tested with this Dockerfile: