#1074 closed defect

get rid of tahoe.exe launcher — at Version 17

Reported by: zooko Owned by: davidsarah
Priority: major Milestone: 1.8β
Component: packaging Version: 1.6.1
Keywords: windows win64 setuptools unicode reviewed Cc: sneves
Launchpad Bug:

Description (last modified by davidsarah)

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".)

Change History (23)

comment:1 Changed at 2010-06-08T18:56:09Z by davidsarah

  • Description modified (diff)
  • Owner changed from somebody to davidsarah
  • Status changed from new to assigned
  • Summary changed from there is a binary (cli.exe) in the Tahoe-LAFS source tree under revision control to get rid of tahoe.exe launcher

Changed at 2010-06-08T23:29:51Z by davidsarah

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

comment:2 Changed at 2010-06-12T20:49:01Z by davidsarah

  • Milestone changed from 1.8.0 to 1.7.1

comment:3 follow-up: Changed at 2010-07-01T03:46:26Z by 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.

comment:4 in reply to: ↑ 3 ; follow-up: Changed at 2010-07-01T19:27:35Z by 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.

comment:5 in reply to: ↑ 4 Changed at 2010-07-01T19:35:37Z by 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.)

comment:6 Changed at 2010-07-08T04:52:19Z by zooko

  • Priority changed from major to critical

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.

comment:7 follow-up: Changed at 2010-07-10T19:48:40Z by 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.

comment:8 in reply to: ↑ 7 Changed at 2010-07-10T21:00:42Z by davidsarah

  • Description modified (diff)
  • Keywords win64 added

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).

comment:9 Changed at 2010-07-10T21:30:53Z by 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.

comment:10 Changed at 2010-07-10T22:28:14Z by 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.

Last edited at 2010-07-10T22:40:44Z by davidsarah (previous) (diff)

Changed at 2010-07-10T22:32:04Z by davidsarah

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

comment:11 Changed at 2010-07-10T22:38:59Z by davidsarah

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

Last edited at 2010-07-10T22:40:19Z by davidsarah (previous) (diff)

comment:12 follow-up: Changed at 2010-07-14T02:35:17Z by 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.

Changed at 2010-07-14T02:39:05Z by davidsarah

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

Changed at 2010-07-14T02:39:49Z by 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.

comment:13 Changed at 2010-07-14T02:40:42Z by davidsarah

  • Keywords review-needed added
  • Owner changed from davidsarah to zooko
  • Status changed from assigned to new

comment:14 Changed at 2010-07-14T02:55:42Z by zooko

  • Status changed from new to assigned

Changed at 2010-07-14T02:59:33Z by davidsarah

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

comment:15 in reply to: ↑ 12 Changed at 2010-07-14T03:07:38Z by 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.

comment:16 follow-up: Changed at 2010-07-14T06:55:11Z by zooko

  • Keywords review-needed removed
  • Owner changed from zooko to davidsarah
  • Status changed from assigned to new

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?

comment:17 in reply to: ↑ 16 Changed at 2010-07-15T03:49:29Z by davidsarah

  • Description modified (diff)

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).

  1. 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.

  1. 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.

Changed at 2010-07-15T05:48:02Z by 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.

Note: See TracTickets for help on using tickets.