Bug 220080 – [Cocoa] Add a script to generate the "Feature Flags" plist file

• Component: New Bugs
• Status: RESOLVED FIXED

[Peng Liu]

[Cocoa] Add a script to generate the "Feature Flags" plist file

[Peng Liu]

Created attachment 416652 [details]
Patch

[Daniel Bates]

Comment on attachment 416652 [details]
Patch

Didn't really look at this patch closely, but it jogged a memory: I think merging plists can be done with: "/usr/libexec/plistbuddy -c merge file2 : file1" or something like that, though it overwrites file1.

[Daniel Bates]

^^^ I think : not needed. I don't know. See man page,

[Peng Liu]

(In reply to Daniel Bates from comment #2)
> Comment on attachment 416652 [details]
> Patch
> 
> Didn't really look at this patch closely, but it jogged a memory: I think
> merging plists can be done with: "/usr/libexec/plistbuddy -c merge file2 :
> file1" or something like that, though it overwrites file1.

Yes, PlistBuddy supports merging two plists, but it skips duplicated keys.

[Daniel Bates]

Comment on attachment 416652 [details]
Patch

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

> Source/WebKit/FeatureFlags/WebKit-ios.plist:60
> +    <key>WebGL2</key>

Ok as is, this key and 2 below don't seem to have same formatting.

> Source/WebKit/Scripts/generate-feature-flags-plist.sh:26
> +if [[ ${WK_PLATFORM_NAME} == "appletvos" ]]; then

Does this work for simulator, too? Same question apples to ios and watch.

> Source/WebKit/Scripts/generate-feature-flags-plist.sh:35
> +    exit 0;

This seems like a programmer error to reach. If so. consider emitting error message and exiting with non-zero return code. Error message should conform to special Xcode syntax to get nice GUI results:

error: Message.

 See <https://stackoverflow.com/questions/7239312/can-i-fully-customise-an-xcode-4-run-script-build-phase-error-warning-within-the> Etc etc.

> Source/WebKit/Scripts/generate-feature-flags-plist.sh:37
> +

Ok as is, consider adding "set -e" so that command failure below causes script to exit immediately.

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:13099
> +				"$(INSTALL_ROOT)/$(WK_INSTALL_PATH_PREFIX)/$(SYSTEM_LIBRARY_DIR)/FeatureFlags/Domain/WebKit.plist",

Ok as is. Is this necessary?

[Daniel Bates]

Comment on attachment 416652 [details]
Patch

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

Forgot to check Python...

> Source/WebKit/Scripts/combine-feature-flags-plist.py:32
> +        return 1

Ok as is. Consider emitting usage message.

> Source/WebKit/Scripts/combine-feature-flags-plist.py:40
> +        try:

Is this correct place for this? What about line above? Consider having script give precise error message when can't open each file. This will make it easier to debug if it fails.

> Source/WebKit/Scripts/combine-feature-flags-plist.py:45
> +                        return 1

Ok as is. Consider emitting error message to help debugging.

> Source/WebKit/Scripts/combine-feature-flags-plist.py:49
> +            pass

Ok as is. Consider emitting error message + exit with non-zero status, Use Xcode special format see earlier comment.

[Daniel Bates]

Comment on attachment 416652 [details]
Patch

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

> Source/WebKit/Scripts/combine-feature-flags-plist.py:1
> +#!/usr/bin/env python

Is this correct for python3?

[Daniel Bates]

Comment on attachment 416652 [details]
Patch

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

> Source/WebKit/Scripts/combine-feature-flags-plist.py:1
> +#!/usr/bin/env python

Is this correct for python3?

[Peng Liu]

(In reply to Daniel Bates from comment #8)
> Comment on attachment 416652 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416652&action=review
> 
> > Source/WebKit/Scripts/combine-feature-flags-plist.py:1
> > +#!/usr/bin/env python
> 
> Is this correct for python3?

Since the shell script runs the python script using the interpreter explicitly, we can remove this line.

[Peng Liu]

Comment on attachment 416652 [details]
Patch

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

>> Source/WebKit/Scripts/combine-feature-flags-plist.py:32
>> +        return 1
> 
> Ok as is. Consider emitting usage message.

Will fix it.

>> Source/WebKit/Scripts/combine-feature-flags-plist.py:40
>> +        try:
> 
> Is this correct place for this? What about line above? Consider having script give precise error message when can't open each file. This will make it easier to debug if it fails.

Agree, will fix it.

>> Source/WebKit/Scripts/generate-feature-flags-plist.sh:26
>> +if [[ ${WK_PLATFORM_NAME} == "appletvos" ]]; then
> 
> Does this work for simulator, too? Same question apples to ios and watch.

No, only supports the four platforms listed here.

>> Source/WebKit/Scripts/generate-feature-flags-plist.sh:35
>> +    exit 0;
> 
> This seems like a programmer error to reach. If so. consider emitting error message and exiting with non-zero return code. Error message should conform to special Xcode syntax to get nice GUI results:
> 
> error: Message.
> 
>  See <https://stackoverflow.com/questions/7239312/can-i-fully-customise-an-xcode-4-run-script-build-phase-error-warning-within-the> Etc etc.

This is the intended behavior. For simulators, we will ignore the plist file.

>> Source/WebKit/Scripts/generate-feature-flags-plist.sh:37
>> +
> 
> Ok as is, consider adding "set -e" so that command failure below causes script to exit immediately.

Good point, will fix it.

>> Source/WebKit/WebKit.xcodeproj/project.pbxproj:13099
>> +				"$(INSTALL_ROOT)/$(WK_INSTALL_PATH_PREFIX)/$(SYSTEM_LIBRARY_DIR)/FeatureFlags/Domain/WebKit.plist",
> 
> Ok as is. Is this necessary?

Yes, it was a mistake to use {} here.

[Peng Liu]

Daniel, thanks for the review!
By the way, what is the current guideline about adding python code? Does the code need to support both python 2 and 3?

[Peng Liu]

Created attachment 416679 [details]
Revise the patch based on Dan's comments

[Peng Liu]

Comment on attachment 416679 [details]
Revise the patch based on Dan's comments

This patch includes a Python3 script.

[Daniel Bates]

Comment on attachment 416679 [details]
Revise the patch based on Dan's comments

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

> Source/WebKit/Scripts/combine-feature-flags-plist.py:1
> +#

Ok as-is. Sorry, I think I caused some confusion with my last review. Consider adding shebang line, but I think shebang line should be: #!/usr/bin/env python3

From <https://docs.python.org/3/using/unix.html>

Also consider making this script executable (maybe it is already - didn't look at raw diff) this way with ^^^ script can be run from command line directly...

> Source/WebKit/Scripts/generate-feature-flags-plist.sh:53
> +    python3 "${SRCROOT}/Scripts/combine-feature-flags-plist.py" "${WEBKIT_PLIST_DIR}/${WEBKIT_PLIST_FILE_NAME}" "${INTERNAL_WEBKIT_PLIST_DIR}/${WEBKIT_PLIST_FILE_NAME}" "${DEST_DIR}/WebKit.plist"

No need for explicit python3 interpreter call if add shebang ...

[Radar WebKit Bug Importer]

<rdar://problem/72715339>

[Peng Liu]

Created attachment 417633 [details]
Patch for landing

[EWS]

Committed r271498: <https://trac.webkit.org/changeset/271498>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417633 [details].