get rid of tahoe.exe launcher

[zooko]

There is a binary (cli.exe) in the Tahoe-LAFS source tree under revision control. It is built from launcher.c in zetuptoolz:

​http://tahoe-lafs.org/trac/zetuptoolz/browser/launcher.c

source:setup.py copies it to bin\tahoe.exe when building.

There are several reasons to get rid of this launcher:

• it mangles any non-ASCII arguments, preventing us from fixing #565 on Windows;
• it does not work on Win64;
• it adds a small amount of overhead to running CLI commands;
• it's unnecessary complexity;
• it isn't in the spirit of open source to have a binary that is not compiled from source as part of our build process -- getting rid of it fixes this without complicating the build or requiring a C compiler;

A Python script called tahoe.py, say, can be run just fine on Windows as tahoe, provided that the PATHEXT environment variable includes ".py". (Actually we're going to use ".pyscript".)

[davidsarah]

Attaching my work-in-progress for Unicode argument support on Windows and removal of tahoe.exe, so that it doesn't get lost.

[zooko]

Is this going to require users to edit their PATHEXT environment variables in order to run Tahoe-LAFS? Or will installing Python do that for them? If we're going to make this change for v1.7.1 let us make it early in the release cycle so that users on Windows have plenty of time t let us know if it changes the behavior in a way that causes them problems.

[davidsarah]

Replying to zooko:

Is this going to require users to edit their PATHEXT environment variables in order to run Tahoe-LAFS? Or will installing Python do that for them?

Installing Python does not edit PATHEXT. But it's very easy to do so without going through the GUI:

setx PATHEXT "%PATHEXT%;.PY"

This takes effect for any new instances of cmd.exe run by the current user. (There is an /m option to set the variable for all users on the current machine, but it will typically fail on Vista and later unless you run setx with elevation, and I don't think we need to do that anyway.)

setx.exe is an executable in %SystemRoot%\system32, it's not a cmd.exe builtin. To set PATHEXT for the current cmd.exe instance as well (e.g. in a batch file), also do set "PATHEXT=%PATHEXT%;.PY", which is a builtin.

Installing Python does associate the .py extension with that Python interpreter, so even without the setx command, it's sufficient to run tahoe.py from the command-line instead of tahoe.

If we're going to make this change for v1.7.1 let us make it early in the release cycle so that users on Windows have plenty of time to let us know if it changes the behavior in a way that causes them problems.

OK, that makes sense.

[davidsarah]

Replying to davidsarah:

Replying to zooko:

Is this going to require users to edit their PATHEXT environment variables in order to run Tahoe-LAFS? Or will installing Python do that for them?

Installing Python does not edit PATHEXT. But it's very easy to do so without going through the GUI:

setx PATHEXT "%PATHEXT%;.PY"

On WinXP Service Pack 2 and later, apparently. But we can probably assume that. (If any system running earlier Windows is connected to the Internet and still usably free of malware, I'd be surprised.)

[zooko]

This issue appears to be blocking some of our buildbots from working (unless I misunderstand):

​http://tahoe-lafs.org/buildbot/builders/Soultcer%20Win7-64%20py2.6 ​http://tahoe-lafs.org/buildbot/builders/Dcoder%20Win7-64%20py2.6

Also I think it is blocking #565. Let's prioritize this ticket! What's the next step? I think it is to decide if we want the new install docs to instruct all users on all platforms to execute a file named "tahoe.py" instead of a file named "tahoe", or if we want the install docs to differ depending on platform here, or what.

I don't fully understand how things would change at this level -- install docs and user experience -- if we got rid of tahoe.exe.

[zeromus]

I don't speak for every windows user, but I can try to. Once a windows user has gone to the trouble to install python and build tahoe, I don't think he cares whether he has to double click tahoe.exe or tahoe.py. It is going to feel gross to him, and very likely unacceptably so, compared to a redistributable zipfile with an executable in it, e.g. a py2exe frozen thingy.

I advise you not to sweat this exe at all and focus on making sure the py2exe process is working instead.

Folks who use python and windows will have a different opinion, because they're used to this.

[davidsarah]

Replying to zeromus:

I don't speak for every windows user, but I can try to. Once a windows user has gone to the trouble to install python and build tahoe, I don't think he cares whether he has to double click tahoe.exe or tahoe.py.

This is the command-line interface. The user isn't double-clicking anything; they're typing "tahoe ...args..." at a Command Prompt. (Or a cygwin shell prompt; we don't support cygwin, but I don't want to break Tahoe on cygwin unnecessarily.)

Apart possibly from running an extra command in the install process to set up PATHEXT, the difference between tahoe.exe and tahoe.py is an implementation detail that doesn't affect the user interface.

For bbfreeze, which is similar to py2exe, see #585 -- but I think that's a distraction as far as this ticket is concerned, since we don't want to create a dependency on bbfreeze (or py2exe).

[zeromus]

Agh I am mixed up. You're right. but--

There is one huge difference between the .exe and the .py, if I recall correctly: the .exe has the c:\path\to\python baked into it which renders it completely non redistributable. The .py has no such dependency in it. As a consequence, I can send my python directory to someone else and have them run the .py, but the .exe isn't going to work unless they put it in the same place in their filesystem.

[davidsarah]

Samuel Neves wrote on tahoe-dev:

Regarding #1074, which is the issue preventing Win64 from being "all green". I applied to 'work-in-progress' patch in there. However, setuptools now fails to build, due to the missing cli.exe file. I tracked this down to setuptools/command/easy_install.py (get_script_args()), but haven't been able to fix it as of yet, since I know neither python nor the build tools/environment this uses.

Please try the attached zetuptoolz-dont-write-script-exe.2.dpatch (on top of unicode-args-and-rm-tahoe-exe-wip.dpatch).

If your PATHEXT does not already contain .py, also do:

setx PATHEXT "%PATHEXT%;.PY"

Then test the tahoe cli in a new Command Prompt.

[davidsarah]

zetuptoolz: don't write .exe files for scripts (fixed version)

[davidsarah]

Please ignore zetuptoolz-dont-write-script-exe.dpatch, and test with zetuptoolz-dont-write-script-exe.2.dpatch instead.

If you already have a non-working tahoe.exe file in your scripts directory from a previous install, you will need to delete it.

[davidsarah]

Here is a new version that handles more corner cases and is less likely to break other scripts installed using zetuptoolz. This also fixes Unicode console output: it now magically works independently of the code page setting. (If stdout or stderr are redirected, the output is UTF-8.)

To answer zeromus' point, cli.exe (or name.exe) did not have any baked-in paths; it used argv[0] to find its name-script.py file. The name.pyscript and name files generated by the new version of zetuptoolz also do not, so there is no regression.

[davidsarah]

Changes to zetuptoolz to get rid of .exe wrappers, and support Unicode arguments.

[davidsarah]

Changes to Tahoe needed to work with new zetuptoolz (that does not use .exe wrappers on Windows), and to support Uni code arguments and stdout/stderr.

[davidsarah]

regedit file to set up .pyscript association (run with 'regedit pyscript.reg' and accept the scary prompts).

[davidsarah]

Replying to davidsarah:

To answer zeromus' point, cli.exe (or name.exe) did not have any baked-in paths; it used argv[0] to find its name-script.py file. The name.pyscript and name files generated by the new version of zetuptoolz also do not, so there is no regression.

Correction: the name script (which is only used on cygwin) does have hard-coded paths. I'll see if I can fix that.

[zooko]

Here are my comments:

1. This requires updates to docs/running.html (although I'm not yet sure what exactly needs to be changed)
2. There should be some theory-of-operation doc, perhaps just at the top of the setuptools/command/easy_install.py, explaining how we set up an executable on Windows.

3.

         except Exception:
             # okay, probably it was already gone

This could be a narrower catch:

    except EnvironmentError, le:
        # Ignore "No such file or directory", collect any other exception.
        if (le.args[0] != 2 and le.args[0] != 3) or (le.args[0] != errno.ENOENT):
            excs.append(le)

(copied from ​http://tahoe-lafs.org/trac/pyutil/browser/trunk/pyutil/fileutil.py?rev=218#L205 )

Okay overall I don't feel like I can really review this patch very well without more doc because I don't understand it very well. It looks to me like there are at least two parts that deserve to be publicized as being of interest to people outside of Tahoe-LAFS: 1. the "deep magic" in windows_fixups() seems like it should be offered to setuptools, distribute, and/or python core teams via their bug trackers, 2. the "bug in cygwin" bash seems like it ought to be reported to cygwin via their bug tracker. Let's open those tickets and link them back to this one and probably link them into launchpad.net (I'm happy to do some of that ticket gardening.)

Now what about testing? I think that trunk/src/allmydata/test/test_runner.py​ will test this code, although it may be (rightly or wrongly) disabled on Windows and/or Cygwin. Also we don't have a Cygwin buildslave. Also this patches doesn't update tests that of the unicode stdout on Windows. Probably there is already a test from the v1.7.0-cycle unicode work that tests unicode stdout and those tests need to be marked as no-longer-TODOs on Windows?

[davidsarah]

Note that the only discussion of cygwin on this ticket should be about supporting Windows Python (sys.platform == "win32", which confusingly includes Win64), but allowing the scripts to run correctly from a cygwin shell.

Supporting cygwin Python (sys.platform == "cygwin") is an entirely different issue, for which I've opened ticket #1119.

Replying to davidsarah:

To answer zeromus' point, cli.exe (or name.exe) did not have any baked-in paths; it used argv[0] to find its name-script.py file. The name.pyscript and name files generated by the new version of zetuptoolz also do not, so there is no regression.

Correction: the name script (which is only used on cygwin) does have hard-coded paths. I'll see if I can fix that.

OK, done. You can now move or copy the name and name.pyscript files, provided they are in the same directory. (The path to the Python interpreter is still hard-coded, because it should be possible to move the script relative to the interpreter; also, because that is what setuptools does for cygwin Python.)

Replying to zooko:

Here are my comments:

1. This requires updates to docs/running.html (although I'm not yet sure what exactly needs to be changed)

quickstart.html needs to be updated to add a step just before running python setup.py build. I've been waiting to do that until I know what command needs to be run (probably python setup.py winsetup).

2. There should be some theory-of-operation doc, perhaps just at the top of the setuptools/command/easy_install.py, explaining how we set up an executable on Windows.

I'm not sure it should be at the top of that file, but I'll add a new zetuptoolz.txt file describing the differences between setuptools and zetuptoolz.

3.

    except Exception:
        # okay, probably it was already gone

This could be a narrower catch:

    except EnvironmentError, le:
        # Ignore "No such file or directory", collect any other exception.
        if (le.args[0] != 2 and le.args[0] != 3) or (le.args[0] != errno.ENOENT):
            excs.append(le)

(copied from ​http://tahoe-lafs.org/trac/pyutil/browser/trunk/pyutil/fileutil.py?rev=218#L205 )

I've changed it (and the similar case for tahoe.exe) to:

      except Exception:
          if os.path.exists(tahoe_script):
              raise

Okay overall I don't feel like I [can't] really review this patch very well without more doc because I don't understand it very well. It looks to me like there are at least two parts that deserve to be publicized as being of interest to people outside of Tahoe-LAFS:

1. the "deep magic" in windows_fixups() seems like it should be offered to setuptools, distribute, and/or python core teams via their bug trackers,

Yes.

2. the "bug in cygwin" bash seems like it ought to be reported to cygwin via their bug tracker.

It turns out that this was only because we were writing the script with CRLF line endings. It's still a bug that cygwin bash doesn't tolerate that (and I'll report this bug if it is present in the latest version), but it doesn't affect us any more.

Let's open those tickets and link them back to this one and probably link them into launchpad.net (I'm happy to do some of that ticket gardening.)

Now what about testing? I think that trunk/src/allmydata/test/test_runner.py​ will test this code,

test_runner.py will test (to the same extent as on other platforms) that we respond correctly to Unicode arguments given that they are passed to the runner function. We previously had no tests that Unicode arguments were interpreted correctly when the bin/tahoe script is run as a separate process, but I've added one.

(Due to limitations of Python's and twisted's process-spawning primitives, it has to use the mangled encoding on Windows rather than testing with an actual Unicode argument, but that at least tests that the implementation is behaving as we expect.)

although it may be (rightly or wrongly) disabled on Windows and/or Cygwin.

The tests that involve twisted acting as a daemon are disabled on Windows (#27). We can test Unicode arguments without that. The disabling of runner tests on cygwin is ticket #908.

Also this patches doesn't update tests that of the unicode stdout on Windows. Probably there is already a test from the v1.7.0-cycle unicode work that tests unicode stdout and those tests need to be marked as no-longer-TODOs on Windows?

They were not marked as TODO; they were skipped because the Unicode arguments and/or output was not representable in the output encoding. They're no longer skipped.

[davidsarah]

Changes to Tahoe needed to work with new zetuptoolz (that does not use .exe wrappers on Windows), and to support Uni code arguments and stdout/stderr -- v4.

[davidsarah]

unicode-args-and-rm-tahoe-exe.4.dpatch moves the Windows-specific code to src/allmydata/windows/fixups.py, and makes the tests pass for the right reasons :-) We also have a new test that Unicode arguments and output works (added to TheRightCode in runner.py), and a change to quickstart.html. The zetuptoolz doc isn't written yet.

[zooko]

I just updated the PatchReviewProcess with this ticket in mind. I added:

Note: you do not have to have a complete 100% understanding of the patch's contents in order to sign off on it. You just have to have:

1. If there were some huge glaring hole in this patch, would I have a reasonable chance of noticing it?
2. Do I understand the whole intent of this code better now than I did when I started?
3. Do I understand what the implications of this patch are for our users? 

I chose not to sign off on this patch when I reviewed it because I didn't think I quite achieved number 1 there, and also because I thought it needed more/better doc or code-refactoring to make it easier to understand. It sounds like from comment:18 that attachment:unicode-args-and-rm-tahoe-exe.4.dpatch​ may have accomplished some of that.

Hm, in fact, regarding number 3 there, this patch probably needs to come with a NEWS snippet since it has a user-visible impact.

[zooko]

Here is a proposed NEWS snippet for this ticket:

 - Make the "tahoe" command-line tool work on 64-bit Windows and
   support unicode arguments and outputs on Windows (#1074)

[zooko]

Some notes from IRC as fodder for zetuptoolz.txt:

<alus> wow. why the CommandLineToArgvW code? what's wrong with sys.argv ?
…
<davidsarah> sys.argv is initialized from the result of CommandLineToArgvA,
	     which mangles characters that aren't representable in the "ANSI"
	     charset
<alus> Python bug?
<davidsarah> GetCommandLineA I mean
<davidsarah> yes, definitely
<alus> filed?
<davidsarah> let me check
<alus> the link to http://bugs.python.org/issue1602 was helpful.        [23:16]
<davidsarah> http://bugs.python.org/issue2128
<davidsarah> http://bugs.python.org/issue8775 also seems relevant
<davidsarah> oh, the second of those isn't the same bug, but it is a bug that
	     we want fixed in order to be able to *test* unicode arguments
								        [23:17]
<alus> scripts on my system are installed as C:\Python27\Scripts\*.py   [23:19]
<alus> why .pyscript?
<davidsarah> because the directory from which you run a script is added to
	     sys.path						        [23:20]
<davidsarah> so if any scripts in that directory have a .py extension, then
	     they will be dumped into the global namespace, and may conflict
	     with module names
<davidsarah> for example, zfec has a 'zfec' script, and also a 'zfec' module
								        [23:21]
<davidsarah> I have some half-written docs explaining this
…
<alus> what about the script hacking its own path out of sys.path?
…
<davidsarah> "hacking its own path out of sys.path" does actually seem to
	     work. I wonder if it solves the problem with zfec, or whether it
	     has any other undesirable side effects

[zooko]

The current ticket1074 branch has a couple of interesting failures on my Mac OS 10.6 "zomp":

• ​http://tahoe-lafs.org/buildbot/builders/Zooko%20zomp%20Mac-amd64%2010.6%20py2.6/builds/66/steps/test-from-prefixdir/logs/stdio
• ​http://tahoe-lafs.org/buildbot/builders/Zooko%20zomp%20Mac-amd64%2010.6%20py2.6/builds/66/steps/test-from-egg/logs/stdio

[davidsarah]

Changes to handling of scripts on Windows: get rid of .exe wrappers and use .pyscript/.pyw files instead. (Note: this patch is to zetuptoolz trunk, not Tahoe.)

[davidsarah]

Add zetuptoolz.txt documenting changes made by the fork

[davidsarah]

Replacement of file documenting Windows wrappers (this is in zetuptoolz.dpatch, but may be too hard to read in the diffs).

[davidsarah]

I decided not to mess about with sys.path:

• the two approaches are roughly as complicated overall, but I prefer the complication to be in the installer (scriptsetup.py) rather than the script wrappers, and that approach was already implemented;
• another advantage of using .pyscript is that the user doesn't have any reason to override that association, whereas they might want to override .py.

[davidsarah]

Replying to zooko:

Here is a proposed NEWS snippet for this ticket:

 - Make the "tahoe" command-line tool work on 64-bit Windows and
   support unicode arguments and outputs on Windows (#1074)

That looks fine, but s/unicode/Unicode/ . Also reference #565 and #1073.

[davidsarah]

Replying to zooko:

The current ticket1074 branch has a couple of interesting failures on my Mac OS 10.6 "zomp":

Answered in 1137#comment:7.

[davidsarah]

setuptools/command/easy_install.py: if site.py does not start with the 'def boot():' signature, warn rather than failing the installation. (This is a patch to zetuptools, not Tahoe-LAFS.)

[davidsarah]

setuptools/command/easy_install.py: if site.py does not start with the 'def boot():' signature, warn (and do not overwrite it) rather than failing the installation.

[david-sarah@…]

In [4627/ticket798]:

Bundle setuptools-0.6c16dev (with Windows script changes, and the change to only warn if site.py wasn't generated by setuptools) instead of 0.6c15dev. addresses #565, #1073, #1074

[davidsarah]

Fixed, leaving open for review.

The bundled zetuptoolz was updated to 0.6c16dev on trunk in c045241a55056848 d112df80ff4b0265 58cce54ada16c012 e16c794f192f0cd7. These correspond to ​584 ​586 ​588 on zetuptoolz trunk, and are documented in ​zetuptoolz.txt and ​win_script_wrapper.txt.

The changes to Tahoe-LAFS to take account of the zetuptoolz changes and enable Unicode arguments on Windows are 37b07a545f17f8bb 9d04b2a317c2ecf4 1fad717aaae6382c 54bbc5325f9dae8b ec6a674a5c87fd55 b5f052a0d7abfd55 1a5a33886d771db6 88b6c57a1c12187f d81e8acf882a7784. (Sorry, those should probably have been refactored a bit more before committing them to trunk.)

[davidsarah]

Here's a ​combined view of the Tahoe-LAFS changes (excluding d81e8acf882a7784).

[davidsarah]

This ticket introduced some newly written code that calls Windows APIs from Python, which is tricky and error-prone:

• src/allmydata/windows/fixups.py​, which sets up Unicode output streams and retrieves Unicode command-line arguments.
• setuptools-0.6c16dev3.egg/setuptools/command/scriptsetup.py, which changes the registry in order to add ".pyscript" and ".pyw" to the PATHEXT environment variable and associate ".pyscript" with the Python interpreter.

Ideally these would be reviewed by someone familiar with Python on Windows.

[davidsarah]

Replying to zooko:

1. the "deep magic" in windows_fixups() seems like it should be offered to setuptools, distribute, and/or python core teams via their bug trackers,

Later (less buggy :-) versions of the console output and Unicode argument fixes have been submitted to Python issues ​1602 and ​2128. I'm not entirely sure they belong in setuptools or distribute (that would have to be implemented by putting this code in the Windows wrapper script, and I think it's probably better that application authors run it explicitly).

[davidsarah]

Replying to davidsarah:

• setuptools-0.6c16dev3.egg/setuptools/command/scriptsetup.py, which changes the registry in order to add ".pyscript" and ".pyw" to the PATHEXT environment variable and associate ".pyscript" with the Python interpreter.

#1302 (installing Python 3 breaks bin\tahoe on Windows) is evidence that I didn't think carefully enough about this code :-( -- although I think it would be too disruptive to fix that issue for 1.8.2.

[zooko]

I've reviewed this. I didn't understand the Windows-specific parts so I asked sneves to look at that. I didn't really study the code carefully to look for errors but I read through it and it looked good to me.

[sneves]

Let me say that the comments are really helpful in understanding this patch (in particular what bugs/issues are being worked around; it'd be impossible to figure it out otherwise).

I went through the code (particularly the Win32 portions) and was unable to find anything blatantly wrong with it. It seems to be working, too.