get rid of tahoe.exe launcher #1074

New issue

Closed

opened 2010-06-08 18:29:39 +00:00 by zooko · 40 comments

zooko commented

2010-06-08 18:29:39 +00:00

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

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

zooko added the

labels 2010-06-08 18:29:39 +00:00

zooko added this to the 1.8.0 milestone 2010-06-08 18:29:39 +00:00

daira changed title from ~~there is a binary (cli.exe) in the Tahoe-LAFS source tree under revision control~~ to get rid of tahoe.exe launcher

2010-06-08 18:56:09 +00:00

daira commented

2010-06-08 23:29:51 +00:00

Attachment unicode-args-and-rm-tahoe-exe-wip.dpatch (63266 bytes) added

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

**Attachment** unicode-args-and-rm-tahoe-exe-wip.dpatch (63266 bytes) added Attaching my work-in-progress for Unicode argument support on Windows and removal of tahoe.exe, so that it doesn't get lost.

unicode-args-and-rm-tahoe-exe-wip.dpatch

62 KiB

daira modified the milestone from 1.8.0 to 1.7.1

2010-06-12 20:49:01 +00:00

zooko commented

2010-07-01 03:46:26 +00:00

Author

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.

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.

daira commented

2010-07-01 19:27:35 +00:00

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.

