Bug 89616 - nrwt: use scm instead of calling svn directly to get the revision in json results generator
Summary: nrwt: use scm instead of calling svn directly to get the revision in json res...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords: NRWT
Depends on:
Blocks:
 
Reported: 2012-06-20 16:44 PDT by Dirk Pranke
Modified: 2012-09-04 00:14 PDT (History)
7 users (show)

See Also:


Attachments
Using detect_scm_system (1.60 KB, patch)
2012-08-06 01:47 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (9.06 KB, patch)
2012-08-31 06:46 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2012-06-20 16:44:35 PDT
nrwt: call executive.run_command instead of subprocess.popen to generate json results
Comment 1 Dirk Pranke 2012-06-20 16:47:47 PDT
note that we need to make sure that this works for chromium checkouts (since this code is used for chromium tests that aren't webkit checkouts); possibly that's why it didn't work before.
Comment 2 Ojan Vafai 2012-06-21 07:56:00 PDT
I think the problem before was that the SCM code didn't work correctly under cygwin SVN. IIRC that has been fixed.
Comment 3 Dirk Pranke 2012-06-26 18:38:59 PDT
We wouldn't care about cygwin svn for chromium, I don't think, since we don't run cygwin svn on the bots (just win32 svn). Users might be running cygwin svn, but they presumably wouldn't be generating json results.

OTOH, apple win does use cygwin svn, so we should presumably make sure it works there.
Comment 4 Dirk Pranke 2012-07-13 19:30:25 PDT
resetting the owner in case someone else wants to take a look, as these bugs aren't on my immediate to-do list.
Comment 5 Zan Dobersek 2012-08-06 01:47:28 PDT
Created attachment 156625 [details]
Using detect_scm_system

The current code checks for the .svn subdirectory in the LayoutTests directory. This doesn't work out well if using Subversion 1.7 or newer. OTOH, using detect_scm_system to detect the correct SCM also makes this work when Git is being used.

Is this approach possible or are there some other caveats?
Comment 6 Zan Dobersek 2012-08-21 11:36:44 PDT
(In reply to comment #5)
> Is this approach possible or are there some other caveats?

One I just discovered is the resulting test-webkitpy failures.

[917/1577] webkitpy.layout_tests.run_webkit_tests_integrationtest.MainTest.test_repeat_each_iterations_num_tests erred:
  Traceback (most recent call last):
...
    File "/home/zan/Dev/webkit/weekend/Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py", line 388, in _get_svn_revision
      scm = detect_scm_system(in_directory, filesystem=self._filesystem)
    File "/home/zan/Dev/webkit/weekend/Tools/Scripts/webkitpy/common/checkout/scm/detection.py", line 81, in detect_scm_system
      return SCMDetector(filesystem, Executive()).detect_scm_system(path, patch_directories)
    File "/home/zan/Dev/webkit/weekend/Tools/Scripts/webkitpy/common/checkout/scm/detection.py", line 69, in detect_scm_system
      if SVN.in_working_directory(absolute_path):
    File "/home/zan/Dev/webkit/weekend/Tools/Scripts/webkitpy/common/checkout/scm/svn.py", line 93, in in_working_directory
      exit_code = Executive().run_command(svn_info_args, cwd=path, return_exit_code=True)
    File "/home/zan/Dev/webkit/weekend/Tools/Scripts/webkitpy/common/system/executive.py", line 402, in run_command
      close_fds=self._should_close_fds())
    File "/home/zan/Dev/webkit/weekend/Tools/Scripts/webkitpy/common/system/executive.py", line 458, in popen
      return subprocess.Popen(*args, **kwargs)
    File "/usr/lib/python2.7/subprocess.py", line 679, in __init__
      errread, errwrite)
    File "/usr/lib/python2.7/subprocess.py", line 1249, in _execute_child
      raise child_exception
  OSError: [Errno 2] No such file or directory: '/test.checkout/LayoutTests'

