Opened 7 years ago

Closed 7 years ago

Last modified 5 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: UI/UX:


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 7 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 Changed 7 years ago by Russell Keith-Magee

milestone: 1.2
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedReady for checkin

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

comment:2 Changed 7 years ago by Karen Tracey

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 --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\", 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\", 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 Changed 7 years ago by Gabriel Hurley

Patch needs improvement: set

comment:4 Changed 7 years ago by Gabriel Hurley

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 Changed 7 years ago by Karen Tracey

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

comment:6 Changed 7 years ago by Gabriel Hurley

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.

Changed 7 years ago by Gabriel Hurley

Attachment: admin_scripts_quoting.diff added

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

comment:7 Changed 7 years ago by Karen Tracey

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 Changed 5 years ago by Jacob

milestone: 1.2

Milestone 1.2 deleted

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