Make letsencrypt-auto release-suitable, self-upgrading, and self-contained. #1665

Merged
merged 112 commits into from Jan 19, 2016

Projects

None yet

4 participants

@erikrose
Contributor
erikrose commented Dec 2, 2015

Fix #1572.

Here's the comment that describes the design. Here's what remains to be done:

  • Gather requirement hashes.
  • Make le-auto not upgrade letsencrypt unless necessary.
  • Make more easily testable.
  • Test.
  • Review.
  • Add building and signing le-auto to release process.
  • Make peep compatible with pip 7.
  • Add an option that skips the self-upgrade, for people who have audited this and want to run only this version.
  • Figure out under what conditions to install dependencies needed by only Python 2.6 (e.g. https://github.com/letsencrypt/letsencrypt/pull/1914/files).
erikrose added some commits Nov 30, 2015
@erikrose erikrose WIP. Here's a letsencrypt-auto script that downloads a new copy of it…
…self, checks a signature on it, and replaces itself with it.
8ba831a
@erikrose erikrose Add a sig to test against. ec415b2
@erikrose erikrose This works now, to the point where it calls the downloaded version of…
… le_auto.
1a8f40e
@erikrose erikrose Compute latest stable version of letsencrypt properly.
PyPI does not appear to give it to us for free through its JSON interface. distutils gives us a sufficient (though not foolproof) comparator without having to go outside the stdlib.
a75c743
@erikrose erikrose Stop catching exception types that are no longer thrown by get(). 602e977
@erikrose erikrose Find a better semantic for HumanException.
These are the exceptions that are likely to happen, so we give them extra, human-readable descriptions.

Also, name them more in line with stdlib exceptions.
7fb9295
@erikrose erikrose Return a temp dir, not the file within.
This lets us reuse the dir for other things and makes it easy to rm afterward.
2c36f59
@erikrose erikrose Add peep and sample requirements file.
We cat it to a file rather than just calling it in place because otherwise the "-" arg would have to be stripped off by editing the script.
86203c8
@erikrose erikrose Unquote heredoc terminators.
Quoting them causes them to not be recognized sometimes. (Perhaps it's when they're not within backticks?)
fe77da2
@erikrose erikrose Split large independent scripts off from the main body of the proof-o…
…f-concept script. Integrate the bits of the old le-auto script that are still useful.

This makes the script more readable and easier to work on. We'll stitch it together with a build process.

Also, stop passing the sudo command as an arg to the experimental bootstrappers. They will be inlined into the main script and can just reference $SUDO. As a result, stop recommending devs run the scripts manually, instead running le-auto --os-packages-only. This has the nice side effect of making dev documentation simpler.

Name the folder "letsencrypt_auto" rather than "letsencrypt-auto" because git yield endless pain when replacing a file with a dir. Perhaps we can change it with impunity in a latter commit.
e3ace6f
@erikrose erikrose Merge master in to get up to date. 4abe7ab
@erikrose erikrose Move OS-package bootstrappers to a private folder.
They're now used only by the le-auto build process. The new public interface for OS-level bootstrapping is le-auto --os-packages-only, which dispatches by OS automatically. That obsoletes install-deps.sh as well, saving some repetition.

Also, switch to mustache-style templating to avoid colliding with shell variable references.

To optimize for the docker cache, we could later add a shim script that sources just deb_common.sh and calls its bootstrap function.
ec9a498
@erikrose erikrose Add build script for letsencrypt-auto.
Change template language to reference files, saving me some boilerplate over the dict-based .format() thing I originally had in mind.

Put newlines at the ends of bootstrap scripts. It makes the built le-auto script prettier.
cdd855c
@erikrose erikrose le-auto now doesn't trigger sh syntax errors when run. 66436c5
@erikrose erikrose Remove test signature, which I shouldn't have committed. f9d1de6
@erikrose erikrose Fix some errors.
Use the correct Python interpreter. Fix a syntax error. Fix a missing import.
9d6cbea
@pde pde and 1 other commented on an outdated diff Dec 2, 2015
letsencrypt_auto/letsencrypt-auto.template
fi
+if [ "$1" = "--os-packages-only" ]; then
@pde
pde Dec 2, 2015 Member

This flag needs to be at least accepted by the main letsencrypt Python client, and probably documented in its --help (even if it does nothing).

@pde
pde Dec 2, 2015 Member

Once that's done we can move all flag detection to the loop on line 105, so that the arguments are ordering-agnostic.

@erikrose
erikrose Dec 2, 2015 Contributor

It makes sense that the options to letsencrypt should be tolerated (passed along, really) by letsencrypt-auto, but why the other way around?

@pde pde commented on an outdated diff Dec 2, 2015
letsencrypt_auto/letsencrypt-auto.template
+ else
+ # Report error:
+ echo $TEMP_DIR
+ exit 1
+ fi
+ fi # should upgrade
+else # --_skip-to-install was passed.
+ # Install Python dependencies with peep, then run letsencrypt.
+ echo "Installing Python package dependencies..."
+ TEMP_DIR="$2"
+ shift 2
+ cat << "UNLIKELY_EOF" > $TEMP_DIR/letsencrypt-auto-requirements.txt
+{{ letsencrypt-auto-requirements.txt }}
+UNLIKELY_EOF
+
+ cat << "UNLIKELY_EOF" > $TEMP_DIR/peep.py
@pde
pde Dec 2, 2015 Member

Perhaps add a big visual separator line here, because we're about to change languages and universes :)

@pde pde commented on an outdated diff Dec 2, 2015
letsencrypt_auto/letsencrypt-auto.template
+ echo $TEMP_DIR
+ exit 1
+ fi
+ fi # should upgrade
+else # --_skip-to-install was passed.
+ # Install Python dependencies with peep, then run letsencrypt.
+ echo "Installing Python package dependencies..."
+ TEMP_DIR="$2"
+ shift 2
+ cat << "UNLIKELY_EOF" > $TEMP_DIR/letsencrypt-auto-requirements.txt
+{{ letsencrypt-auto-requirements.txt }}
+UNLIKELY_EOF
+
+ cat << "UNLIKELY_EOF" > $TEMP_DIR/peep.py
+{{ peep.py }}
+UNLIKELY_EOF
@pde
pde Dec 2, 2015 Member

And another one here as we return to shell?

@pde pde and 1 other commented on an outdated diff Dec 2, 2015
letsencrypt_auto/letsencrypt-auto.template
fi
+if [ "$1" = "--os-packages-only" ]; then
+ Bootstrap
+elif [ "$1" != "--_skip-to-install" ]; then
@pde
pde Dec 2, 2015 Member

Is this flag semantically equivalent to "do not autoupdate"? If so, we should definitely document it and let users supply it :)