Replying to [zooko](/tahoe-lafs/trac/issues/1074#issuecomment-378756): > 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.

daira commented

2010-07-01 19:35:37 +00:00

Replying to [davidsarah]comment:4:

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

Replying to [davidsarah]comment:4: > Replying to [zooko](/tahoe-lafs/trac/issues/1074#issuecomment-378756): > > 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 commented

2010-07-08 04:52:19 +00:00

Author

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.

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

zooko added

p/critical

and removed

p/major

labels 2010-07-08 04:52:19 +00:00

zeromus commented

2010-07-10 19:48:40 +00:00

Owner

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.

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.

daira commented

2010-07-10 21:00:42 +00:00

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

Replying to [zeromus](/tahoe-lafs/trac/issues/1074#issuecomment-378762): > 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 commented

2010-07-10 21:30:53 +00:00

Owner

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.

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.

daira commented

2010-07-10 22:28:14 +00:00

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.

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.

daira commented

2010-07-10 22:32:04 +00:00

Attachment zetuptoolz-dont-write-script-exe.2.dpatch (6775 bytes) added

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

**Attachment** zetuptoolz-dont-write-script-exe.2.dpatch (6775 bytes) added zetuptoolz: don't write .exe files for scripts (fixed version)

zetuptoolz-dont-write-script-exe.2.dpatch

6.6 KiB

daira commented

2010-07-10 22:38:59 +00:00

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

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

daira commented

2010-07-14 02:35:17 +00:00

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.

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.

daira commented

2010-07-14 02:39:05 +00:00

Attachment zetuptoolz-dont-write-script-exe.3.dpatch (26851 bytes) added

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

**Attachment** zetuptoolz-dont-write-script-exe.3.dpatch (26851 bytes) added Changes to zetuptoolz to get rid of .exe wrappers, and support Unicode arguments.

zetuptoolz-dont-write-script-exe.3.dpatch

26 KiB

daira commented

2010-07-14 02:39:49 +00:00

Attachment unicode-args-and-rm-tahoe-exe.3.dpatch (21557 bytes) added

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.

**Attachment** unicode-args-and-rm-tahoe-exe.3.dpatch (21557 bytes) added 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.

unicode-args-and-rm-tahoe-exe.3.dpatch

21 KiB

daira removed their assignment 2010-07-14 02:40:42 +00:00

zooko was assigned by daira

2010-07-14 02:40:42 +00:00

daira commented

2010-07-14 02:59:33 +00:00

Attachment pyscript.reg (182 bytes) added

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

**Attachment** pyscript.reg (182 bytes) added regedit file to set up .pyscript association (run with 'regedit pyscript.reg' and accept the scary prompts).

pyscript.reg

182 B

daira commented

2010-07-14 03:07:38 +00:00

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.

Replying to [davidsarah](/tahoe-lafs/trac/issues/1074#issuecomment-378767): > 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 commented

2010-07-14 06:55:11 +00:00

Author

Here are my comments:

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

         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 source: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?

Here are my comments: 1. This requires updates to source: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 source: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 source: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?

zooko removed their assignment 2010-07-14 06:55:11 +00:00

daira was assigned by zooko

2010-07-14 06:55:11 +00:00

daira commented

2010-07-15 03:49:29 +00:00

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:

This requires updates to source: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).

There should be some theory-of-operation doc, perhaps just at the top of the source: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.

    except Exception:
        # okay, probably it was already gone

This could be a narrower catch:

    except [EnvironmentError](wiki/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:

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.

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 source: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 [source:src/allmydata/scripts/runner.py@4329#L42 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.

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](/tahoe-lafs/trac/issues/1074#issuecomment-378771): > Here are my comments: > 1. This requires updates to source: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 source: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](wiki/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 source: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 [source:src/allmydata/scripts/runner.py@4329#L42 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.

daira commented

2010-07-15 05:48:02 +00:00

Attachment unicode-args-and-rm-tahoe-exe.4.dpatch (33575 bytes) added

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.

**Attachment** unicode-args-and-rm-tahoe-exe.4.dpatch (33575 bytes) added 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.

unicode-args-and-rm-tahoe-exe.4.dpatch

33 KiB

daira commented

2010-07-15 05:52:18 +00:00

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 commented

2010-07-15 22:58:56 +00:00

Author

I just updated the wiki/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:378775 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.

I just updated the [wiki/PatchReviewProcess](wiki/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:378775](/tahoe-lafs/trac/issues/1074#issuecomment-378775) that [attachment:unicode-args-and-rm-tahoe-exe.4.dpatch](/tahoe-lafs/trac/attachments/000078ac-1c60-2ebc-50cd-ac6edfb8346f) 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.

daira modified the milestone from 1.7.1 to 1.8β

2010-07-17 03:45:31 +00:00

zooko commented

2010-07-20 05:19:26 +00:00

Author

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)

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 commented

2010-07-20 06:27:36 +00:00

Author

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

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 commented

2010-07-29 15:03:03 +00:00

Author

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

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>

daira commented

2010-07-31 09:22:10 +00:00

Attachment zetuptoolz.dpatch (164966 bytes) added

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

**Attachment** zetuptoolz.dpatch (164966 bytes) added 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.)

zetuptoolz.dpatch

161 KiB

daira commented

2010-07-31 09:26:12 +00:00

Attachment add-zetuptoolz-txt.dpatch (103892 bytes) added

Add zetuptoolz.txt documenting changes made by the fork

**Attachment** add-zetuptoolz-txt.dpatch (103892 bytes) added Add zetuptoolz.txt documenting changes made by the fork

add-zetuptoolz-txt.dpatch

102 KiB

daira commented

2010-07-31 09:27:26 +00:00

Attachment win_script_wrapper.txt (4744 bytes) added

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

**Attachment** win_script_wrapper.txt (4744 bytes) added Replacement of file documenting Windows wrappers (this is in zetuptoolz.dpatch, but may be too hard to read in the diffs).

win_script_wrapper.txt

4.6 KiB

daira commented

2010-07-31 09:39:04 +00:00

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.

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

daira removed their assignment 2010-07-31 09:39:04 +00:00

zooko was assigned by daira

2010-07-31 09:39:04 +00:00

daira commented

2010-07-31 09:42:00 +00:00

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.

Replying to [zooko](/tahoe-lafs/trac/issues/1074#issuecomment-378778): > 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.

daira commented

2010-07-31 09:45:08 +00:00

Replying to zooko:

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

Answered in ticket:1137#comment:378762.

Replying to [zooko](/tahoe-lafs/trac/issues/1074#issuecomment-378780): > The current ticket1074 branch has a couple of interesting failures on my Mac OS 10.6 "zomp": Answered in ticket:1137#[comment:378762](/tahoe-lafs/trac/issues/1074#issuecomment-378762).

daira commented

2010-08-02 05:56:36 +00:00

Attachment zetuptoolz-only-warn-if-not-our-site-py.dpatch (101276 bytes) added

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

**Attachment** zetuptoolz-only-warn-if-not-our-site-py.dpatch (101276 bytes) added 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.)

zetuptoolz-only-warn-if-not-our-site-py.dpatch

99 KiB

daira commented

2010-08-02 06:20:14 +00:00

Attachment zetuptoolz-only-warn-if-not-our-site-py-v2.dpatch (101454 bytes) added

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.

**Attachment** zetuptoolz-only-warn-if-not-our-site-py-v2.dpatch (101454 bytes) added 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.

zetuptoolz-only-warn-if-not-our-site-py-v2.dpatch

99 KiB

daira commented

2010-08-02 07:23:26 +00:00

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

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 ```

daira commented

2010-08-07 22:03:53 +00:00

Fixed, leaving open for review.

The bundled zetuptoolz was updated to 0.6c16dev on trunk in changeset:c045241a55056848 changeset:d112df80ff4b0265 changeset:58cce54ada16c012 changeset: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 changeset:37b07a545f17f8bb changeset:9d04b2a317c2ecf4 changeset:1fad717aaae6382c changeset:54bbc5325f9dae8b changeset:ec6a674a5c87fd55 changeset:b5f052a0d7abfd55 changeset:1a5a33886d771db6 changeset:88b6c57a1c12187f changeset:d81e8acf882a7784. (Sorry, those should probably have been refactored a bit more before committing them to trunk.)

Fixed, leaving open for review. The bundled zetuptoolz was updated to 0.6c16dev on trunk in changeset:c045241a55056848 changeset:d112df80ff4b0265 changeset:58cce54ada16c012 changeset:e16c794f192f0cd7. These correspond to [584](http://tahoe-lafs.org/trac/zetuptoolz/changeset/584/trunk/) [586](http://tahoe-lafs.org/trac/zetuptoolz/changeset/586/trunk/) [588](http://tahoe-lafs.org/trac/zetuptoolz/changeset/588/trunk/) on zetuptoolz trunk, and are documented in [zetuptoolz.txt](http://tahoe-lafs.org/trac/zetuptoolz/browser/trunk/zetuptoolz.txt?rev=587) and [win_script_wrapper.txt](http://tahoe-lafs.org/trac/zetuptoolz/browser/trunk/setuptools/tests/win_script_wrapper.txt). The changes to Tahoe-LAFS to take account of the zetuptoolz changes and enable Unicode arguments on Windows are changeset:37b07a545f17f8bb changeset:9d04b2a317c2ecf4 changeset:1fad717aaae6382c changeset:54bbc5325f9dae8b changeset:ec6a674a5c87fd55 changeset:b5f052a0d7abfd55 changeset:1a5a33886d771db6 changeset:88b6c57a1c12187f changeset:d81e8acf882a7784. (Sorry, those should probably have been refactored a bit more before committing them to trunk.)

daira added

p/major

and removed

p/critical

labels 2010-08-07 22:03:53 +00:00

daira commented

2010-08-07 22:09:32 +00:00

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

Here's a [combined view of the Tahoe-LAFS changes](http://tahoe-lafs.org/trac/tahoe-lafs/changeset?reponame=trunk&new=4623%40%2F&old=4616%40%2F) (excluding changeset:d81e8acf882a7784).

daira commented

2010-12-31 00:22:22 +00:00

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

source:src/allmydata/windows/fixups.py, which sets up Unicode output streams and retrieves Unicode command-line arguments.
source: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.

This ticket introduced some newly written code that calls Windows APIs from Python, which is tricky and error-prone: * source:src/allmydata/windows/fixups.py, which sets up Unicode output streams and retrieves Unicode command-line arguments. * source: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.

daira commented

2011-01-09 08:18:51 +00:00

Replying to zooko:

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

Replying to [zooko](/tahoe-lafs/trac/issues/1074#issuecomment-378771): > 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](http://bugs.python.org/issue1602) and [2128](http://bugs.python.org/issue2128). 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).

daira commented

2011-01-15 00:19:51 +00:00

Replying to davidsarah:

source: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.

Replying to [davidsarah](/tahoe-lafs/trac/issues/1074#issuecomment-378791): > * source: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 commented

2011-02-19 21:40:57 +00:00

Author

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 commented

2011-02-20 03:53:33 +00:00

Owner

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.

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.

daira added the

r/fixed

label 2011-02-20 06:13:49 +00:00

daira closed this issue

2011-02-20 06:13:49 +00:00