53822 2011-02-04 15:56:59 -0800 test-webkitpy crashes with a WindowsError when run under Win32 Python 2024-09-23 19:15:15 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) PC Windows XP RESOLVED FIXED InRadar, PlatformOnly P2 Normal --- 53854 48728 55811 1 tony tony abarth aroben commit-queue eric evan fujii webkit-bug-importer webkit.review.bot oldest_to_newest 345877 0 tony 2011-02-04 15:56:59 -0800 get test-webkitpy running on win32 python 345878 1 81312 tony 2011-02-04 15:59:05 -0800 Created attachment 81312 Patch 345879 2 tony 2011-02-04 15:59:34 -0800 For reference, here's the current bot output: http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29/builds/8789/steps/webkitpy-test/logs/stdio 345881 3 81312 eric 2011-02-04 16:12:30 -0800 Comment on attachment 81312 Patch I guess. Shell is *evil*. I suspect this will cause other troubles. 345883 4 81312 eric 2011-02-04 16:13:31 -0800 Comment on attachment 81312 Patch Should we do our own path resolution instead? I feel like at some other point I needed/wanted a "which" command written in python. I guess if we don't use a shell, we won't get the environment in which PATH is defined anyway. 345898 5 evan 2011-02-04 16:27:03 -0800 FWIW, I would guess eseidel's dislike of going through a shell because of quoting issues, but on on Windows the low-level process creation API takes a string so you need to get quoting right anyway (http://msdn.microsoft.com/en-us/library/ms682425%28VS.85%29.aspx). Python implicitly converts the array of args to a quoted command line for you on Windows. Passing shell=True on Windows is pretty standard in Python code I see, though I admit it's all within a small circle of people. 345904 6 tony 2011-02-04 16:30:25 -0800 Mid-air conflict while I was writing pretty much what Evan wrote. The only thing I have to add is that since we expect subprocess.Popen to include the environment on other platforms, it seems to be semantically consistent to for us to use shell=True on Windows. Although in practice, maybe we don't need the environment to be copied to subprocesses. *shrug* 345914 7 eric 2011-02-04 16:35:14 -0800 We've talked about creating our own POpen subclass to fix some of the popen bugs we work-around in Executive. This might be further (mild) motivation for such. 345937 8 aroben 2011-02-04 17:22:14 -0800 I don't remember running into this problem back when I was working on getting test-webkitpy working with Win32 Python. But test-webkitpy has undoubtedly changed since then. 345938 9 aroben 2011-02-04 17:23:49 -0800 The subprocess docs say this: The only reason you would need to specify shell=True on Windows is where the command you wish to execute is actually built in to the shell, eg dir, copy. You don’t need shell=True to run a batch file, nor to run a console-based executable. http://docs.python.org/library/subprocess.html#using-the-subprocess-module So…are the docs wrong? Is there some other reason you'd have to pass shell=True on Windows? 345992 10 81312 commit-queue 2011-02-04 18:39:14 -0800 Comment on attachment 81312 Patch Clearing flags on attachment: 81312 Committed r77720: <http://trac.webkit.org/changeset/77720> 345993 11 commit-queue 2011-02-04 18:39:20 -0800 All reviewed patches have been landed. Closing bug. 346010 12 commit-queue 2011-02-04 19:18:37 -0800 The commit-queue encountered the following flaky tests while processing attachment 81312: http/tests/websocket/tests/send-throw.html bug 53834 (author: abarth@webkit.org) The commit-queue is continuing to process your patch. 346021 13 webkit.review.bot 2011-02-04 19:43:24 -0800 http://trac.webkit.org/changeset/77720 might have broken GTK Linux 64-bit Debug 346618 14 tony 2011-02-07 09:22:11 -0800 (In reply to comment #9) > The subprocess docs say this: > > The only reason you would need to specify shell=True on Windows is where the command you wish to execute is actually built in to the shell, eg dir, copy. You don’t need shell=True to run a batch file, nor to run a console-based executable. > > http://docs.python.org/library/subprocess.html#using-the-subprocess-module > > So…are the docs wrong? Is there some other reason you'd have to pass shell=True on Windows? The docs are correct, it just doesn't enumerate all the reasons to use the shell on Windows. We needed it to search the path for things like 'svn' or 'ruby'. Here's the python bug tracking the difference: http://bugs.python.org/issue8557 Anyway, this was rolled out because when shell=True and the command isn't found, we raise ScriptError rather then OSError. 346725 15 eric 2011-02-07 12:44:10 -0800 ScriptError was originally creaed as part of scm.py to indicate when git/svn returned non-zero. These days it's used for more than that I fear. But still we should return OSError when we can't find a command instead of ScriptError. 346781 16 tony 2011-02-07 14:10:26 -0800 (In reply to comment #15) > But still we should return OSError when we can't find a command instead of ScriptError. Hmm, I'm not sure how to do this. The return code is 1, but that doesn't always mean that the command wasn't found. I could search the command output to see if the command wasn't found, but I suspect for non-English versions of Windows, the text will differ. Eric's original suggestion of writing our own path resolution is possible (it's a cross product of PATH and PATHEXT), but I worry that we'll get it wrong. E.g., is it possible for other environment variables to be in %PATH%? What are the rules for case insensitivity? 2062598 17 fujii 2024-09-23 19:14:23 -0700 no problem with Windows Python nowadays. 2062599 18 webkit-bug-importer 2024-09-23 19:15:15 -0700 <rdar://problem/136549155> 81312 2011-02-04 15:59:05 -0800 2011-02-04 18:39:14 -0800 Patch bug-53822-20110204160033.patch text/plain 2824 tony ZGlmZiAtLWdpdCBhL1Rvb2xzL0NoYW5nZUxvZyBiL1Rvb2xzL0NoYW5nZUxvZwppbmRleCA0MWJi Y2JjMzc5N2Y5ZGFlMGZlMmZmMjk0YjFmMDcxNjE2ZDFiYWUxLi5jODEzYWZkZGZkNzc4ZWQ4NjY0 ZjhkZWZiMzhjNzI0NmU4NzkyOGJjIDEwMDY0NAotLS0gYS9Ub29scy9DaGFuZ2VMb2cKKysrIGIv VG9vbHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTEtMDItMDQgIFRvbnkgQ2hhbmcg IDx0b255QGNocm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMh KS4KKworICAgICAgICBnZXQgdGVzdC13ZWJraXRweSBydW5uaW5nIG9uIHdpbjMyIHB5dGhvbgor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTM4MjIKKwor ICAgICAgICBUaGUgdGVzdCBoYXJuZXNzIGNyYXNoZXMgd2l0aCBhIFdpbmRvd3NFcnJvciBiZWNh dXNlIGl0IGNhbid0IGZpbmQKKyAgICAgICAgJ3N2bicgd2hlbiB1c2luZyBzdWJwcm9jZXNzLlBv cGVuLiAgVGhpcyBnZXRzIHVzIHBhc3QgdGhlIGVycm9yCisgICAgICAgIHNvIHdlIGNhbiBzZWUg dGhlIGZhaWxpbmcgdGVzdHMgb24gdGhlIENocm9taXVtIFdpbiBSZWxlYXNlIFRlc3RzCisgICAg ICAgIGJvdC4KKworICAgICAgICAqIFNjcmlwdHMvd2Via2l0cHkvY29tbW9uL3N5c3RlbS9leGVj dXRpdmUucHk6CisKIDIwMTEtMDItMDQgIE1pa2hhaWwgTmFnYW5vdiAgPG1uYWdhbm92QGNocm9t aXVtLm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBQYXZlbCBGZWxkbWFuLgpkaWZmIC0tZ2l0 IGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9jb21tb24vc3lzdGVtL2V4ZWN1dGl2ZS5weSBiL1Rv b2xzL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL3N5c3RlbS9leGVjdXRpdmUucHkKaW5kZXggMDI2 MTlkYjRkNWQ3YjQxYmQ3NzQ0YzQ2ZWE5NDk1MzY1OWRlZTJiMy4uOTE1NTM1MGU4MDZiY2FlMjU5 MDliY2VjNzlhNTkwYWZlMGIxN2ZkZCAxMDA2NDQKLS0tIGEvVG9vbHMvU2NyaXB0cy93ZWJraXRw eS9jb21tb24vc3lzdGVtL2V4ZWN1dGl2ZS5weQorKysgYi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5 L2NvbW1vbi9zeXN0ZW0vZXhlY3V0aXZlLnB5CkBAIC0xMTEsNiArMTExLDExIEBAIGNsYXNzIEV4 ZWN1dGl2ZShvYmplY3QpOgogICAgICAgICAjIHNob3dzIHVwIG9uIE1hYyBhbmQgTGludXguCiAg ICAgICAgIHJldHVybiBzeXMucGxhdGZvcm0gbm90IGluICgnd2luMzInLCAnY3lnd2luJykKIAor ICAgIGRlZiBfc2hvdWxkX3VzZV9zaGVsbChzZWxmKToKKyAgICAgICAgIyBPbiBXaW5kb3dzLCBp ZiB3ZSBkb24ndCB1c2UgdGhlIHNoZWxsLCB3ZSBkb24ndCBzZWFyY2ggJVBBVEglIHRvCisgICAg ICAgICMgZmluZCB0aGUgY29tbWFuZC0tIGFuIGFic29sdXRlIHBhdGggaXMgcmVxdWlyZWQuCisg ICAgICAgIHJldHVybiBzeXMucGxhdGZvcm0uc3RhcnRzd2l0aCgnd2luJykKKwogICAgIGRlZiBf cnVuX2NvbW1hbmRfd2l0aF90ZWVkX291dHB1dChzZWxmLCBhcmdzLCB0ZWVkX291dHB1dCk6CiAg ICAgICAgIGFyZ3MgPSBtYXAodW5pY29kZSwgYXJncykgICMgUG9wZW4gd2lsbCB0aHJvdyBhbiBl eGNlcHRpb24gaWYgYXJncyBhcmUgbm9uLXN0cmluZ3MgKGxpa2UgaW50KCkpCiAgICAgICAgIGFy Z3MgPSBtYXAoc2VsZi5fZW5jb2RlX2FyZ3VtZW50X2lmX25lZWRlZCwgYXJncykKQEAgLTExOCw3 ICsxMjMsOCBAQCBjbGFzcyBFeGVjdXRpdmUob2JqZWN0KToKICAgICAgICAgY2hpbGRfcHJvY2Vz cyA9IHN1YnByb2Nlc3MuUG9wZW4oYXJncywKICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgc3Rkb3V0PXN1YnByb2Nlc3MuUElQRSwKICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgc3RkZXJyPXN1YnByb2Nlc3MuU1RET1VULAotICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBjbG9zZV9mZHM9c2VsZi5fc2hvdWxkX2Ns b3NlX2ZkcygpKQorICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBjbG9z ZV9mZHM9c2VsZi5fc2hvdWxkX2Nsb3NlX2ZkcygpLAorICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICBzaGVsbD1zZWxmLl9zaG91bGRfdXNlX3NoZWxsKCkpCiAKICAgICAg ICAgIyBVc2Ugb3VyIG93biBjdXN0b20gd2FpdCBsb29wIGJlY2F1c2UgUG9wZW4gaWdub3JlcyBh IHRlZSdkCiAgICAgICAgICMgc3RkZXJyL3N0ZG91dC4KQEAgLTM1MSw3ICszNTcsOCBAQCBjbGFz cyBFeGVjdXRpdmUob2JqZWN0KToKICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg c3Rkb3V0PXN1YnByb2Nlc3MuUElQRSwKICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgc3RkZXJyPXN0ZGVyciwKICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgY3dk PWN3ZCwKLSAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgY2xvc2VfZmRzPXNlbGYu X3Nob3VsZF9jbG9zZV9mZHMoKSkKKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg Y2xvc2VfZmRzPXNlbGYuX3Nob3VsZF9jbG9zZV9mZHMoKSwKKyAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgc2hlbGw9c2VsZi5fc2hvdWxkX3VzZV9zaGVsbCgpKQogICAgICAgICBv dXRwdXQgPSBwcm9jZXNzLmNvbW11bmljYXRlKHN0cmluZ190b19jb21tdW5pY2F0ZSlbMF0KIAog ICAgICAgICAjIHJ1bl9jb21tYW5kIGF1dG9tYXRpY2FsbHkgZGVjb2RlcyB0byB1bmljb2RlKCkg dW5sZXNzIGV4cGxpY2l0bHkgdG9sZCBub3QgdG8uCg==