@erikrose
erikrose Dec 2, 2015 Contributor

No; if someone manually passes this flag, it could cause le-auto to try installing Python packages without a virtualenv made.

@erikrose
erikrose Dec 2, 2015 Contributor

Now it is. Renamed the arg and made it public in be6c34d. Moved the venv creation bit to make it work. It should've been there anyway.

erikrose added some commits Dec 2, 2015
@erikrose erikrose Print the final letsencrypt invocation before doing it.
People like to know what they're sudo-ing.
a1b2626
@erikrose erikrose Add visual separators between language changes.
On his first time auditing, pde thought this would help.
346ec58
@erikrose erikrose Standardize semicolon use. 4a69584
@erikrose erikrose Rewrap some comments. fc52608
@erikrose erikrose Install not only LE's dependencies but LE itself. 5bae8e0
@erikrose erikrose Make --no-self-upgrade public.
This replaces --_skip-to-install and is suitable for people who have audited letsencrypt-auto and wish to run it as is, without upgrading to the latest version.

Also...
* rm temp dirs when done. No longer reuse a single temp dir across phases so the user doesn't have to pass a temp dir with --no-self-upgrade as phase 1 itself used to.
* Swap stanzas in the big "if" so we aren't testing negatives all the time.
* Fix a bug in which we ran peep with $LE_PYTHON rather than the python in the venv.
* Bootstrap only if it looks like we never got to the point of making a venv before.
* Move venv creation into Phase 2. Besides the practical benefit of ensuring there's a venv if a user passes --no-self-upgrade, this has the philosophical advantage of making Phase 1 more minimal, giving us more latitude to change behavior in updates.
be6c34d
@erikrose erikrose Upgrade peep to 2.5, for compatibility with pip 7.x. 02255fa
@erikrose
Contributor
erikrose commented Dec 3, 2015

letsencrypt-auto --no-self-upgrade --debug just worked great on my Mac, as of 02255fa.

@pde pde added the letsencrypt-auto label Dec 3, 2015
erikrose added some commits Dec 3, 2015
@erikrose erikrose In Phase 2, recreate a venv and reinstall Python packages only if nec…
…essary.

