Created attachment 94344 [details] Patch

Comment on attachment 94344 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94344&action=review > Tools/Scripts/webkitpy/common/checkout/scm/scm.py:94 > + if sys.platform == 'win32': This file shouldn't depend on sys.platform. We're missing some sort of proper abstraction.

Also, we're missing tests.

(In reply to comment #2) > (From update of attachment 94344 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94344&action=review > > > Tools/Scripts/webkitpy/common/checkout/scm/scm.py:94 > > + if sys.platform == 'win32': > > This file shouldn't depend on sys.platform. We're missing some sort of proper abstraction. Any good ideas? Where is the correct/best place?

Comment on attachment 94344 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94344&action=review >>> Tools/Scripts/webkitpy/common/checkout/scm/scm.py:94 >>> + if sys.platform == 'win32': >> >> This file shouldn't depend on sys.platform. We're missing some sort of proper abstraction. > > Any good ideas? Where is the correct/best place? It seems like Executive.interpreter_for_script should do that work, possibly with a different name. Instead of having to branch on sys.platform everywhere we use a script, we should have a common abstraction do that for us.

http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/ports.py#L45 There's another place we have this branch. It should be moved to the common location also.

(In reply to comment #5) > (From update of attachment 94344 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94344&action=review > > >>> Tools/Scripts/webkitpy/common/checkout/scm/scm.py:94 > >>> + if sys.platform == 'win32': > >> > >> This file shouldn't depend on sys.platform. We're missing some sort of proper abstraction. > > > > Any good ideas? Where is the correct/best place? > > It seems like Executive.interpreter_for_script should do that work, possibly with a different name. Instead of having to branch on sys.platform everywhere we use a script, we should have a common abstraction do that for us. Where? Is Executive the right place?

> Where? Is Executive the right place? Dunno. You should apply your design aesthetic and make a decision.

Created attachment 94348 [details] Patch

Comment on attachment 94348 [details] Patch Looks good, but we need a test.

(In reply to comment #10) > (From update of attachment 94348 [details]) > Looks good, but we need a test. Isn't that tested with the apply_patch tests already?

> Isn't that tested with the apply_patch tests already? Seemingly not if this is broken on Windows.

(In reply to comment #12) > > Isn't that tested with the apply_patch tests already? > > Seemingly not if this is broken on Windows. We do not run the test on native windows at the moment, because the don't work. ;-)

> We do not run the test on native windows at the moment, because the don't work. ;-) My point is that unless this change is tested somewhere, I'm likely to break it next time I work on this code. Are we planning to run these tests on native windows? Can we write a test that checks that we're doing the right thing that works on every platform? Presumably we're passing something different to execute, so this should just be a matter of calling the appropriate function with a logging executive.

(In reply to comment #14) > > We do not run the test on native windows at the moment, because the don't work. ;-) > > My point is that unless this change is tested somewhere, I'm likely to break it next time I work on this code. Are we planning to run these tests on native windows? Can we write a test that checks that we're doing the right thing that works on every platform? > > Presumably we're passing something different to execute, so this should just be a matter of calling the appropriate function with a logging executive. IMHO that's a chicken-and-egg problem. We can't run the native win32 test, when the don't work and we can't change the code if we don't test it. :-( The master bug for getting rid of cygwin is 48166. We call the script with interpreter only on windows (sys.platform == 'win32'), so we can't test it on other platform if we don't mock sys. I i don't like the idea of mocking sys. We have the same problem at bug 55925 too.

Hum... Maybe Eric has an opinion. I definitely think this is valuable work, and I don't want to discourage you.

(In reply to comment #15) > We call the script with interpreter only on windows (sys.platform == 'win32'), so we can't test it on other platform if we don't mock sys. I i don't like the idea of mocking sys. Presumably we have code elsewhere that makes platform-dependent decisions. How do we test that code?

> Presumably we have code elsewhere that makes platform-dependent decisions. How do we test that code? We have very, very little platform-dependent code. The layout_package has a bunch, which it tests by using a mock platform object. Maybe the best thing to do here is to run test-webkitpy in a non-cygwin configuration with a skipped list and then gradually remove tests from the skipped list as we fix them. That will ensure that we have test coverage.