[webkit-reviews] review granted: [Bug 185643] run-gtk-tests (glib/common.py) cannot determine build directory when webKitBranchBuild=true : [Attachment 341187] Try to fix GTK+ build

[bugzilla-daemon at webkit.org]

Frédéric Wang (:fredw) <fred.wang at free.fr> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 185643: run-gtk-tests (glib/common.py) cannot determine build directory
when webKitBranchBuild=true
https://bugs.webkit.org/show_bug.cgi?id=185643

Attachment 341187: Try to fix GTK+ build

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




--- Comment #6 from Frédéric Wang (:fredw) <fred.wang at free.fr> ---
Comment on attachment 341187
  --> https://bugs.webkit.org/attachment.cgi?id=341187
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.