Bug 185643 - run-gtk-tests (glib/common.py) cannot determine build directory when webKitBranchBuild=true
Summary: run-gtk-tests (glib/common.py) cannot determine build directory when webKitBr...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 187904
Blocks:
  Show dependency treegraph
 
Reported: 2018-05-15 00:09 PDT by Frédéric Wang (:fredw)
Modified: 2018-07-23 23:34 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.32 KB, patch)
2018-05-24 06:13 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix GTK+ build (2.40 KB, patch)
2018-05-24 07:24 PDT, Carlos Garcia Campos
fred.wang: review+
Details | Formatted Diff | Diff
Patch for landing (8.16 KB, patch)
2018-06-20 05:56 PDT, Carlos Garcia Campos
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews204 for win-future (12.78 MB, application/zip)
2018-06-20 19:51 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2018-05-15 00:09:00 PDT
If webKitBranchBuild=true and your build directory is e.g. WebKitBuild/MyBranchName then Tools/Scripts/run-gtk-tests fails with error "Could not determine build directory". If you have already built the master branch in WebKitBuild/ then you don't get this error and probably the tests are executed with the wrong build. This is where get_build_path() is called:

 File "Tools/Scripts/run-gtk-tests", line 142, in <module>
    runner = GtkTestRunner(options, args)
  File "Tools/Scripts/run-gtk-tests", line 38, in __init__
    super(GtkTestRunner, self).__init__("gtk", options, tests)
  File "./Tools/glib/api_test_runner.py", line 47, in __init__
    self._programs_path = common.binary_build_path()
  File "./Tools/glib/common.py", line 53, in binary_build_path
    binary_build_dir = build_path('bin', *args)
  File "./Tools/glib/common.py", line 104, in build_path
    return os.path.join(*(get_build_path(),) + args)

For people hitting this problem, here is a quick workaround:

diff --git a/Tools/glib/common.py b/Tools/glib/common.py
index 8a7b2fb465b..43dca365733 100644
--- a/Tools/glib/common.py
+++ b/Tools/glib/common.py
@@ -77,7 +77,7 @@ def get_build_path(fatal=True):
 
     global build_types
     for build_type in build_types:
-        build_dir = top_level_path('WebKitBuild', build_type)
+        build_dir = top_level_path('WebKitBuild', "MyBranchName", build_type)
         if is_valid_build_directory(build_dir):
             return build_dir
Comment 1 Carlos Garcia Campos 2018-05-24 06:13:08 PDT
Created attachment 341184 [details]
Patch
Comment 2 Carlos Garcia Campos 2018-05-24 07:24:01 PDT
Created attachment 341187 [details]
Try to fix GTK+ build
Comment 3 Carlos Garcia Campos 2018-05-31 05:19:47 PDT
Ping reviewers.
Comment 4 Adrian Perez 2018-05-31 06:39:16 PDT
Comment on attachment 341187 [details]
Try to fix GTK+ build

View in context: https://bugs.webkit.org/attachment.cgi?id=341187&action=review

Looks good to me, with a nit :)

> Tools/glib/common.py:24
> +tools_dir = os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..', 'Tools', 'Scripts'))

I would rather use os.path.dirname() to navigate upwards in the
file system tree instead of using “..” path components, as in:

    tools_dir = abspath(join(dirname(dirname(dirname(__file__))), 'Tools', 'Scripts'))

The reason is that “..” is not guaranteed to be “one directory upwards in the
tree” in all systems (well, it is in Windows and Unix, which is what we support
after all, but using “dirname” is the correct thing).
Comment 5 Michael Catanzaro 2018-05-31 06:56:31 PDT
Rumor has it Frédéric Wang is a WebKit reviewer :O
Comment 6 Frédéric Wang (:fredw) 2018-05-31 23:56:34 PDT
Comment on attachment 341187 [details]
Try to fix GTK+ build

View in context: https://bugs.webkit.org/attachment.cgi?id=341187&action=review

LGTM, but please at least address the case of the "master" branch as it is very common.

>> Tools/glib/common.py:24
>> +tools_dir = os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..', 'Tools', 'Scripts'))
> 
> I would rather use os.path.dirname() to navigate upwards in the
> file system tree instead of using “..” path components, as in:
> 
>     tools_dir = abspath(join(dirname(dirname(dirname(__file__))), 'Tools', 'Scripts'))
> 
> The reason is that “..” is not guaranteed to be “one directory upwards in the
> tree” in all systems (well, it is in Windows and Unix, which is what we support
> after all, but using “dirname” is the correct thing).

What Adrián says make sense and I guess you can also do the same for the top_level_path function. Actually, it seems you just want to do tools_dir = top_level_path("Tools", "Scripts")?

> Tools/glib/common.py:94
> +

It looks like the definition of boolean for git config is more generic (see https://git-scm.com/docs/git-config#git-config-boolean) and from a quick test, read_git_config does not put things into canonical form, contrary to the bash command used in Tools/Scripts/VCSUtils.pm ; so we'll have to the conversion ourselves.

Also, when _current_branch() == "master", we actually just use WebKitBuild/ not WebKitBuild/master so we don't need to modify base_build_dir.

I wonder if you want to move some of this logic into git.py, in case we want to re-use it elsewhere.
Comment 7 Michael Catanzaro 2018-06-01 08:44:43 PDT
I wish the build directory was also namespaced by port, moreso than branch.
Comment 8 Carlos Garcia Campos 2018-06-20 05:56:19 PDT
Created attachment 343154 [details]
Patch for landing
Comment 9 Build Bot 2018-06-20 19:50:58 PDT
Comment on attachment 343154 [details]
Patch for landing

Attachment 343154 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8271680

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html
Comment 10 Build Bot 2018-06-20 19:51:09 PDT
Created attachment 343203 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 11 Carlos Garcia Campos 2018-06-21 01:54:14 PDT
Committed r233030: <https://trac.webkit.org/changeset/233030>
Comment 12 Tomas Popela 2018-07-23 01:05:08 PDT
We are not able to compile 2.21.5 from tarball after this, see https://bugs.webkit.org/show_bug.cgi?id=187900
Comment 13 WebKit Commit Bot 2018-07-23 07:10:11 PDT
Re-opened since this is blocked by bug 187904
Comment 14 Carlos Garcia Campos 2018-07-23 23:34:38 PDT
What's the point of rolling this out? It only affects tarballs, 2.21.5 is not going to be fixed by rolling this out, whatever we do to fix it, will be available in the next release in any case.