Fixes issues with pip arguments being used when they shouldn't and not being used when they should #1583
Conversation
…stalling a package
… when it uses pip to update a plugin
|
Hi @donovan6000, Thank you for your contribution! Sadly it looks like there is something wrong with this PR from your branch
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, 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. |
GitIssueBot
added
the
needs some work
| 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) | ||
|
|
There was a problem hiding this comment.
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"]) | |||
There was a problem hiding this comment.
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.
…ing the pluginmanager's pip arguments in the softwareupdate plugin
|
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. |
|
Squashed as new single commit 1b9bfc6 and merged to |
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
--userand 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
--userargument 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--userargument, 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.