From 82f64126d9a1105da17f37386e1f40fcd3e97ea6 Mon Sep 17 00:00:00 2001 From: Josh Soref Date: Thu, 2 May 2019 12:46:59 -0400 Subject: [PATCH 1/6] Grammar (#7013) * spelling: these * grammar: either-or * spelling: e.g. --- certbot/compat/misc.py | 4 ++-- certbot/compat/os.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/certbot/compat/misc.py b/certbot/compat/misc.py index 2f4ba0c6b..4f0e22078 100644 --- a/certbot/compat/misc.py +++ b/certbot/compat/misc.py @@ -31,7 +31,7 @@ def raise_for_non_administrative_windows_rights(subcommand): # Because windll exists only on a Windows runtime, and static code analysis engines # do not like at all non existent objects when run from Linux (even if we handle properly # all the cases in the code). - # So we access windll only by reflection to trick theses engines. + # So we access windll only by reflection to trick these engines. if hasattr(ctypes, 'windll') and subcommand not in UNPRIVILEGED_SUBCOMMANDS_ALLOWED: windll = getattr(ctypes, 'windll') if windll.shell32.IsUserAnAdmin() == 0: @@ -73,7 +73,7 @@ def os_rename(src, dst): raise if not hasattr(os, 'replace'): # pragma: no cover # We should never go on this line. Either we are on Linux and os.rename has succeeded, - # either we are on Windows, and only Python >= 3.4 is supported where os.replace is + # or we are on Windows, and only Python >= 3.4 is supported where os.replace is # available. raise RuntimeError('Error: tried to run os_rename on Python < 3.3. ' 'Certbot supports only Python 3.4 >= on Windows.') diff --git a/certbot/compat/os.py b/certbot/compat/os.py index 0112fbc73..8a139a141 100644 --- a/certbot/compat/os.py +++ b/certbot/compat/os.py @@ -1,6 +1,6 @@ """ This compat modules is a wrapper of the core os module that forbids usage of specific operations -(eg. chown, chmod, getuid) that would be harmful to the Windows file security model of Certbot. +(e.g. chown, chmod, getuid) that would be harmful to the Windows file security model of Certbot. This module is intended to replace standard os module throughout certbot projects (except acme). """ from __future__ import absolute_import From 862577fffc5ea6b29a9bb519d7b850b454d23e84 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 2 May 2019 11:32:49 -0700 Subject: [PATCH 2/6] Bump initial version to 0.33.1. (#7017) We made this change locally yesterday while preparing the release. I tested this change on all AMIs currently in the test farm as well as Fedora 29 and this test passed on all instances. --- tests/letstest/scripts/test_leauto_upgrades.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/letstest/scripts/test_leauto_upgrades.sh b/tests/letstest/scripts/test_leauto_upgrades.sh index d565aa268..d5133ba38 100755 --- a/tests/letstest/scripts/test_leauto_upgrades.sh +++ b/tests/letstest/scripts/test_leauto_upgrades.sh @@ -15,8 +15,8 @@ if ! command -v git ; then exit 1 fi fi -# 0.18.0 is the oldest version of letsencrypt-auto that works on Fedora 26+. -INITIAL_VERSION="0.18.0" +# 0.33.x is the oldest version of letsencrypt-auto that works on Fedora 29+. +INITIAL_VERSION="0.33.1" git checkout -f "v$INITIAL_VERSION" letsencrypt-auto if ! ./letsencrypt-auto -v --debug --version --no-self-upgrade 2>&1 | tail -n1 | grep "^certbot $INITIAL_VERSION$" ; then echo initial installation appeared to fail From e15e848474a5c180b2f22cd059c44905a11568ef Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 2 May 2019 11:36:47 -0700 Subject: [PATCH 3/6] Stop certbot-auto from printing blank lines (#7016) Fixes #7012. Apparently, the previous test we had here doesn't catch the case when certbot-auto prints blank lines. (I don't yet understand why so if someone does, please let me know!) Regardless, I fixed up the test and verified it fails with the version of letsencrypt-auto in master and then fixed letsencrypt-auto so the test passes. I ran test farm tests on the changes here and they passed on all instances. * correct test * fixes #7012 --- letsencrypt-auto-source/letsencrypt-auto | 8 +++++--- letsencrypt-auto-source/letsencrypt-auto.template | 8 +++++--- .../scripts/test_letsencrypt_auto_certonly_standalone.sh | 4 ++-- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/letsencrypt-auto-source/letsencrypt-auto b/letsencrypt-auto-source/letsencrypt-auto index 4e1503715..f9f02da62 100755 --- a/letsencrypt-auto-source/letsencrypt-auto +++ b/letsencrypt-auto-source/letsencrypt-auto @@ -1593,12 +1593,14 @@ UNLIKELY_EOF # --------------------------------------------------------------------------- # If the script fails for some reason, don't break certbot-auto. set +e - # Suppress unexpected error output and only print the script's output if it - # ran successfully. + # Suppress unexpected error output. CHECK_PERM_OUT=$("$LE_PYTHON" "$TEMP_DIR/check_permissions.py" "$0" 2>/dev/null) CHECK_PERM_STATUS="$?" set -e - if [ "$CHECK_PERM_STATUS" = 0 ]; then + # Only print output if the script ran successfully and it actually produced + # output. The latter check resolves + # https://github.com/certbot/certbot/issues/7012. + if [ "$CHECK_PERM_STATUS" = 0 -a -n "$CHECK_PERM_OUT" ]; then error "$CHECK_PERM_OUT" fi fi diff --git a/letsencrypt-auto-source/letsencrypt-auto.template b/letsencrypt-auto-source/letsencrypt-auto.template index 21db0f908..bff4173d4 100755 --- a/letsencrypt-auto-source/letsencrypt-auto.template +++ b/letsencrypt-auto-source/letsencrypt-auto.template @@ -670,12 +670,14 @@ UNLIKELY_EOF # --------------------------------------------------------------------------- # If the script fails for some reason, don't break certbot-auto. set +e - # Suppress unexpected error output and only print the script's output if it - # ran successfully. + # Suppress unexpected error output. CHECK_PERM_OUT=$("$LE_PYTHON" "$TEMP_DIR/check_permissions.py" "$0" 2>/dev/null) CHECK_PERM_STATUS="$?" set -e - if [ "$CHECK_PERM_STATUS" = 0 ]; then + # Only print output if the script ran successfully and it actually produced + # output. The latter check resolves + # https://github.com/certbot/certbot/issues/7012. + if [ "$CHECK_PERM_STATUS" = 0 -a -n "$CHECK_PERM_OUT" ]; then error "$CHECK_PERM_OUT" fi fi diff --git a/tests/letstest/scripts/test_letsencrypt_auto_certonly_standalone.sh b/tests/letstest/scripts/test_letsencrypt_auto_certonly_standalone.sh index 035512ef7..0973bbc03 100755 --- a/tests/letstest/scripts/test_letsencrypt_auto_certonly_standalone.sh +++ b/tests/letstest/scripts/test_letsencrypt_auto_certonly_standalone.sh @@ -42,8 +42,8 @@ if ! letsencrypt-auto --help --no-self-upgrade | grep -F "letsencrypt-auto [SUBC exit 1 fi -OUTPUT=$(letsencrypt-auto --install-only --no-self-upgrade --quiet 2>&1) -if [ -n "$OUTPUT" ]; then +OUTPUT_LEN=$(letsencrypt-auto --install-only --no-self-upgrade --quiet 2>&1 | wc -c) +if [ "$OUTPUT_LEN" != 0 ]; then echo letsencrypt-auto produced unexpected output! exit 1 fi From b19d4801c9dea2898402c5b388da4bd10b103d01 Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Thu, 2 May 2019 23:32:02 +0200 Subject: [PATCH 4/6] Fix oldest tests when local dependencies are used (#7019) Fixes #7014. Using a --force-reinstall (only for oldest tests), dependencies are properly reinstalled. Since this action significantly increases the execution time of oldest tests, I split them into two parts to allow their parallel execution by Travis. We will need to find a better way to solve this in the future. An example of successful execution of oldest tests in the situation of a point release can be found here: https://travis-ci.org/adferrand/certbot/builds/527475532 * Fix for oldest requirements * Split oldest tests * Update a comment --- .travis.yml | 7 ++++++- tools/pip_install.py | 11 +++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index b23164a19..e1859cc0e 100644 --- a/.travis.yml +++ b/.travis.yml @@ -60,7 +60,12 @@ matrix: env: TOXENV=mypy <<: *not-on-master - python: "2.7" - env: TOXENV='py27-{acme,apache,certbot,dns,nginx,postfix}-oldest' + env: TOXENV='py27-{acme,apache,certbot,nginx,postfix}-oldest' + sudo: required + services: docker + <<: *not-on-master + - python: "2.7" + env: TOXENV='py27-dns-oldest' sudo: required services: docker <<: *not-on-master diff --git a/tools/pip_install.py b/tools/pip_install.py index 68268e298..abc7baa91 100755 --- a/tools/pip_install.py +++ b/tools/pip_install.py @@ -99,8 +99,15 @@ def main(args): else: # Otherwise, we merge requirements to build the constraints and pin dependencies requirements = None + reinstall = False if os.environ.get('CERTBOT_OLDEST') == '1': requirements = certbot_oldest_processing(tools_path, args, test_constraints) + # We need to --force-reinstall the tested distribution when using oldest + # requirements because of an error in these tests in particular situations + # described in https://github.com/certbot/certbot/issues/7014. + # However this slows down considerably the oldest tests (5 min -> 10 min), + # so we need to find a better mitigation in the future. + reinstall = True else: certbot_normal_processing(tools_path, test_constraints) @@ -109,8 +116,8 @@ def main(args): pip_install_with_print('--constraint "{0}" --requirement "{1}"' .format(all_constraints, requirements)) - pip_install_with_print('--constraint "{0}" {1}' - .format(all_constraints, ' '.join(args))) + pip_install_with_print('--constraint "{0}" {1} {2}'.format( + all_constraints, '--force-reinstall' if reinstall else '', ' '.join(args))) finally: if os.environ.get('TRAVIS'): print('travis_fold:end:install_certbot_deps') From 4bf6eb2091e3190282b0e2c6540186e64bf4d846 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 2 May 2019 14:52:36 -0700 Subject: [PATCH 5/6] Update changelog for 0.34.1. (#7021) --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 77351c84b..84f28cea7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). ### Fixed -* +* certbot-auto no longer prints a blank line when there are no permissions + problems. Despite us having broken lockstep, we are continuing to release new versions of all Certbot components during releases for the time being, however, the only From 6a970f74d0fe13c739a105e20dc8892c49b677b1 Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Fri, 3 May 2019 00:17:11 +0200 Subject: [PATCH 6/6] Try another approach (#7022) In #7019, a solution has been integrated to fix oldest tests execution in the corner cases described in #7014. However this solution was not very satisfactory, as it consists in making a --force-reinstall for all requirements on each oldest tests (apache, certbot, acme, each dns plugin ...). As a consequence, the overall execution time of these tests increased from 5 min to 10 min. In this PR I propose a more elegant solution: instead of reinstalling all dependencies, we force reinstall only the requirements themselves describe in the relevant oldest-requirements.txt files. This way only the packages that are potentially ignored by pip because they exists locally (acme, certbot, ...) are reinstalled. The result is the same than in #7019 (we are sure that all packages are really installed by pip), but the very limited number of force reinstalled package here make the impact on execution time negligible. As a consequence, I revert back also the tox environments to execute all oldest tests together. A successful execution of oldest tests using this PR material in the context of a point release can be seen here: https://travis-ci.org/adferrand/certbot/builds/527513101 --- .travis.yml | 7 +------ tools/pip_install.py | 20 ++++++++++---------- 2 files changed, 11 insertions(+), 16 deletions(-) diff --git a/.travis.yml b/.travis.yml index e1859cc0e..b23164a19 100644 --- a/.travis.yml +++ b/.travis.yml @@ -60,12 +60,7 @@ matrix: env: TOXENV=mypy <<: *not-on-master - python: "2.7" - env: TOXENV='py27-{acme,apache,certbot,nginx,postfix}-oldest' - sudo: required - services: docker - <<: *not-on-master - - python: "2.7" - env: TOXENV='py27-dns-oldest' + env: TOXENV='py27-{acme,apache,certbot,dns,nginx,postfix}-oldest' sudo: required services: docker <<: *not-on-master diff --git a/tools/pip_install.py b/tools/pip_install.py index abc7baa91..cf0a7aee5 100755 --- a/tools/pip_install.py +++ b/tools/pip_install.py @@ -99,25 +99,25 @@ def main(args): else: # Otherwise, we merge requirements to build the constraints and pin dependencies requirements = None - reinstall = False if os.environ.get('CERTBOT_OLDEST') == '1': requirements = certbot_oldest_processing(tools_path, args, test_constraints) - # We need to --force-reinstall the tested distribution when using oldest - # requirements because of an error in these tests in particular situations - # described in https://github.com/certbot/certbot/issues/7014. - # However this slows down considerably the oldest tests (5 min -> 10 min), - # so we need to find a better mitigation in the future. - reinstall = True else: certbot_normal_processing(tools_path, test_constraints) merge_requirements(tools_path, requirements, test_constraints, all_constraints) - if requirements: + if requirements: # This branch is executed during the oldest tests + # First step, install the transitive dependencies of oldest requirements + # in respect with oldest constraints. pip_install_with_print('--constraint "{0}" --requirement "{1}"' .format(all_constraints, requirements)) + # Second step, ensure that oldest requirements themselves are effectively + # installed using --force-reinstall, and avoid corner cases like the one described + # in https://github.com/certbot/certbot/issues/7014. + pip_install_with_print('--force-reinstall --no-deps --requirement "{0}"' + .format(requirements)) - pip_install_with_print('--constraint "{0}" {1} {2}'.format( - all_constraints, '--force-reinstall' if reinstall else '', ' '.join(args))) + pip_install_with_print('--constraint "{0}" {1}'.format( + all_constraints, ' '.join(args))) finally: if os.environ.get('TRAVIS'): print('travis_fold:end:install_certbot_deps')