RESOLVED FIXED 200787
run-webkit-test: Allow results to be uploaded without scm checkout
https://bugs.webkit.org/show_bug.cgi?id=200787
Summary run-webkit-test: Allow results to be uploaded without scm checkout
Matt Lewis
Reported 2019-08-15 13:40:37 PDT
run-webkit-test: Allow results to be uploaded without scm checkout
Attachments
Patch (2.19 KB, patch)
2019-08-15 15:44 PDT, Matt Lewis
no flags
Patch (8.31 KB, patch)
2019-08-20 15:38 PDT, Matt Lewis
no flags
Patch (9.07 KB, patch)
2019-08-21 17:43 PDT, Matt Lewis
no flags
Patch (7.91 KB, patch)
2019-08-23 11:40 PDT, Matt Lewis
no flags
Patch (7.89 KB, patch)
2019-08-26 16:22 PDT, Matt Lewis
no flags
Patch (12.11 KB, patch)
2019-08-29 08:43 PDT, Matt Lewis
no flags
Patch (12.27 KB, patch)
2019-09-05 18:33 PDT, Matt Lewis
no flags
Patch (12.51 KB, patch)
2019-09-06 09:17 PDT, Matt Lewis
no flags
Patch (12.59 KB, patch)
2019-09-06 09:31 PDT, Matt Lewis
no flags
Patch (12.44 KB, patch)
2019-09-06 09:48 PDT, Matt Lewis
no flags
Patch (12.54 KB, patch)
2019-09-10 13:39 PDT, Matt Lewis
no flags
Patch (13.24 KB, patch)
2019-09-10 14:19 PDT, Matt Lewis
no flags
Patch (13.26 KB, patch)
2019-09-10 15:22 PDT, Matt Lewis
no flags
Radar WebKit Bug Importer
Comment 1 2019-08-15 15:06:00 PDT
Matt Lewis
Comment 2 2019-08-15 15:44:48 PDT
Alexey Proskuryakov
Comment 3 2019-08-15 16:23:56 PDT
Comment on attachment 376435 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376435&action=review > Tools/Scripts/webkitpy/port/base.py:1617 > + if os.path.exists(os.path.join(path, 'repo.json')): This is an abbreviation, and not a very descriptive name. Would “checkout-information.json” be accurate?
Matt Lewis
Comment 4 2019-08-15 16:27:30 PDT
(In reply to Alexey Proskuryakov from comment #3) > Comment on attachment 376435 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=376435&action=review > > > Tools/Scripts/webkitpy/port/base.py:1617 > > + if os.path.exists(os.path.join(path, 'repo.json')): > > This is an abbreviation, and not a very descriptive name. Would > “checkout-information.json” be accurate? That is probably far more appropriate.
Jonathan Bedard
Comment 5 2019-08-15 16:58:15 PDT
Comment on attachment 376435 [details] Patch Turns out, this won't work because there are two places we do this. I talked to Matt about moving this json file check into the detect_scm_system codepath, and I think that's what we're going to need to do.
Matt Lewis
Comment 6 2019-08-20 15:38:22 PDT
Jonathan Bedard
Comment 7 2019-08-20 15:55:05 PDT
Comment on attachment 376821 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376821&action=review > Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:83 > + return json_data['id'] This should probably call native_revision(...)
Matt Lewis
Comment 8 2019-08-21 17:43:42 PDT
Created attachment 376956 [details] Patch Work towards fixing a json and scm detection error
Matt Lewis
Comment 9 2019-08-23 11:40:23 PDT
Created attachment 377141 [details] Patch Work toward consistently running
Matt Lewis
Comment 10 2019-08-26 16:22:33 PDT
Matt Lewis
Comment 11 2019-08-26 16:23:32 PDT
Tested the changes with the stub repository and regular checkouts and worked locally.
Jonathan Bedard
Comment 12 2019-08-26 16:37:41 PDT
Comment on attachment 377284 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377284&action=review > Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:61 > + def in_working_directory(cls, path, executive=None): What do the Git/SVN classes say if you give them something like OpenSource/Tools? Does 'in_working_directory' return true or false?
Matt Lewis
Comment 13 2019-08-26 17:02:41 PDT
(In reply to Jonathan Bedard from comment #12) > Comment on attachment 377284 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=377284&action=review > > > Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:61 > > + def in_working_directory(cls, path, executive=None): > > What do the Git/SVN classes say if you give them something like > OpenSource/Tools? Does 'in_working_directory' return true or false? Well based on the classes they return True if the .svn directory is in the path or they run the svn and/or git commands and look at the output to find out if they are in a repo. Otherwise they catch an exception out and return False.
Matt Lewis
Comment 14 2019-08-26 17:03:13 PDT
(In reply to Matt Lewis from comment #13) > (In reply to Jonathan Bedard from comment #12) > > Comment on attachment 377284 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=377284&action=review > > > > > Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:61 > > > + def in_working_directory(cls, path, executive=None): > > > > What do the Git/SVN classes say if you give them something like > > OpenSource/Tools? Does 'in_working_directory' return true or false? > > Well based on the classes they return True if the .svn directory is in the > path or they run the svn and/or git commands and look at the output to find > out if they are in a repo. Otherwise they catch an exception out and return > False. Based on my understanding of the classes*
Matt Lewis
Comment 15 2019-08-29 08:43:21 PDT
Created attachment 377593 [details] Patch Added unittesting for stub repository.
Jonathan Bedard
Comment 16 2019-08-29 09:14:08 PDT
Comment on attachment 377593 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377593&action=review > Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:38 > +from .scm import AuthenticationError, SCM, commit_error_handler We don't need AuthenticationError or commit_error_handler > Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:43 > +class StubRepository(SCM): High level comment: Since every other SCM's in_worker_directory works recursively, this one should to. You could just duplicate the loop logic, but something like this might be more elegant: @classmethod def _run_at_root(cls, path, callback, filesystem=None): if not filesystem: filesystem = SystemHost().filesystem while path: if filesystem.isfile(filesystem.join(path, 'checkout_information.json')): return callback(path, filesystem) path = filesystem.dirname(path) return None Then, your functions would look like this: @classmethod def in_working_directory(cls, path, executive=None, filesystem=None): return bool(cls._run_at_root(path, lambda path, filesystem: True, filesystem=filesystem)) ... def svn_revision(self, path): return self._run_at_root(path, lambda path, filesystem: self._decode_json(path)['id'], filesystem=self._filesystem) > Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:49 > + if filesystem.isfile(filesystem.join(path, 'checkout_information.json')): checkout_information.json should be a class variable > Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:70 > + # self.native_revision(path) What is this comment doing? > Tools/Scripts/webkitpy/common/checkout/scm/stub_repository_unittest.py:41 > + # stub_repository_json = json.loads(checkout_information_json) What is this comment doing? > Tools/Scripts/webkitpy/common/checkout/scm/stub_repository_unittest.py:56 > + self.assertEqual(repository.native_revision(path=repository._filesystem.getcwd()), checkout_information_json['id']) I think this should be host.filesystem.getcwd(), instead of using the private _filesystem variable > Tools/Scripts/webkitpy/common/checkout/scm/stub_repository_unittest.py:71 > + self.assertEquals(repository.find_checkout_root(path=repository._filesystem.getcwd()), host.filesystem.getcwd()) In order to truly test this, you need to change the cwd to some directory within the repository....which you'll probably need to create
Matt Lewis
Comment 17 2019-09-05 18:33:13 PDT
Jonathan Bedard
Comment 18 2019-09-06 08:06:35 PDT
Comment on attachment 378146 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378146&action=review Looks like all the logic is here, just a few nits. > Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:50 > + path = filesystem.dirname(filesystem.abspath(path)) I think we should do path = filesystem.abspath(path) before the loop, so that the first iteration of the loop passes an absolute path to the callback too. If we do that we shouldn't need to call abspath in ever loop iteration. Existing unit tests should easily easily cover this if I'm wrong. > Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:58 > + return self._find_parent_path_matching_callback_condition(path, lambda path: self._decode_json(path)['id'], filesystem=self._filesystem) This should call self.native_revision(...) as to not duplicate code. > Tools/Scripts/webkitpy/common/checkout/scm/stub_repository_unittest.py:31 > + '/TestDirectory/checkout_information.json': '{ "branch": "trunk", "id": "2738499" }', Just do a single indent, don't align with the bracket > Tools/Scripts/webkitpy/common/checkout/scm/stub_repository_unittest.py:66 > + self.assertEquals(repository.find_checkout_root(path=host.join('TestDirectory', 'TestDirectory2')), host.join(host.getcwd(), 'TestDirectory')) Can we test the failure case too? (ie, we pass find_checkout_root '/' and get back None)
Jonathan Bedard
Comment 19 2019-09-06 08:09:27 PDT
Comment on attachment 378146 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378146&action=review > Tools/Scripts/webkitpy/common/checkout/scm/stub_repository_unittest.py:33 > + '/TestDirectory/TestDirectory2/test2.txt': 'test' Missing , at the end of this line
Matt Lewis
Comment 20 2019-09-06 09:17:01 PDT
Matt Lewis
Comment 21 2019-09-06 09:31:18 PDT
Created attachment 378199 [details] Patch I miissed a piece of the patch on the first upload.
Jonathan Bedard
Comment 22 2019-09-06 09:34:17 PDT
Comment on attachment 378199 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378199&action=review > Tools/ChangeLog:33 > + Make sure the first letter of each sentence is capitalized, don't indent 2nd line, always end sentances with a period (even if they aren't completeO) > Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:51 > + path = filesystem.dirname(filesystem.abspath(path)) Probably don't need to re-call abspath every time, although it also isn't going to be harmful. > Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:59 > + # return self._find_parent_path_matching_callback_condition(path, lambda path: self._decode_json(path)['id'], filesystem=self._filesystem) Remove comment
Matt Lewis
Comment 23 2019-09-06 09:48:33 PDT
WebKit Commit Bot
Comment 24 2019-09-06 11:10:55 PDT
Comment on attachment 378202 [details] Patch Clearing flags on attachment: 378202 Committed r249582: <https://trac.webkit.org/changeset/249582>
WebKit Commit Bot
Comment 25 2019-09-06 11:10:57 PDT
All reviewed patches have been landed. Closing bug.
Matt Lewis
Comment 26 2019-09-06 17:42:40 PDT
Reverted r249582 for reason: This caused results.html fail to be created on internal testers. Committed r249602: <https://trac.webkit.org/changeset/249602>
Matt Lewis
Comment 27 2019-09-10 13:39:37 PDT
Created attachment 378481 [details] Patch I've tested the change a bit more and found that we were hitting an loop in the find_checkout_root function that was being triggered due to being passed a path that is 1 level higher than the checkout and never bailing when hitting the root directory.
Matt Lewis
Comment 28 2019-09-10 14:19:20 PDT
Created attachment 378492 [details] Patch Added tests for the find_parent_path_matching_callback_condition function.
Jonathan Bedard
Comment 29 2019-09-10 14:37:13 PDT
Comment on attachment 378492 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378492&action=review > Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:51 > + if path == filesystem.dirname(path): I think we should do this with a temp variable to avoid multiple calls to dirname. Something like this: previous_path = '' path = filesystem.abspath(path) while path and path != previous_path: if filesystem.exists(filesystem.join(path, cls._stub_repository_json)): return callback(path) path = filesystem.dirname(path)
Matt Lewis
Comment 30 2019-09-10 15:22:28 PDT
(In reply to Jonathan Bedard from comment #29) > Comment on attachment 378492 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378492&action=review > > > Tools/Scripts/webkitpy/common/checkout/scm/stub_repository.py:51 > > + if path == filesystem.dirname(path): > > I think we should do this with a temp variable to avoid multiple calls to > dirname. Something like this: > > previous_path = '' > path = filesystem.abspath(path) > > while path and path != previous_path: > if filesystem.exists(filesystem.join(path, cls._stub_repository_json)): > return callback(path) > path = filesystem.dirname(path) This wont work, but I'm uploading a patch that should integrate this idea correctly.
Matt Lewis
Comment 31 2019-09-10 15:22:58 PDT
Jonathan Bedard
Comment 32 2019-09-10 16:19:52 PDT
Comment on attachment 378500 [details] Patch Let's try this again...
Matt Lewis
Comment 33 2019-09-10 17:25:19 PDT
Comment on attachment 378500 [details] Patch Here goes round 2.
WebKit Commit Bot
Comment 34 2019-09-10 18:12:44 PDT
Comment on attachment 378500 [details] Patch Clearing flags on attachment: 378500 Committed r249749: <https://trac.webkit.org/changeset/249749>
WebKit Commit Bot
Comment 35 2019-09-10 18:12:46 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.