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

Add system-site-packages setting (rebased) #2686

Closed

Conversation

wyl8899
Copy link
Contributor

@wyl8899 wyl8899 commented Jul 19, 2020

This PR is a rebase of #1982 by @kam1sh , making necessary changes to make the tests pass.

Please let me know if things are missing or need to be done. Thank you!

Pull Request Check List

Resolves: #1393

  • Added tests for changed code.
  • Updated documentation for changed code.

@wyl8899 wyl8899 force-pushed the 1393-system-site-packages branch 3 times, most recently from 3141d25 to f2d4101 Compare July 19, 2020 20:16
@abn abn changed the base branch from develop to master July 24, 2020 11:45
@abn
Copy link
Member

abn commented Oct 16, 2020

@wyl8899 assuming @kam1sh has not responded please see #1982 (comment)

@wyl8899
Copy link
Contributor Author

wyl8899 commented Oct 21, 2020

Sure, I will take a stab.

@wyl8899 wyl8899 force-pushed the 1393-system-site-packages branch from f2d4101 to 833d5da Compare October 21, 2020 15:53
@wyl8899
Copy link
Contributor Author

wyl8899 commented Oct 21, 2020

@abn Done! Could you please take a look?

I don't quite like the fact that assert_called_with... flags={"always-copy": False, "system-site-packages": False} checks are everywhere, though. Maybe it could be done better.

@wyl8899 wyl8899 force-pushed the 1393-system-site-packages branch from 833d5da to 287c405 Compare October 21, 2020 16:24
Copy link
Member

@abn abn left a comment

Choose a reason for hiding this comment

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

One minor change; otherwise looking good.

I agree we should make sure that we keep the defaults DRY when testing. Maybe another PR to do just that? :)

poetry/config/config.py Outdated Show resolved Hide resolved
@wyl8899 wyl8899 force-pushed the 1393-system-site-packages branch from 7028a5e to 9d2b951 Compare October 23, 2020 18:14
@wyl8899
Copy link
Contributor Author

wyl8899 commented Oct 23, 2020

Suggestions applied and squashed. Thanks!

@abn
Copy link
Member

abn commented Oct 24, 2020

This will need a rebase since a new boolean config was added in 1.1.4.

Copy link
Member

@abn abn left a comment

Choose a reason for hiding this comment

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

Changes look good. Rebase required.

@wyl8899 wyl8899 force-pushed the 1393-system-site-packages branch from 9d2b951 to ce550be Compare October 25, 2020 17:26
@jmahlik
Copy link

jmahlik commented Dec 3, 2020

Any updates on this, looks sooo close :) ? This would be a much loved feature vs. scripting changes to the .venv/pyvenv.cfg.

@wyl8899
Copy link
Contributor Author

wyl8899 commented Dec 4, 2020

Oops, totally forgot about this. (I use script to change pyvenv.cfg at $dayjob too.)

Let me do another rebase.

@wyl8899 wyl8899 force-pushed the 1393-system-site-packages branch from ce550be to 8b26bd5 Compare December 4, 2020 04:08
@wyl8899 wyl8899 force-pushed the 1393-system-site-packages branch from 8b26bd5 to f0fc9c2 Compare December 4, 2020 08:23
@wyl8899 wyl8899 requested a review from abn December 4, 2020 08:50
@wyl8899
Copy link
Contributor Author

wyl8899 commented Dec 4, 2020

@abn Could you please take a look?

@jmahlik
Copy link

jmahlik commented Dec 10, 2020

Not sure if this is related to poetry specifically but thought I'd mention it here to make sure it is accounted for.

When you have a package in the virtual env and that same package in your system site packages, poetry update will try to overwrite the package installed in the system site packages, which (on windows) produces the following (which it should IMO):

ERROR: Could not install packages due to an EnvironmentError: [WinError 5] Access is denied:

This specifically happened for the regex dependency of black for me. So it doesn't look like it tries to overwrite the system packages all the time, because black itself successfully updated.

Manually edited the .venv/pyvenv.cfg to include-system-site-packages = true

Not sure if this could be accounted for in the way poetry is updating packages with this option enabled. If so, great, if not, the docs should mention running this option might cause issues on poetry update.

Again, I'm not sure this is a poetry issue, but just wanted to get this out here.

@abn
Copy link
Member

abn commented Mar 21, 2021

Resolved via #3711 and #3817

@abn abn closed this Mar 21, 2021
Copy link

github-actions bot commented Mar 1, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support for venv --system-site-packages
4 participants