* Teach the build script how to do special vars. Factor up file reading.
* Use a static string for the PyPI JSON location, as it will soon be overrideable via an env var for testing.
46779da
@erikrose erikrose In Phase 1, download a new letsencrypt-auto script only if necessary.
* Temp dir creation is now always done in shell.
* Split download_upgrade.py into 2 phases itself so we can have it report back the latest LE version and make a decision based on it before doing and major downloading. Rename it for clarity.
5cc69d9
@erikrose erikrose "none" is clearer than "0.0.0" as a sentinel value. 55a52d1
@erikrose erikrose Put off rm-ing the venv for as long as possible, since it triggers a …
…re-bootstrap.

If DeterminePythonVersion has an error, we shouldn't re-bootstrap.
4bcd594
@erikrose erikrose Add a header for peep errors...
...since the shell's collected output is such a line-break-lacking mess.
4a44c46
@erikrose erikrose Correct length of dividers. 6db54e2
erikrose added some commits Dec 4, 2015
@erikrose erikrose Put quotes around variables that might contain spaces.
Bourne does the dumb thing when substituting vars; this helps that. Bash does the smart thing but is unhurt by this.

We don't do it to $SUDO because Bourne takes "" as a command rather than a no-op, and we don't want the SUDO= case to generate command-not-found errors.
1da5e47
@erikrose erikrose Update LE package pins to 0.1.0, the public beta. 8b2c5cb
@pde pde and 1 other commented on an outdated diff Dec 5, 2015
letsencrypt_auto/build.py
+ special_replacements = {
+ 'LE_AUTO_VERSION': le_version(dir)
+ }
+
+ def replacer(match):
+ token = match.group(1)
+ if token in special_replacements:
+ return special_replacements[token]
+ else:
+ return file_contents(join(dir, 'pieces', token))
+
+ result = re.sub(r'{{\s*([A-Za-z0-9_./-]+)\s*}}',
+ replacer,
+ file_contents(join(dir, 'letsencrypt-auto.template')))
+ with open(join(dir, 'letsencrypt-auto'), 'w') as out:
+ out.write(result)
@pde
pde Dec 5, 2015 Member

We also need to os.chmod() the resulting script to be executable.

@erikrose
erikrose Dec 10, 2015 Contributor

