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

Fixes issues with pip arguments being used when they shouldn't and not being used when they should #1583

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
3 participants
@donovan6000
Copy link

commented Nov 7, 2016

What does this PR do and why is it necessary?

Makes it so that installing, uninstalling, and updating plugins uses the appropriately provided pip arguments.

How was it tested? How can it be tested by the reviewer?

Testing was done by setting the pluginmanager's pip_args setting to various values, like --user and nothing, and seeing if I could then install, uninstall, and update a plugin without encountering any unexpected issues.

Any background context you want to provide?

Pip wont uninstall a package if it's provided with inapplicable arguments like --user, --ignore-installed, etc, and it'll just return an error whenever that happens. So using the same user provided pip arguments to install and uninstall a plugin can result in breaking the ability to uninstall plugins.

I felt like the softwareupdater plugin should use the pluginmanager's pip_args setting when it uses pip to update a plugin. Since those pip arguments are what was needed to install the plugin in the first place then they are also needed to install the newest version of that plugin.

What are the relevant tickets if any?

None

Screenshots (if appropriate)

Not appropriate

Further notes

I left in code to make these changes easily scalable to allow adding more arguments that can be blacklisted depending on if pip is installing or uninstalling a package. As of right now it just includes blacklisting the --user argument when uninstalling a package since that's what I was most concerned with. I know that there's already a pip_force_user setting that deals with the --user argument, but this PR deals with a different issue since it's designed to fix problems with the pip arguments that are provided by the user.

donovan6000 added some commits Nov 6, 2016

@GitIssueBot

This comment has been minimized.

Copy link
Collaborator

commented Nov 7, 2016

Hi @donovan6000,

Thank you for your contribution! Sadly it looks like there is something wrong with this PR from your branch devel to OctoPrint:devel:

  • Your PR's source branch devel is among the blacklisted source branches: master, devel, maintenance. Please always create PRs from a custom branch in your repository to avoid accidental commits making it into your PR.

Please take a look at the section on PRs in the Contribution Guidelines and make sure that your PR follows them. Thank you!

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being, so don't expect any replies from me :) Your PR is read by humans too, I'm just not one of them.

if self._pip_caller is None or not self._pip_caller.available:
raise RuntimeError(u"No pip available, can't operate".format(**locals()))

if "--process-dependency-links" in args:
self._log_message(u"Installation needs to process external dependencies, that might make it take a bit longer than usual depending on the pip version")

additional_args = self._settings.get(["pip_args"])
if additional_args:
args.append(additional_args)

This comment has been minimized.

Copy link
@foosel

foosel Nov 8, 2016

Owner

Suggestion: The pip_usage_type is actually always the first argument we have in our args. Why not have a class property pip_inapplicable_arguments which is a dict, with the pip command as key and the inapplicable arguments list as value?

pip_inapplicable_arguments = dict(uninstall=["--user"])

Then, we can simplify that whole segment to

if additional_args is not None:
    inapplicable_arguments = self.__class__.pip_inapplicable_arguments.get(args[0], list())
    for inapplicable_argument in inapplicable_arguments:
        additional_args = re.sub("(^|\s)" + re.escape(inapplicable_argument) + "\\b", "", additional_args)
    if additional_args:
        args.append(additional_args)

That way, if the list has to be extended later, simply adjusting the contents of the dict will be enough.

@@ -71,6 +72,18 @@ def _log(lines, prefix=None, stream=None):
if "dependency_links" in check and check["dependency_links"]:
pip_args += ["--process-dependency-links"]

additional_args = octoprint.plugin.plugin_manager().plugin_implementations["pluginmanager"]._settings.get(["pip_args"])

This comment has been minimized.

Copy link
@foosel

foosel Nov 8, 2016

Owner

Hm... I don't like that. I know why you are doing it, and the reason is valid, but it couples both plugins and that should be avoided if possible.

I'm wondering if it would be a better idea to maybe extract the whole pip configuration from the plugin manager and into a general platform wide configuration, to be used by both the plugin manager and the software update plugin. Basically taking the extraction of the pip helper one step further. Would necessitate a migration step for the config.

donovan6000
Reorganized how pluginmanager blacklists pip arguments and removed us…
…ing the pluginmanager's pip arguments in the softwareupdate plugin
@donovan6000

This comment has been minimized.

Copy link
Author

commented Nov 10, 2016

Updated the PR to use the recommendations in your code review.

I removed the changes to the softwareupdate plugin since you prefer that the plugins remain independent of one another.

@foosel

This comment has been minimized.

Copy link
Owner

commented Nov 22, 2016

Squashed as new single commit 1b9bfc6 and merged to devel. Thanks!

@foosel foosel closed this Nov 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.