Bug 208726

Summary: Add script to search for feature defines
Product: WebKit Reporter: Don Olmstead <don.olmstead>
Component: Tools / TestsAssignee: Don Olmstead <don.olmstead>
Status: ASSIGNED ---    
Severity: Normal CC: ews-watchlist, glenn, jbedard, mcatanzaro
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
WIP Patch
WIP Patch
WIP Patch none

Description Don Olmstead 2020-03-06 11:39:32 PST
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.
Comment 1 Don Olmstead 2020-03-06 12:54:47 PST
Created attachment 392760 [details]
WIP Patch
Comment 2 Jonathan Bedard 2020-03-06 13:06:29 PST
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
Comment 3 Jonathan Bedard 2020-03-06 13:47:18 PST
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.
Comment 4 Don Olmstead 2020-03-09 11:38:59 PDT
Created attachment 393059 [details]
WIP Patch

Updating with feedback
Comment 5 Don Olmstead 2020-03-09 19:46:36 PDT
Created attachment 393111 [details]
WIP Patch

Fix some issues
Comment 6 Jonathan Bedard 2020-03-10 08:27:45 PDT
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

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