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

Fix pip installs #5386

Merged
merged 1 commit into from Mar 18, 2019
Merged

Conversation

@orlnub123
Copy link
Contributor

@orlnub123 orlnub123 commented Mar 2, 2019

When readthedocs pip installs a previously installed core component the previous version doesn't get fully uninstalled. This is caused by the --ignore-installed flag and the results can be disastrous: https://stackoverflow.com/a/51916623

stsewd
stsewd approved these changes Mar 2, 2019
Copy link
Member

@stsewd stsewd left a comment

Makes sense for me, not sure if there was a reason for this in the past.

@stsewd stsewd requested review from ericholscher and Mar 2, 2019
humitos
humitos approved these changes Mar 3, 2019
Copy link
Member

@humitos humitos left a comment

Thanks!

Seems a safer and robust way to handle this.

@ericholscher ericholscher merged commit 1fc6e4c into readthedocs:master Mar 18, 2019
1 check passed
@vidartf
Copy link
Contributor

@vidartf vidartf commented Mar 27, 2019

This commit broke my build hard. I'm relying on some dependencies for my build that are only available via conda. The --force-reinstall basically makes avoiding pip >= 10 messages like these for deep dependencies unavoidable:

Cannot uninstall 'certifi'. It is a distutils installed project and thus we cannot accurately determine which files belong to it which would lead to only a partial uninstall.

Any workarounds or other hints would be very welcome!

Example failure: https://readthedocs.org/projects/ipyscales/builds/8821876/

@vidartf
Copy link
Contributor

@vidartf vidartf commented Mar 27, 2019

Xref after diggin through blame history: 433f198

@stsewd
Copy link
Member

@stsewd stsewd commented Mar 27, 2019

hmm, what about just removing the flag? What would be the default behaviour of pip? Is this only happening with conda?

@orlnub123
Copy link
Contributor Author

@orlnub123 orlnub123 commented Mar 28, 2019

I'm guessing the reason the --ignore-installed flag was added was to mimic what setup.py install --force does. The --force flag was added in 1617d96 but has no explanation as to why. All I could find about why you might want to use it is in this question. According to this post distutils/setuptools compares the file modification times to see if it should re-build the package but pip doesn't seem to have the same behavior and re-builds it regardless.

The only place the --ignore-installed and --force-reinstall flags get used is here (CTRL-F ignore_installed and force_reinstall). Outright removing it would probably cause some packages to get stale. Replacing it with --upgrade would be more sensible and shouldn't cause any problems.

@humitos
Copy link
Member

@humitos humitos commented Mar 28, 2019

Thanks @orlnub123 for your explanation and help here! I suggest you to open a new issue because I feel that this conversation will be lost here in a merged pull request.

I supposed that the change to --force-reinstall won't bring any issue, but it seems that we didn't consider this case with conda, unfortunately.

At this point, I'm not sure if we can have a "general pip command that works for all the cases" :(

@humitos
Copy link
Member

@humitos humitos commented Mar 28, 2019

Oh, I mixed usernames! The one with the problem is @vidartf :/

(so, please @vidartf, open a new issue :) )

@vidartf
Copy link
Contributor

@vidartf vidartf commented Mar 28, 2019

Thanks for the context! Using it, I've opened #5545.

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

Successfully merging this pull request may close these issues.

None yet

5 participants