Good idea. (If the old one were +x, the new one would be too due to cp's behavior, but if they ran "sh letsencrypt-auto", it might not have been +x.)

@erikrose erikrose Make le-auto pull the requisite things from env vars so we can run ag…
…ainst test servers.

This should let us create a harness that won't force us to mess with GitHub or PyPI just to test.

(I haven't tried this commit yet, but you can if you want to get a head start on testing.)
0c4a7bb
@erikrose
Contributor

I have a crazy HTTPS-serving test harness starting to take shape locally (fake CA, signed certs, the whole bit), but it's not useful yet.

@pde pde added this to the 0.2.0 milestone Dec 10, 2015
@pde
Member
pde commented Dec 16, 2015

Closes: #1256

erikrose and others added some commits Dec 18, 2015
@erikrose erikrose Get the first end-to-end test of letsencrypt-auto passing.
To run it, cd letsencrypt_auto && ./build.py && docker build -t lea . && docker run --rm -t -i lea

So as not to depend on the state of the host machine, the test runs within an Ubuntu Docker image. This lets us sidestep interaction challenges by setting up passwordless sudo. It also lets us insert our own local CA for the mock HTTPS server. (openssl's SSL_CERT_FILE env var replaces rather than adds to the accepted CAs, meaning later connections to PyPI within the same process chain fail. SSL_CERT_DIR seems not to work at all on OS X.) This also demonstrates a way to test across various Linux distros, even within Travis if we like,

Also...

* Switch to an official release of ConfigArgParse.
* Don't redundantly re-bootstrap on --no-self-upgrade (that is, phase 2).
d9cde2b
@erikrose erikrose Add built letsencrypt-auto.
We're going to keep the built artifact in the tree as per certbot#1572 (comment) so that...

1. People's current behavior of cloning from git and running the le-auto script still works.
2. We don't have a deprecation timeline and process to babysit.

We'll enforce its up-to-dateness with a test.
7e04f52
@erikrose erikrose Document le-auto env vars. e6cece5
@erikrose erikrose Merge in master to get up to date.
Bootstrap scripts and letsencrypt-auto itself required some merge work.
cad4e98
@erikrose erikrose Update the built version of letsencrypt-auto. fa30625
@pde pde Create a fake release in a PyPI-style json file 5f694e3
@erikrose erikrose Substitute test-only values for the env vars.
To come: a test-only public key for letsencrypt-auto.sig.
7d182c2
@pde pde A real release signture key 726376f
@pde pde Merge branch 'letsencrypt-auto-release-testing' of ssh://github.com/l…
…etsencrypt/letsencrypt into letsencrypt-auto-release-testing
14d3d4a
@pde pde Switch to a throwaway testing key d0bbe44
@pde pde First attempt at signing with a throwaway key b2a0142
@erikrose erikrose Change version of le-auto script to the one published in the pypi.json. 9181271
@erikrose erikrose Swap _ for - so the phase-1 upgrade doesn't 404. 0f78753
@pde pde Update signature! 1ad21f9
@pde pde Survive unsuccessful apt-get update... 56bda20
@pde pde Move sudo to the top
(So that people who want to audit can see it quickly)
f4011cc
@bmw bmw Updated versions 8bb0631
@pde pde Merge branch 'letsencrypt-auto-release-testing' of ssh://github.com/l…
…etsencrypt/letsencrypt into letsencrypt-auto-release-testing
88c4260
@pde pde Resign b008770
@pde pde Try baking in this 0.1.22 thing 0a122cb
@erikrose erikrose Update le-auto requirements file to fake LE 0.1.22 release. 404de84
@erikrose erikrose Add hashes for new cffi 1.3.1 packages.
New ones for OS X 10.6 were released on 2015-12-16.
484b032
@erikrose erikrose Rebuild and re-sign le-auto. d83dda8
@erikrose erikrose Swap _ for - so the phase-1 upgrade doesn't 404. 275d3b4
@pde @erikrose pde Survive unsuccessful apt-get update... 5aa9fe9
@pde @erikrose pde Move sudo to the top
(So that people who want to audit can see it quickly)
8a3bbf9
@erikrose erikrose Add hashes for new cffi 1.3.1 packages.
New ones for OS X 10.6 were released on 2015-12-16.
4940ee2
@erikrose erikrose Update pinning of LE packages to 0.1.1.
If we keep these at the latest release, something sane should happen if someone runs le-auto from master.
ba6bf45
@erikrose erikrose Mark changes in letsencrypt-auto-release-testing as having been incor…
…porated into letsencrypt-auto-release.

We grabbed the bug fixes but left the changes specific to the infrastructure we mocked out for human testing.
4fd9d39
@erikrose erikrose Remove needless message about reusing venv. Rebuild le-auto. 762709a
@erikrose erikrose Cut down mock PyPI dir listing HTML. 4b075df
@erikrose erikrose Add le-auto tests for "no upgrade needed" and "only a phase-2 upgrade…
… needed".
98b3c41
@erikrose erikrose Don't stomp on the in-tree le-auto during tests. e5e5c2d
@erikrose erikrose Add a test for when openssl signature verification fails during phase…
…-1 upgrade.
134b7ab
@erikrose erikrose Add a test for failed hash verification during phase-2 upgrade. bb31d71
@pde
Member
pde commented Jan 8, 2016

Fixes: #2107

@erikrose
Contributor
erikrose commented Jan 8, 2016

On merging, restore the old le-auto script and the bootstrap dir to the root dir. Delete them after we're satisfied the new le-auto is working.

@erikrose erikrose Rename letsencrypt_auto dir to match other dirs.
Originally, I had it in mind to move letsencrypt-auto inside this dir. However, now we'd like to copy it or link it to the root level, where people are used to finding it (at least for awhile). Since it would be confusing to have a letsencrypt-auto and a letsencrypt_auto right next to each other, we rename this folder.
cd43e90
@erikrose erikrose changed the title from Make letsencrypt-auto release-suitable, self-upgrading, and self-contained. [Do not merge.] to Make letsencrypt-auto release-suitable, self-upgrading, and self-contained. Jan 8, 2016
@pde
Member
pde commented Jan 9, 2016

Do we know why the tests are failing in this branch?

@erikrose
Contributor

@pde: It looks like something related to MySQL connections in the Boulder tests, probably a side effect of changing to sudo: required. Ideas are welcome. I haven't dug in much yet.

@pde
Member
pde commented Jan 11, 2016

I'd say that for now, let's run all nosetests that require sudo manually outside of travis? That's what I'm doing with tox -e apacheconftest.

pde and others added some commits Jan 11, 2016
@pde pde Remove test key e17bb27
@erikrose erikrose Switch to real key, and add signing to release script. Close #1573. 2f3425a
@pde pde Use SHA256 openssl signatures be653e8
@pde pde Clearer notes about when / how to edit the script 916f891
@pde pde Better processing & documentation of leauto flags
 - move them to the top for clarity
 - accept them in any position
 - shadow & document them in the Python client
1b3c8e8
@erikrose erikrose Take le-auto tests out of Travis until we figure out why sudo:require…
…d causes other ones to fail.

For now, we'll run them locally with `tox -e le_auto` as we do with the apacheconf tests.
66ca744
@erikrose erikrose Remove mock as an install requirement.
The motivation is to free us of a reliance on a rather modern version of setuptools, which caused le-auto failures for people on Wheezy and other older distros. (The alternative would have been to forcibly upgrade setuptools as the old le-auto did, but less is more.)

Mock is used only in tests, so we move it to tests_require. It will still be installed automatically when setup.py test is run. Give all packages a test_suite so this works.

The "testing" extra remains for optional packages not required for the nose tests but used in tox. However, the extra is much less useful now and is a candidate for deletion. We could roll the list of packages therein into the tox config so as not to favor any particular package.

Remove tests_require=install_requires, which I don't think does anything useful, since install requirements are implicitly installed when running setup.py test.

Fix tests to pass with mock removed. We had to stop them pulling down LE from PyPI, since the current version there (0.1.1) requires mock and explodes when `letsencrypt` is run.
6c05197
@erikrose erikrose Master master into letsencrypt-auto-release so Travis will build it. ed56264
@erikrose erikrose Get all tests, even le_auto, working on Travis.
Switch to a MySQL 5.6 setup based on https://github.com/mozilla/treeherder/pull/1080/files and Travis's beta trusty infra, which runs on Google Compute Engine.

Remove MariaDB addon, which conflicts with the socket used by the treeherder approach's mysql package. Remove maria service (which has no effect).
7ee23b7
@erikrose erikrose Disable too-many-instance-attributes for the acme linter.
This should make the linter pass and allow us to merge the letsencrypt-auto-release branch when it's ready. IHNI why it passes on master without this disabled.
a3288a9
@erikrose erikrose Fix Fedora 23 crasher.
This fixes an "OSError: [Errno 2] No such file or directory" on Fedora 23. Note that openssl-devel was not sufficient to install the openssl commandline tool.

The current manual-testing build of le-auto now crashes with #1548, but that should have been resolved when we upgraded the cryptography lib and so should go away when we build a new version.
cb5beb8
@pde pde Merge branch 'letsencrypt-auto-release' of https://github.com/erikros…
…e/letsencrypt into letsencrypt-auto-release
3abf028
@erikrose erikrose Bring built le-auto script up to date. a7ae436
@pde pde Undelete the old letsencrypt-auto for now 435dfc0
@erikrose @pde erikrose Bring built le-auto script up to date. 2d4c21a
@bmw @pde bmw Fix fake letsencrypt e192cce
@bmw @pde bmw Rebuild sdist 7945db7
@bmw @pde bmw Fixed fake letsencrypt hash ab07620
@erikrose erikrose Remove backported Python 2.7 assertion helpers.
I didn't backport their imports, so they had NameErrors in the failure case anyway. And, because of the docker image, these tests currently are run under only 2.7 at the moment.
86266f5
@erikrose erikrose Upgrade half-sign to sha256. Bring back old le-auto temporarily. Impr…
…ove le-auto's option parsing.

If the new le-auto works well in the minutes or hours after release, we'll make another commit to master that removes the old le-auto and bootstrap scripts.

Close #2.
d813097
@kuba kuba and 1 other commented on an outdated diff Jan 13, 2016
acme/.pylintrc
@@ -60,7 +60,7 @@ confidence=
# --enable=similarities". If you want to run only the classes checker, but have
# no Warning level messages displayed, use"--disable=all --enable=classes
# --disable=W"
-disable=fixme,locally-disabled,abstract-class-not-used
@kuba
kuba Jan 13, 2016 Contributor

that doesn't seem to belong to this PR, please revert

@erikrose
erikrose Jan 13, 2016 Contributor

I'll roll this back with the Travis config rollback. Its relevance lay in the Travis config, in that the linter mysteriously ignored the too-many-attrs "problem" on master but not on this branch.

@kuba kuba and 2 others commented on an outdated diff Jan 13, 2016
acme/setup.py
if sys.version_info < (2, 7):
install_requires.extend([
# only some distros recognize stdlib argparse as already satisfying
'argparse',
- 'mock<1.1.0',
@kuba
kuba Jan 13, 2016 Contributor

I'm really not convinced about moving this out of install_requires

@erikrose
erikrose Jan 13, 2016 Contributor

I think we resolved this to our mutual satisfaction in IRC. Kuba's concern was that test_utils routines in acme might be called from future third-party acme clients and that test_utils-only dependencies would thus not get installed. We will solve this, if it arises, by keeping a [testing] extra in acme that includes mock and having the third-party client's tests_require='acme[testing'].

@kuba
kuba Jan 13, 2016 Contributor

Unfortunately, we haven't been able to reach a consensus here yet. In fact, after more extensive IRC discussion, I'm pretty sure this should not be merged here.

In general, I strongly believe that:

  1. le-auto should not impose any constraints on setup.py. By the way, how would you impose such constraints on setup.py files found external dependencies?
  2. Up so far, we have kept our tests together with the main source code. As long as we do it, we should also have test deps in install_requires, because that code is shipped to end-users and importable. Rationale for such setup is not really in the scope of this PR, but I'm open to discussion elsewhere. Moreover, messing with this is likely introduce more bugs. That particular concern Erik brought up, was the quickest argument I could come up with, in that particular context.

An obvious solution here is to have le-auto pin "mock==1.1.0" as this would satisfy acme's setup.py constraints under both Python 2.6 and 2.7. Implementing this plan is about adding one new line to pin mock in conditional_requirements.py and one (or more) to add it's hash (anyway it must be easy - it was meant to be this way, because we're likely often to update those pins and their hashes - that's the whole point of this PR). Touching setup.py is really the wrong hammer...

Apologies if you think this comes out late in review, @erikrose . This PR was hanging in the queue as WIP almost forever now, lastests commits are still being added, and it was never labelled with acme - hence I didn't realize it's touching acme/ subproject. However, I've hinted you about my concerns much earlier than the comment above.

@kuba
kuba Jan 13, 2016 Contributor

FTR Remark 1. goes together with my generic comment about conditional_requirements.py from https://github.com/letsencrypt/letsencrypt/pull/1665/files#r49646113.

@erikrose
erikrose Jan 13, 2016 Contributor

Upon further discussion, I don't think we've reached agreement yet after all.

@pde
pde Jan 13, 2016 Member

I don't really agree that we need to split tests into separate packages before removing mock as a dependency (I think it's not insane to have a package that functions with core functionality without some extra package X, but which can do more -- such as run tests -- if X is present).

But I do agree that there's some risk of introducing bugs this way. So my vote would be pin mock==1.1.0 for this release, then take more time to consider possible breakage cases, perhaps wrap import mocks in an error handler, and then revert the mock==1.1.0 pin for a future release?

@pde
pde Jan 13, 2016 Member

After much further discussion on IRC and at EFF, our revised conclusion is: we'd like to move the mock dependency out of the main setup.py, but we aren't going to rush shipping that way, so 0.2.0 is going out without this change and we can do it in a more relaxed manner for 0.2.1 on Monday.

@kuba
kuba Jan 15, 2016 Contributor

Would you mind actually addressing my concerns and suggested solution instead of ignoring it? I don't see any relevant discussion in IRC either.

To make it explicit: please don't remove mock dependency (at least as a part of this PR). Thanks!

@pde
pde Jan 15, 2016 Member

@kuba absolutely yes! We're not going to land the mock removal in a hurry and without your agreement. Regarding your questions:

  1. Obviously we can't modify the setup.py of third party dependencies unless we fork of vendorise those projects, though in some cases (ConfigArgParse) we've already been tempted to do that, and it's not totally out of the question. We'd always prefer to work with those projects to keep their dependency sets minimal if possible.
  2. How would you feel about a separate PR against acme which (1) removes mock from the main dependency set; (2) raises a planned exception if the mock-dependent test code is called without mock installed; (3) documents this behaviour as part of the API?
@kuba kuba commented on an outdated diff Jan 13, 2016
acme/setup.py
@@ -75,6 +74,7 @@
packages=find_packages(),
include_package_data=True,
install_requires=install_requires,
+ tests_require='mock<1.1.0' if sys.version_info < (2, 7) else 'mock',
@kuba
kuba Jan 13, 2016 Contributor

if we're really going with this approach (see https://github.com/letsencrypt/letsencrypt/pull/1665/files#r49645236), then please use a tests_require variable just as we do for all other package lists

@bmw bmw commented on an outdated diff Jan 13, 2016
docs/contributing.rst
@@ -22,7 +22,7 @@ once:
git clone https://github.com/letsencrypt/letsencrypt
cd letsencrypt
- ./bootstrap/install-deps.sh
+ ./letsencrypt-auto/letsencrypt-auto --os-packages-only
@bmw
bmw Jan 13, 2016 Contributor

Incorrect path.

@bmw bmw and 1 other commented on an outdated diff Jan 13, 2016
docs/contributing.rst
-In general:
+.. code-block:: shell
+
+ letsencrypt-auto/letsencrypt-auto --os-packages-only
@bmw
bmw Jan 13, 2016 Contributor

Ditto

@bmw
bmw Jan 13, 2016 Contributor

Actually, I think we should just make it simpler and revert all changes to this file for now.

@erikrose
erikrose Jan 13, 2016 Contributor

I think we can be confident in the le-auto-based bootstrapping at this point. It's textually almost identical to the old scripts, and we've vetted it on the test farm and in the community. Furthermore, we're talking to devs here, and they should be using the latest, not least because they know how to file bugs at us if it doesn't work. :-)

@kuba kuba commented on the diff Jan 13, 2016
acme/setup.py
@@ -22,15 +22,14 @@
]
# env markers in extras_require cause problems with older pip: #517
+# Keep in sync with conditional_requirements.py.
@kuba
kuba Jan 13, 2016 Contributor

What is the plan for this when acme gets into it's own repo? it will become pretty much infeasible to keep those in sync. Looking at the problem from different perspective, how do you make sure in conditional_requirements.py that it pulls conditional deps from our own dependencies that we don't have control over?

@erikrose
erikrose Jan 13, 2016 Contributor

That, unfortunately, is a price we pay for deeply pinning dependencies: more security, less dynamism. I haven't audited our third-party dependencies for conditional subdependencies. That said, we haven't received any test reports about failures spawned therefrom. If we do, we'll include them here. (It's a dirty job, but that's packaging for you.)

@kuba
kuba Jan 16, 2016 Contributor

This sounds a bit scary. Even simplest changes to setup.py tend to cause problems as we've seen with #1914. I hope you're ready to receive a bunch of bug reports from affected users. Or maybe it's better to perform such audit? In fact, you may want to have an automatic script that does it (before merging this PR), as any dep version bump might cause troubles.

@erikrose
erikrose Jan 19, 2016 Contributor

It is a bit scary. My favored approach (please see #2182 (comment) and the following comment) is to do away with conditional dependencies.

@kuba kuba and 1 other commented on an outdated diff Jan 13, 2016
@@ -45,14 +47,13 @@ branches:
- master
- /^test-.*$/
-# container-based infrastructure
-sudo: false
+sudo: required
@kuba
kuba Jan 13, 2016 Contributor

That setting was pushed back and forth a lot of times... Why is it necessary this time and how does it relate to what this PR is trying to solve?

@erikrose
erikrose Jan 13, 2016 Contributor

It allows us to run the le-auto integration tests, which need sudo almost by definition, on Travis. (I isolate them in a Docker container so our scribbling doesn't interfere with other tests and so they can be run locally.)

@kuba kuba and 1 other commented on an outdated diff Jan 13, 2016
# apacheconftest
#- apache2
# http://docs.travis-ci.com/user/ci-environment/#CI-environment-OS
# gimme has to be kept in sync with Boulder's Go version setting in .travis.yml
before_install:
+ - sudo apt-get update -qq
+ - sudo apt-get install -qq mysql-server-5.6 mysql-client-5.6 mysql-client-core-5.6
@kuba
kuba Jan 13, 2016 Contributor

Again, this PR is about letsencrypt-auto - why is mysql touched here?

@erikrose
erikrose Jan 13, 2016 Contributor

Please see the commit message.

@bmw bmw and 1 other commented on an outdated diff Jan 13, 2016
@@ -22,8 +22,8 @@ WORKDIR /opt/letsencrypt
# directories in its path.
-COPY bootstrap/ubuntu.sh /opt/letsencrypt/src/ubuntu.sh
-RUN /opt/letsencrypt/src/ubuntu.sh && \
+COPY letsencrypt-auto-source/letsencrypt-auto /opt/letsencrypt/src/letsencrypt-auto
@bmw
bmw Jan 13, 2016 Contributor

Since we're not instructing users to make the shift to the new letsencrypt-auto yet, I don't think we should be making the changes to the Dockerfile now.

@erikrose
erikrose Jan 13, 2016 Contributor

Again, this is a dev-only thing, and I think we're pretty confident it works. Do you really want to revert it?

@bmw bmw commented on an outdated diff Jan 13, 2016
@@ -7,7 +7,7 @@ VAGRANTFILE_API_VERSION = "2"
# Setup instructions from docs/contributing.rst
$ubuntu_setup_script = <<SETUP_SCRIPT
cd /vagrant
-./bootstrap/install-deps.sh
+./letsencrypt-auto/letsencrypt-auto --os-packages-only
@bmw
bmw Jan 13, 2016 Contributor

Bad path

@kuba kuba added the acme label Jan 13, 2016
bmw and others added some commits Jan 13, 2016
@bmw bmw Revert "Get all tests, even le_auto, working on Travis."
This reverts commit 7ee23b7.
587e2e7
@bmw bmw Revert changes to Dockerfile a1f6678
@bmw bmw Fix Vagrantfile path a287b50
@bmw bmw Fix paths in contributing.rst bccb212
@erikrose erikrose Roll back change to acme's pylintrc, which was needed to get lint to …
…pass on Travis's Trusty beta (sudo) infra.

We're stepping off that infra briefly, to keep it the same as boulder's. When we retire the old le-auto, we'll step back on and change boulder to use it as well.
c3ea4bd
@erikrose erikrose Bring built le-auto up to date again. 25e428c
@erikrose erikrose Merge master in before computing a known-good set for 0.2.0.
This also serves as a suitable base to build sdists for isnot.org, so we can try the old le-auto script against mockless versions of the LE packages.
2771249
@pde pde modified the milestone: 0.2.0, 0.2.1 Jan 13, 2016
erikrose added some commits Jan 15, 2016
@erikrose erikrose Merge master in to get the unconditionalization of dependencies. ecbe2a5
@erikrose erikrose Update known-good-set, and make deps unconditional.
Bring everything to the latest versions.

Make dependencies unconditional: argparse, ndg-httpsclient, and pyasn1 get in all the time, to match the state of master as of 0.2.0.
1706619
@erikrose erikrose Revert moving mock to test_requires.
We'll take this up later, but I don't want to hold up the new le-auto on this debate.
e1bd164
@pde
Member
pde commented Jan 15, 2016

LGTM! @bmw want to give this a final look over and merge?

@bmw bmw was assigned by pde Jan 15, 2016
@erikrose erikrose Add mock==1.0.1, the Python 2.6 compatible version, to le-auto reqs.
This should ward off the runtime crashes described in 6c05197.
e923901
@kuba kuba commented on an outdated diff Jan 16, 2016
# are detected, c.f. #1002
commands =
- pip install -e acme[testing]
- nosetests -v acme
- pip install -e .[testing]
- nosetests -v letsencrypt
+ pip install -e acme
+ python acme/setup.py test
@kuba
kuba Jan 16, 2016 Contributor

I believe all lines in tox.ini should be reverted, including above 2, due to the fact we changed handling of install_requires and tests_requires. Especially given the fact that you removed tests_require=install_requires lines throughout.

@kuba kuba and 1 other commented on an outdated diff Jan 16, 2016
@@ -8,23 +8,20 @@
skipsdist = true
envlist = py{26,27,33,34,35},py{26,27}-oldest,cover,lint
-# nosetest -v => more verbose output, allows to detect busy waiting
-# loops, especially on Travis
@kuba
kuba Jan 16, 2016 Contributor

so why is nosetest actually removed? If it is removed here, why do we keep 'nose' in testing_extras throughout?

@erikrose
erikrose Jan 19, 2016 Contributor

Replacing nosetests with python setup.py test allowed us to move mock to test_requires; setuptools then installs it on demand, when tests are run. That was the immediate practical benefit. However, since you've argued for not moving mock yet, there's less reason to do this at the moment. If you like, I can bundle it with the later PR that moves mock.

@kuba
kuba Jan 19, 2016 Contributor

Please do, thanks!

@kuba kuba and 1 other commented on an outdated diff Jan 19, 2016
# are detected, c.f. #1002
commands =
- pip install -e acme[testing]
+ pip install -e acme
@kuba
kuba Jan 19, 2016 Contributor

pip install lines should also be reverted, I believe

@erikrose
erikrose Jan 19, 2016 Contributor

Good catch.

erikrose added some commits Jan 19, 2016
@erikrose erikrose Revert switch to `python setup.py test` in tox.ini.
This had more of a purpose when we were moving mock to test_requires. I'll reintroduce this in the separate PR for that.

Also bring back the testing extra in tox for now.
aefd5b2
@erikrose erikrose Remove errant DS_Store. Ick. b20eab6
@pde
Member
pde commented Jan 19, 2016

Looks good to me as long as setup.cfg doesn't need that trailing newline.

@erikrose
Contributor

@pde It doesn't, but here it is anyway.

@bmw
Contributor
bmw commented Jan 19, 2016

Fixes #1572

LGTM && merging

@bmw bmw merged commit 8301f2f into certbot:master Jan 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment