tahoe-lafs/trac

remove "bin/tahoe" and fancy "@" runner support #2735

New issue

Open

opened 2016-02-23 05:45:14 +00:00 by warner · 4 comments

warner commented

2016-02-23 05:45:14 +00:00

There And Back Again: Climbing The Complexity Mountain (and Coming Back Down The Other Side)

A long long time ago, one of the first bits of node-management code we added to the Tahoe tree was a launcher script which set PYTHONPATH (to the source tree), then ran twistd to load a tahoe node from a .tac file (which daemonized into the background).

Later (b75276a in Sep-2007), when we were figuring out dependency management, we ended up writing a bespoke implementation of virtualenv (before virtualenv was a thing). This was a scheme where everything we needed (except tahoe itself) was installed into a support/ directory, and the launcher script's $PYTHONPATH pointed into both support/ and the source tree's src/ directory (for tahoe). Because the twistd binary had to come from support/bin/, the launcher script had to set $PATH too.

This launcher script was installed as bin/tahoe at the top of the source tree, so the build process was like python setup.py build and then bin/tahoe --version. Getting the shbang line right hard, as was making sure the file had a windows ".exe" suffix when appropriate, so ([083795d] in Jan-2009) bin/tahoe was generated from a template, instead of being checked in like a regular file, which introduced a new make_executable command for setup.py.

About that same time (b77c89a in Jan-2009), some unit tests appeared, which had complicated checks to make sure that $PATH contained the expected things, and that we weren't executing tahoe from anywhere else, etc. The unit tests were made more challenging by the fork-and-exec that occurred inside twistd, which also escapes control of the code-coverage tracing function. To allow the exec() to locate twistd, both $PATH and $PYTHONPATH must be inherited by all child processes, so by this point the command to run the tahoe test suite had blossomed from a simple trial allmydata to a complex shell script.

The pinnacle of complexity was achieved ([3798d99] in Jan-2011) when the tahoe executable itself absorbed responsibility for running the test suite, or coverage, or flogtool, or python, or other commands that might need $PATH/$PYTHONPATH set to fully immerse oneself in the Tahoe lifestyle. python bin/tahoe debug repl was just like python but different. python bin/tahoe @python -3 tahoe --version let you put extra python arguments into your tahoe. python bin/tahoe @coverage run @tahoe debug trial allmydata became a thing.

It's All Downhill From Here

Fortunately, by that time (ac3b26ec in Oct-2010), we had already stopped shelling out to a twistd executable: instead, we imported the twistd library and invoked its functionality directly. This retains the fork(), but removes the exec(). Still later ([87a6894] in May-2012) we stopped using the contents of .tac files to load the tahoe node.

It's time to continue the trend towards simplicity. I believe we no longer need anything which manipulates $PATH or $PYTHONPATH, and can instead rely upon a fully-installed tahoe+dependencies (either installed to the system "for reals", or in a virtualenv). The one entry point shall be a bare "tahoe" installed as a setup.py "entrypoint", and looked up by the shell on $PATH like any other command. trial allmydata, perhaps fronted by tox (or run under coverage), shall be the one test-running command. pip install . (perhaps with --editable) shall be the one install-from-source-tree command.

Simplify! Simplify! Simplify!

To that end, I'd like to delete the following:

bin/tahoe and setup.py make_executable
scripts.debug.repl, trial, flogtool, and all the "@" stuff
test_runner.BinTahoe
test_runner.RunNode

