Opened 15 years ago

Closed 15 years ago

Last modified 13 years ago

#13173 closed (fixed)

entire admin_scripts test suite fails on Windows if sys.executable path has a space in it.

Reported by: Gabriel Hurley Owned by: Gabriel Hurley
Component: Core (Management commands) Version: 1.1
Severity: Keywords: admin_scripts
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This is an edge case to be sure, but it sure is annoying if you have Python installed somewhere funny...

If you have Python installed on Windows to a location like "C:\Program Files\python2.6\" then every test in regressiontests.admin_scripts will fail. This is due to the (known) poor escaping done by Popen.

Simply quoting the sys.executable path isn't enough, because if Popen finds a quote at the beginning of args, it strips that quote and automatically assumes it corresponds to a quote at the end of args and will fail with mismatched quotes without realizing what it's done.

The most common solution I could find is to double-quote the args string if you need it to properly handle the extra spaces.

The attached patch currently passes the tests with Python 2.6.4 on Ubuntu and Python 2.6.5 on Windows (both with and without spaces in the sys.executable path).

It's not an incredibly urgent bug, but I figured I'd add a fix here.

Attachments (1)

admin_scripts_quoting.diff (854 bytes ) - added by Gabriel Hurley 15 years ago.
Patch includes smarter quoting to account for spaces in executable path and Popen irregularities.

Download all attachments as: .zip

Change History (9)

comment:1 by Russell Keith-Magee, 15 years ago

milestone: 1.2
Triage Stage: UnreviewedReady for checkin

Bug that causes test suite failures is important; especially when there is a RFC patch.

comment:2 by Karen Tracey, 15 years ago

Triage Stage: Ready for checkinAccepted

With the patch applied, I cannot get the admin_scripts tests to pass if sys.executable does NOT have a space in it. For example:

C:\u\kmt\django\trunk\tests>\bin\Python27\python.exe runtests.py --settings=testdb.sqlite -v1

produces failures like:

======================================================================
FAIL: test_builtin_command (regressiontests.admin_scripts.tests.DjangoAdminAlternateSettings)
alternate: django-admin builtin commands fail with an import error when no settings provided
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\u\kmt\django\trunk\tests\regressiontests\admin_scripts\tests.py", line 400, in test_builtin_command
    self.assertOutput(err, 'environment variable DJANGO_SETTINGS_MODULE is undefined')
  File "C:\u\kmt\django\trunk\tests\regressiontests\admin_scripts\tests.py", line 153, in assertOutput
    self.failUnless(msg in stream, "'%s' does not match actual output text '%s'" % (msg, stream))
AssertionError: 'environment variable DJANGO_SETTINGS_MODULE is undefined' does not match actual output text 'The filename, directory name, or volume label syntax is incorrect.
'

comment:3 by Gabriel Hurley, 15 years ago

Patch needs improvement: set

comment:4 by Gabriel Hurley, 15 years ago

I'm gonna have to take this one back to the drawing board as it seems to be *very* finicky. Quoting for the shell across systems isn't exactly a simple problem. I thought I'd found one that worked since it was happy on the systems I had access to, but apparently not. :-/

comment:5 by Karen Tracey, 15 years ago

Forgot to mention the box with the failures up above is Windows XP. That's the only Windows version I have.

comment:6 by Gabriel Hurley, 15 years ago

Patch needs improvement: unset
Status: newassigned

I was able to reproduce the failure noted by Karen above. I've got a new patch here which is a bit smarter. I've tried it on Windows XP, Ubuntu, and Mac OS X with success so far, so...

Give this one a go and see how it does. It's a massive pain testing all the different system configurations with this thing (since it involves moving Python installs on my machines). I think it's good, but getting a few more testers on it would be really good.

by Gabriel Hurley, 15 years ago

Attachment: admin_scripts_quoting.diff added

Patch includes smarter quoting to account for spaces in executable path and Popen irregularities.

comment:7 by Karen Tracey, 15 years ago

Resolution: fixed
Status: assignedclosed

(In [12842]) Fixed #13173: Made the admin_scripts tests pass when the running python executable has a space in its pathname. Thanks gabrielhurley.

comment:8 by Jacob, 13 years ago

milestone: 1.2

Milestone 1.2 deleted

Note: See TracTickets for help on using tickets.
Back to Top