common.checkout.scm.svn runs commands on the real filesystem and not the mock filesystem used in settings.
Comment 7 Zan Dobersek 2012-08-31 06:46:19 PDT
Created attachment 161689 [details]
Patch
Comment 8 Eric Seidel (no email) 2012-08-31 10:04:28 PDT
Comment on attachment 161689 [details]
Patch

LGTM.
Comment 9 WebKit Review Bot 2012-08-31 12:48:37 PDT
Comment on attachment 161689 [details]
Patch

Clearing flags on attachment: 161689

Committed r127302: <http://trac.webkit.org/changeset/127302>
Comment 10 WebKit Review Bot 2012-08-31 12:48:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Ojan Vafai 2012-08-31 18:22:36 PDT
This caused me to get the following error whenever I run the tests. I have a WebKit checkout embedded in a Chromium checkout. Don't know if that matters. Also, I'm on Ubuntu Precise. It's failing to run because /work/chromium/src/build does not exist. Not sure where that path is coming from.

ScriptError raised: Failed to run "[u'git', u'log', u'-25', u'/work/chromium/src/build']" exit_code: 128
    File "/work/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py", line 110, in run
      unexpected_result_count = manager.run(args)
    File "/work/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py", line 450, in run
      self._upload_json_files(summarized_results, result_summary, individual_test_timings)
    File "/work/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py", line 570, in _upload_json_files
      self._options.master_name)
    File "/work/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_package/json_layout_results_generator.py", line 76, in __init__
      self.generate_json_output()
    File "/work/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py", line 244, in generate_json_output
      json_object = self.get_json()
    File "/work/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py", line 284, in get_json
      self._insert_generic_metadata(results_for_builder)
    File "/work/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py", line 529, in _insert_generic_metadata
      self._get_svn_revision(path),
    File "/work/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py", line 391, in _get_svn_revision
      return scm.svn_revision(in_directory)
    File "/work/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 250, in svn_revision
      git_log = self.run([self.executable_name, 'log', '-25', path])
    File "/work/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm.py", line 76, in run
      decode_output=decode_output)
    File "/work/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 423, in run_command
      (error_handler or self.default_error_handler)(script_error)
    File "/work/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 340, in default_error_handler
      raise error
Failed to execute Tools/Scripts/new-run-webkit-tests at /work/chromium/src/third_party/WebKit/Tools/Scripts/run-webkit-tests line 124.
Comment 12 Dirk Pranke 2012-08-31 19:21:15 PDT
That path is coming from chromium.py's implementation of repository_paths() ... how can you have a chromium checkout that doesn't have src/build ? 

(I just tripped over this myself, but this was from running the chromium port in a webkit checkout that had a hand-copied set of binaries and not a full chromium checkout).
Comment 13 Kenichi Ishibashi 2012-09-02 21:10:01 PDT
(In reply to comment #12)
> That path is coming from chromium.py's implementation of repository_paths() ... how can you have a chromium checkout that doesn't have src/build ? 
> 
> (I just tripped over this myself, but this was from running the chromium port in a webkit checkout that had a hand-copied set of binaries and not a full chromium checkout).

I have the same problem. src/build is exists, but git log fails because src/build is outside repository when you put unmanaged WebKit repository in chromium's third_party directory. I have a patch to fix this. Will upload soon.
Comment 14 Zan Dobersek 2012-09-03 23:54:40 PDT
I think the patch is OK but the error occurs as expected in such conditions (copying the WebKit checkout into Chromium checkout and similar).

I'm not sure what a proper fix would be, though. Seeing that using different checkouts together is common, that should definitely be taken into account.
Comment 15 noel gordon 2012-09-04 00:05:58 PDT
Could we roll out until we have a proper fix?
Comment 16 Kenichi Ishibashi 2012-09-04 00:14:00 PDT
(In reply to comment #15)
> Could we roll out until we have a proper fix?

I posted a proposal fix. https://bugs.webkit.org/show_bug.cgi?id=95662