(although maybe test_runner.BinTahoe.test_unicode_arguments_and_output, which makes sure that unicode values in argv get delivered to the tahoe code correctly, should be retained in some form that doesn't require so much complexity in the entrypoint script)

We need to keep exercising the tahoe start functionality in unit tests. This is tricky because twistd, even as a library, terminates the calling process, so we need a fork() in the test code. But I think we can write a narrow test that doesn't require tahoe to be a general-purpose binary launcher.

## There And Back Again: Climbing The Complexity Mountain (and Coming Back Down The Other Side) A long long time ago, one of the first bits of node-management code we added to the Tahoe tree was a launcher script which set PYTHONPATH (to the source tree), then ran `twistd` to load a tahoe node from a .tac file (which daemonized into the background). Later (b75276a in Sep-2007), when we were figuring out dependency management, we ended up writing a bespoke implementation of virtualenv (before virtualenv was a thing). This was a scheme where everything we needed (except tahoe itself) was installed into a `support/` directory, and the launcher script's $PYTHONPATH pointed into both `support/` and the source tree's `src/` directory (for tahoe). Because the `twistd` binary had to come from `support/bin/`, the launcher script had to set $PATH too. This launcher script was installed as `bin/tahoe` at the top of the source tree, so the build process was like `python setup.py build` and then `bin/tahoe --version`. Getting the shbang line right hard, as was making sure the file had a windows ".exe" suffix when appropriate, so ([083795d] in Jan-2009) `bin/tahoe` was generated from a template, instead of being checked in like a regular file, which introduced a new `make_executable` command for setup.py. About that same time (b77c89a in Jan-2009), some unit tests appeared, which had complicated checks to make sure that $PATH contained the expected things, and that we weren't executing `tahoe` from anywhere else, etc. The unit tests were made more challenging by the fork-and-exec that occurred inside `twistd`, which also escapes control of the code-coverage tracing function. To allow the `exec()` to locate twistd, both $PATH and $PYTHONPATH must be inherited by all child processes, so by this point the command to run the tahoe test suite had blossomed from a simple `trial allmydata` to a complex shell script. The pinnacle of complexity was achieved ([3798d99] in Jan-2011) when the `tahoe` executable itself absorbed responsibility for running the test suite, or `coverage`, or `flogtool`, or `python`, or other commands that might need $PATH/$PYTHONPATH set to fully immerse oneself in the Tahoe lifestyle. `python bin/tahoe debug repl` was just like `python` but different. `python bin/tahoe @python -3 tahoe --version` let you put extra python arguments into your tahoe. `python bin/tahoe @coverage run @tahoe debug trial allmydata` became a thing. ## It's All Downhill From Here Fortunately, by that time (ac3b26ec in Oct-2010), we had already stopped shelling out to a `twistd` executable: instead, we imported the twistd library and invoked its functionality directly. This retains the `fork()`, but removes the `exec()`. Still later ([87a6894] in May-2012) we stopped using the contents of .tac files to load the tahoe node. It's time to continue the trend towards simplicity. I believe we no longer need anything which manipulates $PATH or $PYTHONPATH, and can instead rely upon a fully-installed tahoe+dependencies (either installed to the system "for reals", or in a virtualenv). The one entry point shall be a bare "tahoe" installed as a setup.py "entrypoint", and looked up by the shell on $PATH like any other command. `trial allmydata`, perhaps fronted by `tox` (or run under `coverage`), shall be the one test-running command. `pip install .` (perhaps with `--editable`) shall be the one install-from-source-tree command. ## Simplify! Simplify! Simplify! To that end, I'd like to delete the following: * `bin/tahoe` and `setup.py make_executable` * `scripts.debug.repl`, `trial`, `flogtool`, and all the "@" stuff * `test_runner.BinTahoe` * `test_runner.RunNode` (although maybe `test_runner.BinTahoe.test_unicode_arguments_and_output`, which makes sure that unicode values in argv get delivered to the tahoe code correctly, should be retained in some form that doesn't require so much complexity in the entrypoint script) We need to keep exercising the `tahoe start` functionality in unit tests. This is tricky because `twistd`, even as a library, terminates the calling process, so we need a `fork()` in the test code. But I think we can write a narrow test that doesn't require `tahoe` to be a general-purpose binary launcher.

warner added the

labels 2016-02-23 05:45:14 +00:00

warner added this to the undecided milestone 2016-02-23 05:45:14 +00:00

warner commented

2016-02-23 19:03:23 +00:00

Author

Daira gave me a braindump: #565 explains what test_unicode_arguments_and_output is testing. Basically the setuptools entrypoint wrapper .exe (on windows) doesn't handle sys.argv correctly (it irretrievably mangles non-ascii characters), and the tahoe workaround is to make a .pyscript wrapper instead (adding it to the windows registry as an executable file type). The test asserts that this new-and-improved wrapper is able to receive a non-ascii argument (by looking at the "unknown command" error it emits, which should echo the argument that was passed in).

The long-term fix for #565 is to get setuptools to use a better wrapper. Then the setuptools-created entry point (venv/bin/tahoe for unix, venv/bin/tahoe.exe for windows) will behave correctly.

So: do we still need this test? Probably yes. This test should still execute the wrapper, but the actual pathname of the wrapper will be unusual on windows while our workaround is in place (tahoe.pyscript instead of tahoe or tahoe.exe).

Is this the only place in the test suite where we want to execute a child process? (testing the launcher requires os.fork, but maybe not os.exec, and I'm hoping we can get rid of os.exec calls)

Daira gave me a braindump: #565 explains what `test_unicode_arguments_and_output` is testing. Basically the setuptools entrypoint wrapper .exe (on windows) doesn't handle sys.argv correctly (it irretrievably mangles non-ascii characters), and the tahoe workaround is to make a .pyscript wrapper instead (adding it to the windows registry as an executable file type). The test asserts that this new-and-improved wrapper is able to receive a non-ascii argument (by looking at the "unknown command" error it emits, which should echo the argument that was passed in). The long-term fix for #565 is to get setuptools to use a better wrapper. Then the setuptools-created entry point (venv/bin/tahoe for unix, venv/bin/tahoe.exe for windows) will behave correctly. So: do we still need this test? Probably yes. This test should still execute the wrapper, but the actual pathname of the wrapper will be unusual on windows while our workaround is in place (`tahoe.pyscript` instead of `tahoe` or `tahoe.exe`). Is this the only place in the test suite where we want to execute a child process? (testing the launcher requires `os.fork`, but maybe not `os.exec`, and I'm hoping we can get rid of `os.exec` calls)

Brian Warner <warner@lothar.com> commented

2016-09-09 23:22:12 +00:00

Owner

In 8d9afdc/trunk:

CLI: remove 'debug trial', 'debug repl'

These are obsolete. Tests are run with 'tox', or by running 'trial
allmydata' from a populated virtualenv. A populated virtualenv is also
the right way to get a repl: just run 'python'.

refs ticket:2735

In [8d9afdc/trunk](/tahoe-lafs/trac/commit/8d9afdc27eea4dd58747baa3176d9769ea4b5213): ``` CLI: remove 'debug trial', 'debug repl' These are obsolete. Tests are run with 'tox', or by running 'trial allmydata' from a populated virtualenv. A populated virtualenv is also the right way to get a repl: just run 'python'. refs ticket:2735 ```

warner commented

2016-09-09 23:26:16 +00:00

Author

Some updates:

we got rid of bin/tahoe and setup.py make_executable in 1.11 ([0f3ce7a] 15-Mar-2016)
I just removed debug trial and debug repl in [8d9afdc], and that change will appear in 1.12
test_runner still has BinTahoe and RunNode, and I'm not planning on doing anything with them for 1.12

Some updates: * we got rid of `bin/tahoe` and `setup.py make_executable` in 1.11 ([0f3ce7a] 15-Mar-2016) * I just removed `debug trial` and `debug repl` in [8d9afdc], and that change will appear in 1.12 * `test_runner` still has `BinTahoe` and `RunNode`, and I'm not planning on doing anything with them for 1.12

exarkun commented

2021-01-13 19:43:03 +00:00

Some updates:

For ticket:3588 I added direct test coverage for two non-ASCII-related behaviors of a process that is using allmydata/windows/fixups.py: receiving such values in argv and writing such values to stdout and stderr. This very directly tests the behavior that BinTahoe.test_unicode_arguments_and_output is also meant to test - but without getting allmydata.scripts.runner involved.
Because there is no longer a wrapper script for tahoe on Windows, for ticket:3581 I was able to remove all of the unicode "mangling" being done for Windows and adapted BinTahoe.test_unicode_arguments_and_output so that it still tests argv/output functionality conferred upon allmydata.scripts.runner by allmydata.windows.fixups. I think BinTahoe.test_unicode_arguments_and_output is now pretty fine to keep.
RunNode still exists but it seems to have been cleaned up a lot since this ticket was filed. It still doesn't strike me as uniformly fantastic but neither does it seem like a substantial liability.

So we may be zeroing in on being able to consider this ticket resolved.

Some updates: * For ticket:3588 I added direct test coverage for two non-ASCII-related behaviors of a process that is using `allmydata/windows/fixups.py`: receiving such values in argv and writing such values to stdout and stderr. This very directly tests the behavior that `BinTahoe.test_unicode_arguments_and_output` is also meant to test - but without getting `allmydata.scripts.runner` involved. * Because there is no longer a wrapper script for tahoe on Windows, for ticket:3581 I was able to remove all of the unicode "mangling" being done for Windows and adapted `BinTahoe.test_unicode_arguments_and_output` so that it still tests argv/output functionality conferred upon `allmydata.scripts.runner` by `allmydata.windows.fixups`. I think `BinTahoe.test_unicode_arguments_and_output` is now pretty fine to keep. * `RunNode` still exists but it seems to have been cleaned up a lot since this ticket was filed. It still doesn't strike me as uniformly fantastic but neither does it seem like a substantial liability. So we may be zeroing in on being able to consider this ticket resolved.