Replace script_path with script_shell_command
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.