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

Rework determination of virtualenv site-packages flags for v20 #578

Closed

Conversation

PeterJCLaw
Copy link

@PeterJCLaw PeterJCLaw commented Oct 31, 2020

virtualenv 20.0 removed --no-site-packages so we cannot pass that on that version. This means we have three version scenarios to cope with:

  • < 1.7: site packages installed by default, with --no-site-packages to opt out

  • 1.7 - 20.0: --no-site-packages present and default, --system-site-packages to opt back in

  • > 20.0: no site packages by default, --system-site-packages to opt back in

This PR aims to add support for the last of these, so that all three are supported.

This was mentioned at #534 (comment) and I've hit this myself (on Fedora 33, where this patch fixes it for me).

Meta: I've not yet added any tests; I'm not sure how to do that, but if you can point me in the right direction I'm happy to give it a go.

virtualenv 20.0 removed --no-site-packages so we cannot pass that.
This means we have three version scenarios to cope with:

 < 1.7: site packages installed by default, with --no-site-packages
        to opt out

 1.7 - 20.0: --no-site-packages present and default,
             --system-site-packages to opt back in

 > 20.0: no site packages by default, --system-site-packages to opt
         back in
$system_pkgs_flag = $systempkgs ? {
# --system-site-packages was introducd in 1.7, and --no-site-packages
# became the default
true => versioncmp($_virtualenv_version,'1.7') > 0 ? {
Copy link
Author

Choose a reason for hiding this comment

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

Note: I thiiink that this should probably be >=, however I believe the current spelling has the same error.

PeterJCLaw added a commit to PeterJCLaw/sr-server-puppet that referenced this pull request Oct 31, 2020
This moves to my fork of puppet-python as it contains a fix for
working with virtualenv >= 20. Otherwise this is just poking
puppet-python to cope with Fedora 33 *only* having Python 3,
which it makes it available as both `python` and `python3`.

See voxpupuli/puppet-python#578 for the
aforementioned virtualenv fix.
PeterJCLaw added a commit to PeterJCLaw/sr-server-puppet that referenced this pull request Oct 31, 2020
This moves to my fork of puppet-python as it contains a fix for
working with virtualenv >= 20. Otherwise this is just poking
puppet-python to cope with Fedora 33 *only* having Python 3,
which it makes it available as both `python` and `python3`.

See voxpupuli/puppet-python#578 for the
aforementioned virtualenv fix.
PeterJCLaw added a commit to PeterJCLaw/sr-server-puppet that referenced this pull request Oct 31, 2020
This moves to my fork of puppet-python as it contains a fix for
working with virtualenv >= 20. Otherwise this is just poking
puppet-python to cope with Fedora 33 *only* having Python 3,
which it makes it available as both `python` and `python3`.

See voxpupuli/puppet-python#578 for the
aforementioned virtualenv fix.
PeterJCLaw added a commit to PeterJCLaw/sr-server-puppet that referenced this pull request Nov 1, 2020
This moves to my fork of puppet-python as it contains a fix for
working with virtualenv >= 20. Otherwise this is just poking
puppet-python to cope with Fedora 33 *only* having Python 3,
which it makes it available as both `python` and `python3`.

See voxpupuli/puppet-python#578 for the
aforementioned virtualenv fix.
PeterJCLaw added a commit to PeterJCLaw/sr-server-puppet that referenced this pull request Nov 1, 2020
This moves to my fork of puppet-python as it contains a fix for
working with virtualenv >= 20. Otherwise this is just poking
puppet-python to cope with Fedora 33 *only* having Python 3,
which it makes it available as both `python` and `python3`.

See voxpupuli/puppet-python#578 for the
aforementioned virtualenv fix.
PeterJCLaw added a commit to PeterJCLaw/sr-server-puppet that referenced this pull request Nov 1, 2020
This moves to my fork of puppet-python as it contains a fix for
working with virtualenv >= 20. Otherwise this is just poking
puppet-python to cope with Fedora 33 *only* having Python 3,
which it makes it available as both `python` and `python3`.

See voxpupuli/puppet-python#578 for the
aforementioned virtualenv fix.
@crazymind1337
Copy link
Member

crazymind1337 commented Nov 9, 2020

I have tested your branch at our CI. The result is what I expected:

The CI fails due to the fact $virtualenv is not set, because it is not installed for a fresh system and the fact itself is been set in the beginning of the run and not been updated while virtualenv is been installed. Therefore the comparison which you are making is not catching up.

My PR which was based on the $OS, like: if $os == 20.04 => remove --no-site-packages parameter has not been accepted due to the fact the removal of the parameter is based on the virtualenv version.

I think we will have to modify the exec for virtualenv. Maybe we make 2 execs with an onlyif parameter which differs on the different virtualenv version.

@crazymind1337
Copy link
Member

crazymind1337 commented Nov 9, 2020

The actual cases are:

  • $systempkgs = true and $virtualenv_version > 1.7 and $virtualenv_version < 20:
    $system_pkgs_flag = "--system-site-packages"

  • $systempkgs = false and $virtualenv_version > 1.7 and $virtualenv_version < 20:
    $system_pkgs_flag = "--no-site-packages"

  • $systempkgs = true and $virtualenv_version >= 20:
    $system_pkgs_flag = "--system-site-packages"

  • $systempkgs = false and $virtualenv_version >= 20:
    $system_pkgs_flag = ""

Which in the end is:

  • $systempkgs = true:
    $system_pkgs_flag = "--system-site-packages"

  • $systempkgs = false:
    $virtualenv_version < 20:
    $system_pkgs_flag = "--no-site-packages"
    $virtualenv_version >= 20
    $system_pkgs_flag = ""

I am not sure if we still need to check on the case if $virtualenv_version < 1.7. On any OSes we are using the $virtualenv_version is > 1.7.

But still: The value for the fact $virtualenv_version is unset on the fresh installation. Therefore we only can rely on the fact if it is set.

@PeterJCLaw
Copy link
Author

Ah, just seen that #552 was a previous attempt at this.

The actual cases are:

  • $systempkgs = true and $virtualenv_version > 1.7 and $virtualenv_version < 20:
    $system_pkgs_flag = "--system-site-packages"

  • $systempkgs = false and $virtualenv_version > 1.7 and $virtualenv_version < 20:
    $system_pkgs_flag = "--no-site-packages"

  • $systempkgs = true and $virtualenv_version >= 20:
    $system_pkgs_flag = "--system-site-packages"

  • $systempkgs = false and $virtualenv_version >= 20:
    $system_pkgs_flag = ""

so on any virtualenv version we have if $systempkgs = true we can add $system_pkgs_flag = "--system-site-packages". and based on the virtualenv version we need to figure out what to set for $system_pkgs_flag.

From a quick look these seem correct, but incomplete -- there's no handling for virtualenv before 1.7. I don't think that --system-site-packages exists in the earlier versions.

I am not sure if we still need to check on the case if $virtualenv_version < 1.7. On any OSes we are using the virtualenv_version is > 1.7.

I would expect that support is required unless the module explicitly declares that it doesn't support the version. (as a user, I'd also hope that it would warn clearly about that somewhere, perhaps at runtime when trying to use an incompatible virtualenv).

@crazymind1337
Copy link
Member

From virtualenv --help:

  --no-site-packages    DEPRECATED. Retained only for backward compatibility.
                        Not having access to global site-packages is now the
                        default behavior.

@crazymind1337
Copy link
Member

crazymind1337 commented Nov 9, 2020

virtualenv in version 1.7 has been release 9 years ago. I don't find anything if it is already EOL but I think we are safe to drop its support.

https://pypi.org/project/virtualenv/1.7/

I have checked all supported OS (from metadata.json). All have virtualenv => 15 for their default repository available.

@crazymind1337
Copy link
Member

crazymind1337 commented Nov 9, 2020

I think we are safe to just add

  $system_pkgs_flag = $systempkgs ? {
    true    => '--system-site-packages',
    default => '',
  }

@bastelfreak
Copy link
Member

hey people. I created #589 . can you both please take a look? I'm also open for suggestions to properly test if this is working

@bastelfreak
Copy link
Member

hey people. Since #593 got merged, this issue can be closed?

@PeterJCLaw
Copy link
Author

Yup, that PR has fixed it for me also.

@PeterJCLaw PeterJCLaw closed this Dec 19, 2020
@PeterJCLaw PeterJCLaw deleted the virtualenv-20-no-site-packages branch December 19, 2020 14:33
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.

3 participants