49122 2010-11-05 20:33:05 -0700 webkitpy/tool/* unittests change cwd and don't clean up properly 2010-11-06 19:44:29 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 dpranke dpranke abarth eric mihaip oldest_to_newest 305711 0 dpranke 2010-11-05 20:33:05 -0700 In the course of merging and preparing to land the patch for bug 48280 , I stumbled into this. various unit tests under webkitpy/tool/* change the current working directory and don't seem to change back when they're done. For example, download_unittest.test_apply_attachment creates a MockSCM() object, and then invokes a series of steps including steps/cleanworkingdirectory.py (which has the effect of changing to /private/tmp ). This by itself isn't that bad. However, the real scm checkout code uses the current working directory to figure out where the checkout root is, in default_scm(): def default_scm(): cwd = os.getcwd() scm_system = detect_scm_system(cwd) if not scm_system: script_directory = os.path.abspath(sys.path[0]) scm_system = detect_scm_system(script_directory) if scm_system: log("The current directory (%s) is not a WebKit checkout, using %s" % (cwd, scm_system.checkout_root)) else: error("FATAL: Failed to determine the SCM system for either %s or %s" % (cwd, script_directory)) return scm_system The first part fails, so we fall back to os.path.abspath(sys.path[0]). This by itself also isn't that bad. Normally, in python, sys.path[0] is defined to be the directory of the script that was invoked (i.e., test-webkitpy, so WebKitTools/Scripts) . Sadly, in the monkeying with paths that we do now in test-webkit that seems to have something to do with making sure that QueueStatusServer is in our path, we call out to the WebKitTools/QueueStatusServer/__init__.py:fix_sys_path() which manipulates sys.path . Even that's not so bad. Sadly, that code also calls google_appengine.fix_sys_path(). And THAT code, at least in the version I happen to have installed on my machine (1.3.8), *prepends* EXTRA_PATHS to sys.paths, which breaks the assumption that sys.path[0] will be inside the tree. So, if any test code then attempts to call the real scm.default_scm() code, it will fail. Which happens after the above unittests execute (and we've changed the directory), when we run commands/rebaseline_unittest, when that code constructs a layout_tests/port object, which calls the real scm code when I've merged in the changes in 48280. A comedy of cascading errors, so to speak. There are several things that can/should be fixed here. (1) The AppEngine code is probably wrong. (2) the tool/commands unittests should not actually be changing directories. (3) It may be bad to be relying only on sys.path[0] and cwd, since clearly both of those things can be outside the tree. The only other mechanism I can think of which should be safe would be to look at __file__ from inside scm.py . I think there are times when even that doesn't work right, because there was a reason I started using sys.path[0] instead of __file__ , but those reasons may not apply in this usage. I don't remember exactly what they were. (4) Arguably the rebaseline_unittest command should be trying to get a test port, not a real port, and it should be passing in a MockConfig() object so that we don't use the real scm detection code. But there might need to be other stuff that changes in that test, I haven't really explored this yet. I think (3) is safe, so I'll upload a patch for that. 305713 1 73161 dpranke 2010-11-05 20:35:57 -0700 Created attachment 73161 Patch 305714 2 73161 eric 2010-11-05 20:41:31 -0700 Comment on attachment 73161 Patch Hmmm... I'm not sure this is right. There was a reason why I used sys.path[0], but now I'm not sure I remember. The bug title doesn't seem to align with what the change actually does. :) 305749 3 eric 2010-11-05 23:11:40 -0700 Sorry, I didn't read your long bug description before reviewing the first time. 305752 4 eric 2010-11-05 23:16:53 -0700 (In reply to comment #0) > In the course of merging and preparing to land the patch for bug 48280 , I stumbled into this. > > various unit tests under webkitpy/tool/* change the current working directory and don't seem to change back when they're done. For example, download_unittest.test_apply_attachment creates a MockSCM() object, and then invokes a series of steps including steps/cleanworkingdirectory.py (which has the effect of changing to /private/tmp ). Yeah, we have to be careful to use MockSCM().checkout_root and avoid using os.getcwd() as otherwise tests may or may not have manipulated the cwd. > This by itself isn't that bad. > > However, the real scm checkout code uses the current working directory to figure out where the checkout root is, in default_scm(): > > def default_scm(): > cwd = os.getcwd() > scm_system = detect_scm_system(cwd) > if not scm_system: > script_directory = os.path.abspath(sys.path[0]) > scm_system = detect_scm_system(script_directory) > if scm_system: > log("The current directory (%s) is not a WebKit checkout, using %s" % (cwd, scm_system.checkout_root)) > else: > error("FATAL: Failed to determine the SCM system for either %s or %s" % (cwd, script_directory)) > return scm_system > > The first part fails, so we fall back to os.path.abspath(sys.path[0]). > > This by itself also isn't that bad. Normally, in python, sys.path[0] is defined to be the directory of the script that was invoked (i.e., test-webkitpy, so WebKitTools/Scripts) . > > Sadly, in the monkeying with paths that we do now in test-webkit that seems to have something to do with making sure that QueueStatusServer is in our path, we call out to the WebKitTools/QueueStatusServer/__init__.py:fix_sys_path() which manipulates sys.path . I *just* added this fix_sys_path code. I should probably remove it and or fix it. It seems to be the major problem here. > Even that's not so bad. Sadly, that code also calls google_appengine.fix_sys_path(). And THAT code, at least in the version I happen to have installed on my machine (1.3.8), *prepends* EXTRA_PATHS to sys.paths, which breaks the assumption that sys.path[0] will be inside the tree. > > So, if any test code then attempts to call the real scm.default_scm() code, it will fail. Which happens after the above unittests execute (and we've changed the directory), when we run commands/rebaseline_unittest, when that code constructs a layout_tests/port object, which calls the real scm code when I've merged in the changes in 48280. Hmm.. seems slightly fishy that we use real scm code... > A comedy of cascading errors, so to speak. Agreed. :) > There are several things that can/should be fixed here. > > (1) The AppEngine code is probably wrong. > > (2) the tool/commands unittests should not actually be changing directories. > > (3) It may be bad to be relying only on sys.path[0] and cwd, since clearly both of those things can be outside the tree. The only other mechanism I can think of which should be safe would be to look at __file__ from inside scm.py . I think there are times when even that doesn't work right, because there was a reason I started using sys.path[0] instead of __file__ , but those reasons may not apply in this usage. I don't remember exactly what they were. Yeah. :) webkit-patch tries hard to make it so that you can have webkit-patch in your path, but use it with a different checkout. As long as that use case doesn't beak, we can basically do whatever we want here. :) > (4) Arguably the rebaseline_unittest command should be trying to get a test port, not a real port, and it should be passing in a MockConfig() object so that we don't use the real scm detection code. But there might need to be other stuff that changes in that test, I haven't really explored this yet. > > I think (3) is safe, so I'll upload a patch for that. I'd like to better understand the contract behind sys.path[0] before r+'ing this. I should go read some python docs and see if I can figure out how we ended up using sys.path[0]. Maybe other things will break now as a result of this new AppEngine code? 305755 5 eric 2010-11-05 23:20:18 -0700 http://docs.python.org/library/sys.html#sys.path "As initialized upon program startup, the first item of this list, path[0], is the directory containing the script that was used to invoke the Python interpreter. If the script directory is not available (e.g. if the interpreter is invoked interactively or if the script is read from standard input), path[0] is the empty string, which directs Python to search modules in the current directory first. Notice that the script directory is inserted before the entries inserted as a result of PYTHONPATH." It seems changing to __file__ here is safe since assuming path[0] is a valid directory from which the python interpreter was invoked sounds like it will break. If we're invoking webkit-patch inside another checkout the previous detect_scm_system(cwd) will have already succeeded, so __file__ seems fine. Sorry you had to run down this goose. I think we'll need to consider some of the other fixes you mention as well for future patches. 305835 6 dpranke 2010-11-06 19:31:06 -0700 Filed bug http://code.google.com/p/googleappengine/issues/detail?id=4031 against the app engine SDK as well. 305836 7 73161 dpranke 2010-11-06 19:31:13 -0700 Comment on attachment 73161 Patch Clearing flags on attachment: 73161 Committed r71472: <http://trac.webkit.org/changeset/71472> 305837 8 dpranke 2010-11-06 19:31:18 -0700 All reviewed patches have been landed. Closing bug. 305839 9 dpranke 2010-11-06 19:44:29 -0700 Committed r71473: <http://trac.webkit.org/changeset/71473> 73161 2010-11-05 20:35:57 -0700 2010-11-06 19:31:13 -0700 Patch bug-49122-20101105203556.patch text/plain 1354 dpranke ZGlmZiAtLWdpdCBhL1dlYktpdFRvb2xzL0NoYW5nZUxvZyBiL1dlYktpdFRvb2xzL0NoYW5nZUxv ZwppbmRleCA2Njg3M2Q0OThiYjI0N2NhNjdjYjY3OTFlOWVkNmY2N2YwYmVhMTE3Li4yZTEyM2Zl NDBlMmE3MzY4MTMyMGVlYWNjNzc5ZmZjMzRkZWI4YmNhIDEwMDY0NAotLS0gYS9XZWJLaXRUb29s cy9DaGFuZ2VMb2cKKysrIGIvV2ViS2l0VG9vbHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTMgQEAK KzIwMTAtMTEtMDUgIERpcmsgUHJhbmtlICA8ZHByYW5rZUBjaHJvbWl1bS5vcmc+CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgd2Via2l0cHkvdG9vbC8q IHVuaXR0ZXN0cyBjaGFuZ2UgY3dkIGFuZCBkb24ndCBjbGVhbiB1cCBwcm9wZXJseQorCisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD00OTEyMgorCisgICAg ICAgICogU2NyaXB0cy93ZWJraXRweS9jb21tb24vY2hlY2tvdXQvc2NtLnB5OgorCiAyMDEwLTEx LTA1ICBDaHJpcyBNYXJyaW4gIDxjbWFycmluQGFwcGxlLmNvbT4KIAogICAgICAgICBSZXZpZXdl ZCBieSBTaW1vbiBGcmFzZXIuCmRpZmYgLS1naXQgYS9XZWJLaXRUb29scy9TY3JpcHRzL3dlYmtp dHB5L2NvbW1vbi9jaGVja291dC9zY20ucHkgYi9XZWJLaXRUb29scy9TY3JpcHRzL3dlYmtpdHB5 L2NvbW1vbi9jaGVja291dC9zY20ucHkKaW5kZXggOWI2MDJjMzA1N2ZkZGEyYWNjZjM0Yzc0Mzcx ZDAyZmUyNWIwMDY3Mi4uNzg3MGZmZmRmY2ExMTIwN2I5ZGEwYTM3NWJhODcxNzY0Y2ZhZTZmZSAx MDA2NDQKLS0tIGEvV2ViS2l0VG9vbHMvU2NyaXB0cy93ZWJraXRweS9jb21tb24vY2hlY2tvdXQv c2NtLnB5CisrKyBiL1dlYktpdFRvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL2NoZWNrb3V0 L3NjbS5weQpAQCAtNjQsNyArNjQsNyBAQCBkZWYgZGVmYXVsdF9zY20oKToKICAgICBjd2QgPSBv cy5nZXRjd2QoKQogICAgIHNjbV9zeXN0ZW0gPSBkZXRlY3Rfc2NtX3N5c3RlbShjd2QpCiAgICAg aWYgbm90IHNjbV9zeXN0ZW06Ci0gICAgICAgIHNjcmlwdF9kaXJlY3RvcnkgPSBvcy5wYXRoLmFi c3BhdGgoc3lzLnBhdGhbMF0pCisgICAgICAgIHNjcmlwdF9kaXJlY3RvcnkgPSBvcy5wYXRoLmFi c3BhdGgoX19maWxlX18pCiAgICAgICAgIHNjbV9zeXN0ZW0gPSBkZXRlY3Rfc2NtX3N5c3RlbShz Y3JpcHRfZGlyZWN0b3J5KQogICAgICAgICBpZiBzY21fc3lzdGVtOgogICAgICAgICAgICAgbG9n KCJUaGUgY3VycmVudCBkaXJlY3RvcnkgKCVzKSBpcyBub3QgYSBXZWJLaXQgY2hlY2tvdXQsIHVz aW5nICVzIiAlIChjd2QsIHNjbV9zeXN0ZW0uY2hlY2tvdXRfcm9vdCkpCg==