If the local repository is git based rather than svn based, then the following 4 tests fail when running test-webkitpy --all as follows. In each case, an attempt is being made to invoke 'svn' directly. % test-webkitpy --all Suppressing most webkitpy logging while running unit tests. [128/1584] webkitpy.common.checkout.scm.scm_unittest.SVNTest.test_add_recursively erred: Traceback (most recent call last): File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 825, in test_add_recursively self._shared_test_add_recursively() File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 466, in _shared_test_add_recursively self.scm.add("added_dir/added_file") File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm.py", line 134, in add self.add_list([path], return_exit_code) File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/svn.py", line 187, in add_list return self._run_svn(["add"] + paths, return_exit_code=return_exit_code) File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/svn.py", line 137, in _run_svn return self.run([self.executable_name] + args, **kwargs) File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm.py", line 78, in run decode_output=decode_output) File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/system/executive.py", line 432, in run_command (error_handler or self.default_error_handler)(script_error) File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/system/executive.py", line 350, in default_error_handler raise error ScriptError: Failed to run "['svn', 'add', 'added_dir/added_file']" exit_code: 1 [143/1584] webkitpy.common.checkout.scm.scm_unittest.SVNTest.test_delete_recursively erred: Traceback (most recent call last): File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 839, in test_delete_recursively self._shared_test_delete_recursively() File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 472, in _shared_test_delete_recursively self.scm.add("added_dir/added_file") File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm.py", line 134, in add self.add_list([path], return_exit_code) File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/svn.py", line 187, in add_list return self._run_svn(["add"] + paths, return_exit_code=return_exit_code) File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/svn.py", line 137, in _run_svn return self.run([self.executable_name] + args, **kwargs) File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm.py", line 78, in run decode_output=decode_output) File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/system/executive.py", line 432, in run_command (error_handler or self.default_error_handler)(script_error) File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/system/executive.py", line 350, in default_error_handler raise error ScriptError: Failed to run "['svn', 'add', 'added_dir/added_file']" exit_code: 1 [144/1584] webkitpy.common.checkout.scm.scm_unittest.SVNTest.test_delete_recursively_or_not erred: Traceback (most recent call last): File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 842, in test_delete_recursively_or_not self._shared_test_delete_recursively_or_not() File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 481, in _shared_test_delete_recursively_or_not self.scm.add("added_dir/added_file") File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm.py", line 134, in add self.add_list([path], return_exit_code) File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/svn.py", line 187, in add_list return self._run_svn(["add"] + paths, return_exit_code=return_exit_code) File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/svn.py", line 137, in _run_svn return self.run([self.executable_name] + args, **kwargs) File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm.py", line 78, in run decode_output=decode_output) File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/system/executive.py", line 432, in run_command (error_handler or self.default_error_handler)(script_error) File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/system/executive.py", line 350, in default_error_handler raise error ScriptError: Failed to run "['svn', 'add', 'added_dir/added_file']" exit_code: 1 [495/1584] webkitpy.common.checkout.scm.scm_unittest.SVNTest.test_svn_lock failed: Traceback (most recent call last): File "/Users/glenn/work/webkit/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py", line 897, in test_svn_lock self.assertRaises(ScriptError, run_command, ['svn', 'update']) AssertionError: ScriptError not raised Ran 1584 tests in 39.856s FAILED (failures=1, errors=3)
Further investigation shows this isn't an issue with git-svn vs svn.
Turns out these fails are caused by changes introduced by svn client version 1.7: * .svn/lock no longer used * svn add needs --parents option to add intermediate directories, and returns exit code 1 if a dir/file has already been added
Created attachment 197465 [details] Patch
Comment on attachment 197465 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=197465&action=review > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:894 > + if self.scm.svn_version() > "1.6": Why don't we convert the svn version to float otherwise, we'll be doing a string comparison. Also, it's probably better to do >= 1.7 instead. > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:895 > + # .svn/lock isn't used with svn client 1.7 and later I would say "subversion client" or "svn", not "svn client". > Tools/Scripts/webkitpy/common/checkout/scm/svn.py:187 > + if self.svn_version() < "1.7": Ditto about casting it to float first. > Tools/Scripts/webkitpy/common/checkout/scm/svn.py:198 > + if return_exit_code: > + return exit_code > + if exit_code and exit_code != 1: > + raise ScriptError(script_args=svn_add_args, exit_code=exit_code) We might need to return 0 when the exit code is 1 and return_exit_code is set True for the compatibility.
(In reply to comment #4) > (From update of attachment 197465 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=197465&action=review > > > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:894 > > + if self.scm.svn_version() > "1.6": > > Why don't we convert the svn version to float otherwise, we'll be doing a string comparison. > Also, it's probably better to do >= 1.7 instead. The problem is that the version string is normally three parts, e.g., "1.7.8" is returned from my current version. Also there was existing code that did a string comparison: see SVN._status_regexp(). > > > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:895 > > + # .svn/lock isn't used with svn client 1.7 and later > > I would say "subversion client" or "svn", not "svn client". OK, i can change that comment before committing. > > > Tools/Scripts/webkitpy/common/checkout/scm/svn.py:187 > > + if self.svn_version() < "1.7": > > Ditto about casting it to float first. See above. > > > Tools/Scripts/webkitpy/common/checkout/scm/svn.py:198 > > + if return_exit_code: > > + return exit_code > > + if exit_code and exit_code != 1: > > + raise ScriptError(script_args=svn_add_args, exit_code=exit_code) > > We might need to return 0 when the exit code is 1 and return_exit_code is set True for the compatibility. * SCM.add() calls add_list but returns no value regardless of what return_exit_code is passed. * MockSCM.add() does the same thing * BaselineOptimizer._move_baselines ignores the return value from add_list * AbstractParallelRebaselineCommand._rebaseline also ignores So no caller of add_list is actually using the return value, thus I see no compat issue.
(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 197465 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=197465&action=review > > > > > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:894 > > > + if self.scm.svn_version() > "1.6": > > > > Why don't we convert the svn version to float otherwise, we'll be doing a string comparison. > > Also, it's probably better to do >= 1.7 instead. > > The problem is that the version string is normally three parts, e.g., "1.7.8" is returned from my current version. Also there was existing code that did a string comparison: see SVN._status_regexp(). As for using >= "1.7", you are right; i will also change that prior to commit; i.e., "1.6.1" is lexically greater than "1.6" but I don't want the test skipped in that case.
Created attachment 197498 [details] Patch
(In reply to comment #5) > (In reply to comment #4) > > * SCM.add() calls add_list but returns no value regardless of what return_exit_code is passed. > * MockSCM.add() does the same thing > * BaselineOptimizer._move_baselines ignores the return value from add_list > * AbstractParallelRebaselineCommand._rebaseline also ignores > > So no caller of add_list is actually using the return value, thus I see no compat issue. Why don't we get rid of return_exit_code in that case? Sounds like this argument is completely useless.
(In reply to comment #8) > (In reply to comment #5) > > (In reply to comment #4) > > > > * SCM.add() calls add_list but returns no value regardless of what return_exit_code is passed. > > * MockSCM.add() does the same thing > > * BaselineOptimizer._move_baselines ignores the return value from add_list > > * AbstractParallelRebaselineCommand._rebaseline also ignores > > > > So no caller of add_list is actually using the return value, thus I see no compat issue. > > Why don't we get rid of return_exit_code in that case? Sounds like this argument is completely useless. I could do that, but it is unrelated to the bug fix. I don't like to make opportunistic changes that are unrelated to a fix.
(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #5) > > > (In reply to comment #4) > > > > > > * SCM.add() calls add_list but returns no value regardless of what return_exit_code is passed. > > > * MockSCM.add() does the same thing > > > * BaselineOptimizer._move_baselines ignores the return value from add_list > > > * AbstractParallelRebaselineCommand._rebaseline also ignores > > > > > > So no caller of add_list is actually using the return value, thus I see no compat issue. > > > > Why don't we get rid of return_exit_code in that case? Sounds like this argument is completely useless. > > I could do that, but it is unrelated to the bug fix. I don't like to make opportunistic changes that are unrelated to a fix. Also, it is effectively part of the base SCM class API that is implemented by both git.py and svn.py.
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > (In reply to comment #5) > > > > (In reply to comment #4) > > > > > > > > * SCM.add() calls add_list but returns no value regardless of what return_exit_code is passed. > > > > * MockSCM.add() does the same thing > > > > * BaselineOptimizer._move_baselines ignores the return value from add_list > > > > * AbstractParallelRebaselineCommand._rebaseline also ignores > > > > > > > > So no caller of add_list is actually using the return value, thus I see no compat issue. > > > > > > Why don't we get rid of return_exit_code in that case? Sounds like this argument is completely useless. > > > > I could do that, but it is unrelated to the bug fix. I don't like to make opportunistic changes that are unrelated to a fix. > > Also, it is effectively part of the base SCM class API that is implemented by both git.py and svn.py. But we're adding a code that nobody uses in practice. That seems like a bad idea.
Comment on attachment 197498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=197498&action=review On my second thought, we shouldn't be adding code that nobody really uses. r-. > Tools/Scripts/webkitpy/common/checkout/scm/svn.py:196 > + if return_exit_code: > + return exit_code Please mention, in the change log, it's okay to return 1 in some cases.
(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > (In reply to comment #8) > > > > (In reply to comment #5) > > > > > (In reply to comment #4) > > > > > > > > > > * SCM.add() calls add_list but returns no value regardless of what return_exit_code is passed. > > > > > * MockSCM.add() does the same thing > > > > > * BaselineOptimizer._move_baselines ignores the return value from add_list > > > > > * AbstractParallelRebaselineCommand._rebaseline also ignores > > > > > > > > > > So no caller of add_list is actually using the return value, thus I see no compat issue. > > > > > > > > Why don't we get rid of return_exit_code in that case? Sounds like this argument is completely useless. > > > > > > I could do that, but it is unrelated to the bug fix. I don't like to make opportunistic changes that are unrelated to a fix. > > > > Also, it is effectively part of the base SCM class API that is implemented by both git.py and svn.py. > > But we're adding a code that nobody uses in practice. That seems like a bad idea. I don't understand this comment. The return_exit_code parameter was there. I did nothing to change it. I merely ensured that the semantics of the change would work consistently with that pre-existing API, whether it was used or not.
(In reply to comment #13) > I don't understand this comment. The return_exit_code parameter was there. I did nothing to change it. I merely ensured that the semantics of the change would work consistently with that pre-existing API, whether it was used or not. In WebKit, we don't normally fix code that's never used. If we find out that some feature or API is never used, we try to delete that part of the API as soon as possible.
(In reply to comment #12) > (From update of attachment 197498 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=197498&action=review > > On my second thought, we shouldn't be adding code that nobody really uses. r-. I don't understand this comment. We aren't adding code that nobody really uses. We are maintaining an existing API's semantics without changing it. Someone else put the argument there in the first place. I can't say if it was used in the past and that usage was removed or if they anticipated its use in the future. Simplifying or changing the API is out of scope of the fix, and should not be merged into this patch IMO. If you want me to do a follow on patch that changes this API, I can do that, but I don't want to merge it with this patch since it is unrelated. > > > Tools/Scripts/webkitpy/common/checkout/scm/svn.py:196 > > + if return_exit_code: > > + return exit_code > > Please mention, in the change log, it's okay to return 1 in some cases. OK.
(In reply to comment #15) > > Simplifying or changing the API is out of scope of the fix, and should not be merged into this patch IMO. If you want me to do a follow on patch that changes this API, I can do that, but I don't want to merge it with this patch since it is unrelated. Then please remove the argument first.
(In reply to comment #14) > (In reply to comment #13) > > I don't understand this comment. The return_exit_code parameter was there. I did nothing to change it. I merely ensured that the semantics of the change would work consistently with that pre-existing API, whether it was used or not. > > In WebKit, we don't normally fix code that's never used. If we find out that some feature or API is never used, we try to delete that part of the API as soon as possible. I don't understand this comment. I'm not fixing code that's never used. I'm fixing three failed tests that fail because this code doesn't take into account subversion 1.7 semantics. As I said in my prior comment, I'd be happy to do another, follow-on patch that changes the API, but I don't want to merge it with this patch/bug since it serves an entirely different purpose.
Created attachment 197509 [details] Patch
Comment on attachment 197509 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=197509&action=review > Tools/ChangeLog:7 > + I think you should have a word about why the test were failing on 1.7 and later. > Tools/Scripts/webkitpy/common/checkout/scm/svn.py:194 > + if exit_code and exit_code != 1: Maybe just my coding style but to avoid magic numbers I would do: added_tracked_file_exit_code = 1 if exit_code and exit_code != added_tracked_file_exit_code:
Created attachment 197512 [details] Patch for landing
Comment on attachment 197512 [details] Patch for landing Clearing flags on attachment: 197512 Committed r148177: <http://trac.webkit.org/changeset/148177>
All reviewed patches have been landed. Closing bug.