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

Support for installing virtual packages (Debian) #521

Merged
merged 8 commits into from
Jan 9, 2018
Merged

Support for installing virtual packages (Debian) #521

merged 8 commits into from
Jan 9, 2018

Conversation

mathias-luedtke
Copy link
Contributor

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:

docker build https://gist.githubusercontent.com/ipa-mdl/401f74bff9bfe64fd359c4593c4e3797/raw/028f67ecddeb38180eeae32c122d185266b003a0/Dockerfile

Copy link
Contributor

@wjwwood wjwwood left a 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.

@tfoote
Copy link
Member

tfoote commented Jul 25, 2017

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.

@mathias-luedtke
Copy link
Contributor Author

mathias-luedtke commented Jul 25, 2017

The comment about using python-apt makes me wonder if we should reconsider using it.

I tried to migrate the comments. The implementation should be straight-forward.
However, I was unsure about the implications for non-apt systems.

@tfoote
Copy link
Member

tfoote commented Jul 25, 2017

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.

@wjwwood
Copy link
Contributor

wjwwood commented Jul 27, 2017

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.

@@ -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)
Copy link
Contributor Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

No 😄.

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):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

enumerate(sub_command)?

Copy link
Contributor

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.

@mathias-luedtke
Copy link
Contributor Author

mathias-luedtke commented Aug 1, 2017

rebased to current master. including #533

@mathias-luedtke
Copy link
Contributor Author

mathias-luedtke commented Aug 1, 2017

I have refactored the code to read all information from apt-cache showpkg.

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

@wjwwood
Copy link
Contributor

wjwwood commented Sep 1, 2017

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.

@mathias-luedtke
Copy link
Contributor Author

rebased to current master and squashed some commits.

@mathias-luedtke
Copy link
Contributor Author

I have added batch processing as proposed by @jacobperron.

@mathias-luedtke
Copy link
Contributor Author

mathias-luedtke commented Jan 9, 2018

The test failure seems to be unrelated.

@wjwwood
Copy link
Contributor

wjwwood commented Jan 9, 2018

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.

@wjwwood wjwwood merged commit 6113279 into ros-infrastructure:master Jan 9, 2018
@tfoote
Copy link
Member

tfoote commented Jan 10, 2018

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.

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

Successfully merging this pull request may close these issues.

rosdep add support for virtual packages
3 participants