Bug 208726 – Add script to search for feature defines

• Status: ASSIGNED
• Component: Tools / Tests

[Don Olmstead]

Its hard to keep the FeatureDefines.xcconfig, WebKitFeatures.cmake and FeatureList.pm synced between the different build systems. ENABLE flags are sometimes removed or added without making sure the usages in build files are modified accordingly. Add a script that allows you to search for these usages in build and source code so they can be added or removed accordingly.

[Don Olmstead]

Created attachment 392760 [details]
WIP Patch

[Jonathan Bedard]

Comment on attachment 392760 [details]
WIP Patch

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

> Tools/Scripts/find-feature-flags.py:1
> +#!/usr/bin/env python

This should be Tools/Scripts/find-feature-flags

[Jonathan Bedard]

Comment on attachment 392760 [details]
WIP Patch

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

> Tools/Scripts/find-feature-flags.py:37
> +_ROOT = os.path.normpath(os.path.join(os.path.dirname(__file__), "..", ".."))

Since this is in Tools/Scripts, not much of a reason to mark things with _ in this file. We usually do that to indicate a variable should be considered private.

> Tools/Scripts/find-feature-flags.py:67
> +    parser.add_argument('find', help='the code to search', choices=_CHOICES, nargs='*')

The help could be better. Maybe something like'The type of configuration file or code to search for feature flags in'

> Tools/Scripts/find-feature-flags.py:68
> +    parser.add_argument('--diff', choices=_CHOICES, action='append')

In my experience, we sort of need to baby people with more powerful options like this, otherwise they will never get used. This deserves a help string.

> Tools/Scripts/webkitpy/featuredefines/matcher.py:30
> +@total_ordering

I'm pretty sure this working in Python 2 and 3, but could you run this script in both to be certain?

> Tools/Scripts/webkitpy/featuredefines/matcher.py:87
> +def usage_matcher(macro='ENABLE'):

I tend to dislike free functions in Python, these, in particular, seem to be good @classmethods for FeatureDefineMatcher (maybe just renamed to Matcher since the import path makes it clear?)

> Tools/Scripts/webkitpy/featuredefines/matcher_unittest.py:1
> +# Copyright (C) 2020 Sony Interactive Entertainment

You need an __init__.py in featuredefines.

[Don Olmstead]

Created attachment 393059 [details]
WIP Patch

Updating with feedback

[Don Olmstead]

Created attachment 393111 [details]
WIP Patch

Fix some issues

[Jonathan Bedard]

Comment on attachment 393111 [details]
WIP Patch

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

> Tools/Scripts/find-feature-defines:1
> +#!/usr/bin/env python

Make this script executable.

> Tools/Scripts/find-feature-defines:33
> +_DESCRIPTION = '''

Make sure that we have a description before landing.

> Tools/Scripts/find-feature-defines:37
> +_ROOT = os.path.normpath(os.path.join(os.path.dirname(__file__), "..", ".."))

We usually don't use underscored variables in the script itself since underscores mark a variable as private and we wouldn't be importing from a script anyways.

> Tools/Scripts/find-feature-defines:72
> +    parser.add_argument('--diff', choices=_CHOICES, action='append')

This need a better description