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

Arch Linux: Properly invoke pacman with the correct flags. #472

Merged

Conversation

de-vri-es
Copy link
Contributor

This PR fixes the invocation of pacman.

It addresses the following issues:

  • The previous implementation used pacman -Sy, which can perform a partial upgrade. This is very much not what you should do on Arch Linux [1].
  • The interactive argument was ignored, meaning pacman would always ask for confirmation and rosdep fails when invoked in non-interactive context.
  • Additionally it adds -q if quiet == true, although I believe this currently does not affect the output of pacman -S.

[1] https://wiki.archlinux.org/index.php?title=System_maintenance&redirect=no#Partial_upgrades_are_unsupported

@de-vri-es de-vri-es force-pushed the fix-pacman-invocation branch 6 times, most recently from eaa1b56 to 529d46f Compare September 6, 2016 09:27
@de-vri-es
Copy link
Contributor Author

I forgot:

  • --needed should not be passed when reinstall == true.

I also folded it all into a single pacman invocation, although I'm not sure what the reasoning was for putting it in multiple invocations. A single invocation has the advantage of possibly reducing the number of times hooks are invoked.

@wjwwood
Copy link
Contributor

wjwwood commented Sep 6, 2016

It probably made multiple calls to pacman because that way you can more easily narrow down which rosdep rule was causing which pacman package to get installed. I don't know if that's necessary to preserve or not.

@de-vri-es
Copy link
Contributor Author

It's still the same call to rosdep though, I presume? Only rosdep would invoke pacman once per package instead of once in total. Regardless, if pacman can not find a requested package, it will print a clear error message indicating which package(s) it could not find.

@wjwwood
Copy link
Contributor

wjwwood commented Sep 6, 2016

You don't call rosdep for each rosdep rule you want to resolve, you call it on a package (which has zero to many deps) or many packages. So sometimes it's not clear why rosdep tried to install some particular package.

It's the mapping from the rosdep rule to the pacman command I'm talking about, not whether or not pacman is clear which pacman package is getting installed. Most of the time the rosdep rules mapping are obvious, but not always.

I'm not 100% certain it helps, it's just my guess as to why it used to call it many times. I don't see any problem with making one call instead.


if not interactive: command.append('--noconfirm')
if not reinstall: command.append('--needed')
if quiet: command.append('-q')
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: this repository's files do not follow PEP8, but with additions we can try to follow it where it is not at odds with the files existing style. This isn't PEP8, please put the contents of the if block on new lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, will do

@wjwwood
Copy link
Contributor

wjwwood commented Sep 6, 2016

Other than a small comment, lgtm.

@de-vri-es
Copy link
Contributor Author

Adjusted for code style. Also removed #TODO to implement the interactive argument since that is now supported.

@wjwwood wjwwood merged commit 5be7b71 into ros-infrastructure:master Sep 6, 2016
@wjwwood
Copy link
Contributor

wjwwood commented Sep 6, 2016

Thanks!

@de-vri-es
Copy link
Contributor Author

Thanks for the quick merge :)

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.

2 participants