fix virtualenv deprecated "--no-site-packages" argument

[saz]

--no-site-packages argument isn't available in all versions of virtualenv > 20.0.1

[thebigb]

Why not add another elsif a few lines up?

    } elsif (( versioncmp($_virtualenv_version,'20.0.1') > 0 ) and ( $systempkgs == false )) {
      $system_pkgs_flag = ''

Tbh that logic structure isn't too clean either, but at least it's consistently dirty.

[igalic]

i think i agree with @thebigb on this one
this case is pretty unclean in the else, so why not avoid it altogether

[thebigb]

Ah there's actually a subtle difference in logic here. When the virtualenv_version is not yet populated (e.g. the package has been just installed in the current run), @saz's version will default to an empty $system_pkgs_flag, and my version will default to --no-site-packages.

I don't know enough about older versions virtualenv. If between 1.7 and 20.0.1 the --no-site-packages option was redundant anyway, the logic proposed by this PR is favorable. Otherwise this breaks the first run for everyone installing versions < 20.0.1.

[thebigb]

So depending on what's desirable one of these should do the trick:

    if $_virtualenv_version != '' and versioncmp($_virtualenv_version, '1.7') > 0 and $systempkgs == true {
      $system_pkgs_flag = '--system-site-packages'
    } elsif $_virtualenv_version != '' and versioncmp($_virtualenv_version, '20.0.1') < 0 and $systempkgs == false {
      $system_pkgs_flag = '--no-site-packages'
    } else {
      $system_pkgs_flag = $systempkgs ? {
        true    => '--system-site-packages',
        false   => '',
        default => fail('Invalid value for systempkgs. Boolean value is expected')
      }
    }

    if $_virtualenv_version != '' and versioncmp($_virtualenv_version, '1.7') > 0 and $systempkgs == true {
      $system_pkgs_flag = '--system-site-packages'
    } elsif $_virtualenv_version != '' and versioncmp($_virtualenv_version, '1.7') < 0 and $systempkgs == false {
      $system_pkgs_flag = '--no-site-packages'
    } elsif $_virtualenv_version != '' and versioncmp($_virtualenv_version, '20.0.1') >= 0 and $systempkgs == false {
      $system_pkgs_flag = ''
    } else {
      $system_pkgs_flag = $systempkgs ? {
        true    => '--system-site-packages',
        false   => '--no-site-packages',
        default => fail('Invalid value for systempkgs. Boolean value is expected')
      }
    }

[thebigb]

Found a bug in the examples of my previous comment. For some reason I thought that it would jump to the else block if the virtualenv_version is empty, but that's obviously not the case. I edited the comment and the logic should work now.

[saz]

I don't know enough about older versions virtualenv. If between 1.7 and 20.0.1 the --no-site-packages option was redundant anyway, the logic proposed by this PR is favorable. Otherwise this breaks the first run for everyone installing versions < 20.0.1.
Looking at https://virtualenv.pypa.io/en/legacy/changes.html#v1-7-2011-11-30 it's the case starting from 1.7

[igalic]

if we added type annotations, this default would be unnecessary

the question, of course would remain whether puppet lint would know that…… (that the case is already exhaustive)

[igalic]

i think i agree with @thebigb on this one
this case is pretty unclean in the else, so why not avoid it altogether

[saz]

Type annotations would indeed help here a bit and I was thinking about it.

Is there a plan on adding them? Maybe this part can be marked as "needs some work" after annotations are there.

I'm also curious on how many users are still using virtualenv < 1.7...

[igalic]

I'm also curious on how many users are still using virtualenv < 1.7...

file this under "things i don't wanna know", like, how many people still use CentOS / RHEL 5

[saz]

@thebigb @igalic Does it makes sense to still support virtualenv < 1.7? 1.7 has been released in 2011... Dropping support will make this waaay easier...

[igalic]

i think that mostly depends on what the oldest operating system is that ships it, before we get to update it

[thebigb]

Personally not a big fan of potentially breaking changes unless it's a major release. That said, I doubt that users that are running older versions do keep this puppet module up to date.

I had a look at the changelog you found, and I guess this solution would suffice:

https://github.com/voxpupuli/puppet-python/compare/master...thebigb:master?diff=unified

[saz]

i think that mostly depends on what the oldest operating system is that ships it, before we get to update it

That's the hardest part to find, as not all OSs have a version stated somewhere related to virtualenv.
The usual suspects (e.g. RHEL) seem to have a newer version included (CentOS 5 isn't in the list of supported OSs 😄)

@thebigb Okay, let's use this one. Should I update this PR or how should we proceed?

[thebigb]

Go ahead. Did you test it? I tested it on my local setup, but I might've overlooked something.

[crazymind1337]

@igalic @thebigb @saz how do we process here? I would like to have this released soon, since we require python virtualenv support on 20.04.

[crazymind1337]

@saz you might need to rebase your branch.

[crazymind1337]

@saz @thebigb can we have this release soon please?

[PeterJCLaw]

Looks like a variant of this was included in #593; is this sorted now?

[saz]

Looks good to me. Closing this.