[Cocoa] Add a script to generate the "Feature Flags" plist file
Created attachment 416652 [details] Patch
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.
^^^ I think : not needed. I don't know. See man page,
(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.
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?
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.
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?
(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.
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.
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?
Created attachment 416679 [details] Revise the patch based on Dan's comments
Comment on attachment 416679 [details] Revise the patch based on Dan's comments This patch includes a Python3 script.
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 ...
<rdar://problem/72715339>
Created attachment 417633 [details] Patch for landing
Committed r271498: <https://trac.webkit.org/changeset/271498> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417633 [details].