From 19f3f3f6af50f3bc5366e5a5e3833288ea24a992 Mon Sep 17 00:00:00 2001 From: Matt Low Date: Sun, 9 Jun 2019 17:02:43 +0400 Subject: [PATCH 1/3] Fix python::pip installing $editable VCS packages every puppet run This is due to VCS packages appearing in `pip freeze --all` as: -e git://example.com/package.git@#egg= Updates $grep_regex, which checks whether a given version is installed, to match against the two different output formats of `pip list`. No longer relies on pip freeze. Additionally, this commit: - Removes the multiple exec resources which were mostly the same and replaces with variables $pip_exec_name, $command, and $unless_command, which get used in a single exec resource at the end of the manifest. - Checks if $pkgname contains ==, and if so splits based on == and sets $real_pkgname to the first part, and $_ensure to the second part. Uses of $pkgname have been replaced with $real_pkgname. Fixes #193 --- manifests/pip.pp | 130 ++++++++++++++++++----------------------------- 1 file changed, 49 insertions(+), 81 deletions(-) diff --git a/manifests/pip.pp b/manifests/pip.pp index 06abce8d..0940f18e 100644 --- a/manifests/pip.pp +++ b/manifests/pip.pp @@ -135,14 +135,23 @@ fail('python::pip cannot provide uninstall_args with ensure => present') } + if $pkgname =~ /==/ { + $parts = split($pkgname, '==') + $real_pkgname = $parts[0] + $_ensure = $ensure ? { + 'absent' => 'absent', + default => $parts[1], + } + } else { + $real_pkgname = $pkgname + $_ensure = $ensure + } + # Check if searching by explicit version. - if $ensure =~ /^((19|20)[0-9][0-9]-(0[1-9]|1[1-2])-([0-2][1-9]|3[0-1])|[0-9]+\.\w+\+?\w*(\.\w+)*)$/ { - $grep_regex = "^${pkgname}==${ensure}\$" + if $_ensure =~ /^((19|20)[0-9][0-9]-(0[1-9]|1[1-2])-([0-2][1-9]|3[0-1])|[0-9]+\.\w+\+?\w*(\.\w+)*)$/ { + $grep_regex = "^${real_pkgname}[[:space:]]\\+(\\?${_ensure}\\()$\\|$\\|, \\|[[:space:]]\\)" } else { - $grep_regex = $pkgname ? { - /==/ => "^${pkgname}\$", - default => "^${pkgname}==", - } + $grep_regex = "^${real_pkgname}[[:space:]].*$" } $extras_string = empty($extras) ? { @@ -151,12 +160,12 @@ } $egg_name = $egg ? { - false => "${pkgname}${extras_string}", + false => "${real_pkgname}${extras_string}", default => $egg } $source = $url ? { - false => "${pkgname}${extras_string}", + false => "${real_pkgname}${extras_string}", /^(\/|[a-zA-Z]\:)/ => "'${url}'", /^(git\+|hg\+|bzr\+|svn\+)(http|https|ssh|svn|sftp|ftp|lp|git)(:\/\/).+$/ => "'${url}'", default => "'${url}#egg=${egg_name}'", @@ -165,111 +174,70 @@ $pip_install = "${pip_env} --log ${log}/pip.log install" $pip_common_args = "${pypi_index} ${proxy_flag} ${install_args} ${install_editable} ${source}" + $pip_exec_name = "pip_install_${name}" + # Explicit version out of VCS when PIP supported URL is provided if $source =~ /^'(git\+|hg\+|bzr\+|svn\+)(http|https|ssh|svn|sftp|ftp|lp|git)(:\/\/).+'$/ { - if $ensure != present and $ensure != latest { - exec { "pip_install_${name}": - command => "${pip_install} ${install_args} ${pip_common_args}@${ensure}#egg=${egg_name}", - unless => "${pip_env} freeze --all | grep -i -e ${grep_regex}", - user => $owner, - group => $group, - umask => $umask, - cwd => $cwd, - environment => $environment, - timeout => $timeout, - path => $_path, - } + if $_ensure != present and $_ensure != latest { + $command = "${pip_install} ${install_args} ${pip_common_args}@${_ensure}#egg=${egg_name}" + $unless_command = "${pip_env} list | grep -i -e '${grep_regex}'" } else { - exec { "pip_install_${name}": - command => "${pip_install} ${install_args} ${pip_common_args}", - unless => "${pip_env} freeze --all | grep -i -e ${grep_regex}", - user => $owner, - group => $group, - umask => $umask, - cwd => $cwd, - environment => $environment, - timeout => $timeout, - path => $_path, - } + $command = "${pip_install} ${install_args} ${pip_common_args}" + $unless_command = "${pip_env} list | grep -i -e '${grep_regex}'" } } else { - case $ensure { + case $_ensure { /^((19|20)[0-9][0-9]-(0[1-9]|1[1-2])-([0-2][1-9]|3[0-1])|[0-9]+\.\w+\+?\w*(\.\w+)*)$/: { # Version formats as per http://guide.python-distribute.org/specification.html#standard-versioning-schemes # Explicit version. - exec { "pip_install_${name}": - command => "${pip_install} ${install_args} ${pip_common_args}==${ensure}", - unless => "${pip_env} freeze --all | grep -i -e ${grep_regex} || ${pip_env} list | sed -e 's/[ ]\\+/==/' -e 's/[()]//g' | grep -i -e ${grep_regex}", - user => $owner, - group => $group, - umask => $umask, - cwd => $cwd, - environment => $environment, - timeout => $timeout, - path => $_path, - } + $command = "${pip_install} ${install_args} ${pip_common_args}==${_ensure}" + $unless_command = "${pip_env} list | grep -i -e '${grep_regex}'" } -# + 'present': { # Whatever version is available. - exec { "pip_install_${name}": - command => "${pip_install} ${pip_common_args}", - unless => "${pip_env} freeze --all | grep -i -e ${grep_regex} || ${pip_env} list | sed -e 's/[ ]\\+/==/' -e 's/[()]//g' | grep -i -e ${grep_regex}", - user => $owner, - group => $group, - umask => $umask, - cwd => $cwd, - environment => $environment, - timeout => $timeout, - path => $_path, - } + $command = "${pip_install} ${pip_common_args}" + $unless_command = "${pip_env} list | grep -i -e '${grep_regex}'" } 'latest': { # Unfortunately this is the smartest way of getting the latest available package version with pip as of now # Note: we DO need to repeat ourselves with "from version" in both grep and sed as on some systems pip returns # more than one line with paretheses. - $latest_version = join(["${pip_install} ${pypi_index} ${proxy_flag} ${install_args} ${install_editable} ${pkgname}==notreallyaversion 2>&1", + $latest_version = join(["${pip_install} ${pypi_index} ${proxy_flag} ${install_args} ${install_editable} ${real_pkgname}==notreallyaversion 2>&1", ' | grep -oP "\(from versions: .*\)" | sed -E "s/\(from versions: (.*?, )*(.*)\)/\2/g"', ' | tr -d "[:space:]"']) # Packages with underscores in their names are listed with dashes in their place in `pip freeze` output - $pkgname_with_dashes = regsubst($pkgname, '_', '-', 'G') + $pkgname_with_dashes = regsubst($real_pkgname, '_', '-', 'G') $grep_regex_pkgname_with_dashes = "^${pkgname_with_dashes}==" $installed_version = join(["${pip_env} freeze --all", " | grep -i -e ${grep_regex_pkgname_with_dashes} | cut -d= -f3", " | tr -d '[:space:]'"]) + $command = "${pip_install} --upgrade ${pip_common_args}" $unless_command = "[ \$(${latest_version}) = \$(${installed_version}) ]" - - # Latest version. - exec { "pip_install_${name}": - command => "${pip_install} --upgrade ${pip_common_args}", - unless => $unless_command, - user => $owner, - group => $group, - umask => $umask, - cwd => $cwd, - environment => $environment, - timeout => $timeout, - path => $_path, - } } default: { # Anti-action, uninstall. - exec { "pip_uninstall_${name}": - command => "echo y | ${pip_env} uninstall ${uninstall_args} ${proxy_flag} ${name}", - onlyif => "${pip_env} freeze --all | grep -i -e ${grep_regex}", - user => $owner, - group => $group, - umask => $umask, - cwd => $cwd, - environment => $environment, - timeout => $timeout, - path => $_path, - } + $pip_exec_name = "pip_uninstall_${name}" + $command = "echo y | ${pip_env} uninstall ${uninstall_args} ${proxy_flag} ${name}" + $unless_command = "! ${pip_env} list | grep -i -e '${grep_regex}'" } } } + + exec { $pip_exec_name: + command => $command, + unless => $unless_command, + user => $owner, + group => $group, + umask => $umask, + cwd => $cwd, + environment => $environment, + timeout => $timeout, + path => $_path, + } + } From 8372013b7684db742b479f6daaac53d171668af0 Mon Sep 17 00:00:00 2001 From: Matt Low Date: Sun, 9 Jun 2019 23:25:50 +0400 Subject: [PATCH 2/3] Acceptance test: works with editable=>true Improve formatting of existing tests --- spec/acceptance/virtualenv_spec.rb | 53 +++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 16 deletions(-) diff --git a/spec/acceptance/virtualenv_spec.rb b/spec/acceptance/virtualenv_spec.rb index 4e4e4529..5bbb6dbd 100644 --- a/spec/acceptance/virtualenv_spec.rb +++ b/spec/acceptance/virtualenv_spec.rb @@ -10,16 +10,14 @@ class { 'python' : pip => 'present', virtualenv => 'present', } - -> - python::virtualenv { 'venv' : + -> python::virtualenv { 'venv' : ensure => 'present', systempkgs => false, venv_dir => '/opt/venv', owner => 'root', group => 'root', } - -> - python::pip { 'rpyc' : + -> python::pip { 'rpyc' : ensure => '3.2.3', virtualenv => '/opt/venv', } @@ -36,16 +34,14 @@ class { 'python' : pip => 'present', virtualenv => 'present', } - -> - python::virtualenv { 'venv' : + -> python::virtualenv { 'venv' : ensure => 'present', systempkgs => false, venv_dir => '/opt/venv2', owner => 'root', group => 'root', } - -> - python::pip { 'pip' : + -> python::pip { 'pip' : ensure => '18.0', virtualenv => '/opt/venv2', } @@ -62,16 +58,14 @@ class { 'python' : pip => 'present', virtualenv => 'present', } - -> - python::virtualenv { 'venv' : + -> python::virtualenv { 'venv' : ensure => 'present', systempkgs => false, venv_dir => '/opt/venv3', owner => 'root', group => 'root', } - -> - python::pip { 'rpyc' : + -> python::pip { 'rpyc' : ensure => 'latest', virtualenv => '/opt/venv3', } @@ -90,16 +84,14 @@ class { 'python' : pip => 'present', virtualenv => 'present', } - -> - python::virtualenv { 'venv' : + -> python::virtualenv { 'venv' : ensure => 'present', systempkgs => false, venv_dir => '/opt/venv4', owner => 'root', group => 'root', } - -> - python::pip { 'Randomized_Requests' : + -> python::pip { 'Randomized_Requests' : ensure => 'latest', virtualenv => '/opt/venv4', } @@ -111,5 +103,34 @@ class { 'python' : # but probability of this happening is minimal, so it should be acceptable. apply_manifest(pp, catch_changes: true) end + it 'works with editable=>true' do + pp = <<-EOS + package{ 'git' : + ensure => 'present', + } + -> class { 'python' : + version => 'system', + pip => 'present', + virtualenv => 'present', + } + -> python::virtualenv { 'venv' : + ensure => 'present', + systempkgs => false, + venv_dir => '/opt/venv5', + owner => 'root', + group => 'root', + } + -> python::pip { 'rpyc' : + ensure => '4.1.0', + url => 'git+https://github.com/tomerfiliba/rpyc.git', + editable => true, + virtualenv => '/opt/venv5', + } + EOS + + # Run it twice and test for idempotency + apply_manifest(pp, catch_failures: true) + apply_manifest(pp, catch_changes: true) + end end end From 96770aaa5bd899d74c371b0140b8e67fa78e83a2 Mon Sep 17 00:00:00 2001 From: Matt Low Date: Mon, 10 Jun 2019 01:05:50 +0400 Subject: [PATCH 3/3] Acceptance test: works with == in pkgname --- spec/acceptance/virtualenv_spec.rb | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/spec/acceptance/virtualenv_spec.rb b/spec/acceptance/virtualenv_spec.rb index 5bbb6dbd..7c3fba0d 100644 --- a/spec/acceptance/virtualenv_spec.rb +++ b/spec/acceptance/virtualenv_spec.rb @@ -27,6 +27,7 @@ class { 'python' : apply_manifest(pp, catch_failures: true) apply_manifest(pp, catch_changes: true) end + it 'maintains pip version' do pp = <<-EOS class { 'python' : @@ -51,6 +52,7 @@ class { 'python' : apply_manifest(pp, catch_failures: true) apply_manifest(pp, catch_changes: true) end + it 'works with ensure=>latest' do pp = <<-EOS class { 'python' : @@ -77,6 +79,7 @@ class { 'python' : # but probability of this happening is minimal, so it should be acceptable. apply_manifest(pp, catch_changes: true) end + it 'works with ensure=>latest for package with underscore in its name' do pp = <<-EOS class { 'python' : @@ -103,6 +106,7 @@ class { 'python' : # but probability of this happening is minimal, so it should be acceptable. apply_manifest(pp, catch_changes: true) end + it 'works with editable=>true' do pp = <<-EOS package{ 'git' : @@ -132,5 +136,29 @@ class { 'python' : apply_manifest(pp, catch_failures: true) apply_manifest(pp, catch_changes: true) end + + it 'works with == in pkgname' do + pp = <<-EOS + class { 'python' : + version => 'system', + pip => 'present', + virtualenv => 'present', + } + -> python::virtualenv { 'venv' : + ensure => 'present', + systempkgs => false, + venv_dir => '/opt/venv6', + owner => 'root', + group => 'root', + } + -> python::pip { 'rpyc==4.1.0' : + virtualenv => '/opt/venv6', + } + EOS + + # Run it twice and test for idempotency + apply_manifest(pp, catch_failures: true) + apply_manifest(pp, catch_changes: true) + end end end