Bug 156674 – Build tools should work when the /usr/bin/python is python3

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Drew DeVault]

The build tools fail when the default python is python 3. This is currently worked around by telling the user to change their environment:

https://trac.webkit.org/browser/trunk/Tools/gtk/install-dependencies#L324

A better solution is to either:

- Change the shebangs and such everywhere to #!/usr/bin/env python2, which should work on all systems
- Make the Python code work on both Python 2 and Python 3

Currently working on patches that use the first solution, and then will consider patches that work towards the second solution.

[Alexey Proskuryakov]

FWIW, there is no python2 on OS X.

[Mike Gorse]

Created attachment 335141 [details]
First pass

I needed to be able to build WebKit using python 3, so I made a patch (running 2to3 along with some manual changes).

In some cases, it wasn't clear to me whether files needed to be opened with "wb", since they appear to be text, though I figured that the "b" might be there for a reason and left things as they were, though opening the file as text would allow a str to be passed to write().
Similarly, I'm guessing that there's a better way to handle jsmin.py.

[Michael Catanzaro]

Comment on attachment 335141 [details]
First pass

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

Is SUSE dropping python2 early?

Anyway, I think this patch is good. Maintaining compatibility with both python2 and python3 is going to be painful, but it's obvious that we can't delay this transition much longer, and I assume Apple wants to stick with python2 for now.

> Source/JavaScriptCore/generate-bytecode-files:230
> +        #dowrite(bytecodeHFile, (cCopyrightMsg % bytecodeJSONFile)

This shouldn't be commented out!

[Michael Catanzaro]

(In reply to Mike Gorse from comment #2)
> In some cases, it wasn't clear to me whether files needed to be opened with
> "wb", since they appear to be text, though I figured that the "b" might be
> there for a reason and left things as they were, though opening the file as
> text would allow a str to be passed to write().

Should probably take the opportunity to switch to "wb", indeed.

> Similarly, I'm guessing that there's a better way to handle jsmin.py.

I'm not a python expert. Maybe Konstantin has some opinion on this. I'm OK with uglifying the code in order to support both versions of python at the same time.

Anyway, I'll run this through the early warning service to see how the Mac and Windows bots react... it would be impressive if the first attempt doesn't break anything.

[EWS Watchlist]

Attachment 335141 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_model.py:159:  missing whitespace after ','  [pep8/E231] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_model.py:159:  [BuiltinFunction.__lt__] Method should have "self" as first argument  [pylint/E0213] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_frontend_dispatcher_header.py:49:  [CppFrontendDispatcherHeaderGenerator.domains_to_generate] Instance of 'CppFrontendDispatcherHeaderGenerator' has no 'events_for_domain' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_objc_backend_dispatcher_header.py:50:  [ObjCBackendDispatcherHeaderGenerator.domains_to_generate] Instance of 'ObjCBackendDispatcherHeaderGenerator' has no 'should_generate_commands_for_domain' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_objc_frontend_dispatcher_implementation.py:48:  [ObjCFrontendDispatcherImplementationGenerator.domains_to_generate] Instance of 'ObjCFrontendDispatcherImplementationGenerator' has no 'should_generate_events_for_domain' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/generate-js-builtins.py:56:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Source/JavaScriptCore/Scripts/generateYarrUnicodePropertyTables.py:96:  no spaces around keyword / parameter equals  [pep8/E251] [5]
ERROR: Source/JavaScriptCore/Scripts/generateYarrUnicodePropertyTables.py:220:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_objc_protocol_type_conversions_header.py:54:  [ObjCProtocolTypeConversionsHeaderGenerator.domains_to_generate] Instance of 'ObjCProtocolTypeConversionsHeaderGenerator' has no 'should_generate_types_for_domain' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_backend_dispatcher_header.py:49:  [CppBackendDispatcherHeaderGenerator.domains_to_generate] Instance of 'CppBackendDispatcherHeaderGenerator' has no 'commands_for_domain' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:346:  [CppProtocolTypesHeaderGenerator._generate_forward_declarations_for_binding_traits] Instance of 'CppProtocolTypesHeaderGenerator' has no 'type_needs_shape_assertions' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_backend_dispatcher_implementation.py:48:  [CppBackendDispatcherImplementationGenerator.domains_to_generate] Instance of 'CppBackendDispatcherImplementationGenerator' has no 'commands_for_domain' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_combined_header.py:161:  [BuiltinsCombinedHeaderGenerator.generate_section_for_global_private_code_name_macro] Instance of 'BuiltinsCombinedHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
Total errors found: 13 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

You'll need to fix the EWS bots to not be red. First problem is:

Traceback (most recent call last):
  File "JavaScriptCore/Scripts/generate-js-builtins.py", line 41, in <module>
    from builtins_model import *
ImportError: No module named builtins_model

[Mike Gorse]

(In reply to Michael Catanzaro from comment #3)

> Is SUSE dropping python2 early?

Not for openSUSE that I know of. For SLE 15, it will be part of the legacy module.

> > Source/JavaScriptCore/generate-bytecode-files:230
> > +        #dowrite(bytecodeHFile, (cCopyrightMsg % bytecodeJSONFile)
> 
> This shouldn't be commented out!

Thanks for catching that. I've fixed it in my copy--I probably commented it out while debugging the syntax error and forgot to uncomment.

I need to research EWS--I'd been testing against webkitgtk, but there are problems that I hadn't come across.

[Michael Catanzaro]

The EWS is the bunch of green and red bubbles above the comments on this Bugzilla page. You want them to be green. :) You can attach new speculative patches to try to get closer to successful builds on macOS/iOS/Windows without needing to build for those platforms yourself.

[Mike Gorse]

Created attachment 346638 [details]
Revised patch.

Rebased/updated. Fix erroneously-commented line to write out a copyright. Fixed a few style issues. Hoping I fixed the build on MacOS; renamed builtins to wkbuiltins to avoid a name conflict in python 3. This makes the patch larger than it should be.

[Michael Catanzaro]

Please also use Tools/Scripts/prepare-ChangeLog -b 156674 to generate appropriate ChangeLog entries

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #10)
> Please also use Tools/Scripts/prepare-ChangeLog -b 156674 to generate
> appropriate ChangeLog entries

And mark the patch with the r? cq? Bugzilla flags to request review and commit-queue, respectively.

[Mike Gorse]

Created attachment 347950 [details]
Updated patch.

[Michael Catanzaro]

Comment on attachment 347950 [details]
Updated patch.

Hm, looks like none of the bots were able to apply the patch:

https://webkit-queues.webkit.org/results/8962750

Make sure to use 'git add' to add all moved files to the patch before using 'webkit-patch upload'

[Mike Gorse]

Created attachment 348154 [details]
Patch

[EWS Watchlist]

Attachment 348154 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_wrapper_header.py:41:  [BuiltinsWrapperHeaderGenerator.output_filename] Instance of 'BuiltinsWrapperHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_wrapper_header.py:45:  [BuiltinsWrapperHeaderGenerator.generate_output] Instance of 'BuiltinsWrapperHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_wrapper_header.py:49:  [BuiltinsWrapperHeaderGenerator.generate_output] Instance of 'BuiltinsWrapperHeaderGenerator' has no 'generate_license' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_wrapper_header.py:66:  [BuiltinsWrapperHeaderGenerator.generate_secondary_header_includes] Instance of 'BuiltinsWrapperHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_wrapper_header.py:69:  [BuiltinsWrapperHeaderGenerator.generate_secondary_header_includes] Instance of 'BuiltinsWrapperHeaderGenerator' has no 'generate_includes_from_entries' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_wrapper_header.py:94:  [BuiltinsWrapperHeaderGenerator.generate_constructor] Instance of 'BuiltinsWrapperHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_wrapper_header.py:98:  [BuiltinsWrapperHeaderGenerator.generate_constructor] Instance of 'BuiltinsWrapperHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_wrapper_header.py:108:  [BuiltinsWrapperHeaderGenerator.generate_accessors] Instance of 'BuiltinsWrapperHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_wrapper_header.py:116:  [BuiltinsWrapperHeaderGenerator.generate_members] Instance of 'BuiltinsWrapperHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_separate_header.py:49:  [BuiltinsSeparateHeaderGenerator.macro_prefix] Instance of 'BuiltinsSeparateHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_separate_header.py:53:  [BuiltinsSeparateHeaderGenerator.generate_output] Instance of 'BuiltinsSeparateHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_separate_header.py:62:  [BuiltinsSeparateHeaderGenerator.generate_output] Instance of 'BuiltinsSeparateHeaderGenerator' has no 'generate_license' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_separate_header.py:74:  [BuiltinsSeparateHeaderGenerator.generate_output] Instance of 'BuiltinsSeparateHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_separate_header.py:108:  [BuiltinsSeparateHeaderGenerator.generate_secondary_header_includes] Instance of 'BuiltinsSeparateHeaderGenerator' has no 'generate_includes_from_entries' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_separate_header.py:166:  [BuiltinsSeparateHeaderGenerator.generate_section_for_code_table_macro] Instance of 'BuiltinsSeparateHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_internals_wrapper_header.py:64:  whitespace before ']'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_internals_wrapper_header.py:42:  [BuiltinsInternalsWrapperHeaderGenerator.output_filename] Instance of 'BuiltinsInternalsWrapperHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_internals_wrapper_header.py:46:  [BuiltinsInternalsWrapperHeaderGenerator.generate_output] Instance of 'BuiltinsInternalsWrapperHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_internals_wrapper_header.py:50:  [BuiltinsInternalsWrapperHeaderGenerator.generate_output] Instance of 'BuiltinsInternalsWrapperHeaderGenerator' has no 'generate_license' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_internals_wrapper_header.py:69:  [BuiltinsInternalsWrapperHeaderGenerator.generate_secondary_header_includes] Instance of 'BuiltinsInternalsWrapperHeaderGenerator' has no 'generate_includes_from_entries' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_implementation.py:182:  [CppProtocolTypesImplementationGenerator._generate_builders_for_domain] Instance of 'CppProtocolTypesImplementationGenerator' has no 'type_needs_shape_assertions' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/generateYarrUnicodePropertyTables.py:96:  no spaces around keyword / parameter equals  [pep8/E251] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_separate_implementation.py:49:  [BuiltinsSeparateImplementationGenerator.macro_prefix] Instance of 'BuiltinsSeparateImplementationGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_separate_implementation.py:53:  [BuiltinsSeparateImplementationGenerator.generate_output] Instance of 'BuiltinsSeparateImplementationGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_separate_implementation.py:62:  [BuiltinsSeparateImplementationGenerator.generate_output] Instance of 'BuiltinsSeparateImplementationGenerator' has no 'generate_license' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_separate_implementation.py:64:  [BuiltinsSeparateImplementationGenerator.generate_output] Instance of 'BuiltinsSeparateImplementationGenerator' has no 'generate_primary_header_includes' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_separate_implementation.py:70:  [BuiltinsSeparateImplementationGenerator.generate_output] Instance of 'BuiltinsSeparateImplementationGenerator' has no 'generate_embedded_code_string_section_for_function' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_separate_implementation.py:71:  [BuiltinsSeparateImplementationGenerator.generate_output] Instance of 'BuiltinsSeparateImplementationGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_separate_implementation.py:73:  [BuiltinsSeparateImplementationGenerator.generate_output] Instance of 'BuiltinsSeparateImplementationGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_separate_implementation.py:112:  [BuiltinsSeparateImplementationGenerator.generate_secondary_header_includes] Instance of 'BuiltinsSeparateImplementationGenerator' has no 'generate_includes_from_entries' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_combined_implementation.py:45:  [BuiltinsCombinedImplementationGenerator.output_filename] Instance of 'BuiltinsCombinedImplementationGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_combined_implementation.py:49:  [BuiltinsCombinedImplementationGenerator.generate_output] Instance of 'BuiltinsCombinedImplementationGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_combined_implementation.py:50:  [BuiltinsCombinedImplementationGenerator.generate_output] Instance of 'BuiltinsCombinedImplementationGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_combined_implementation.py:54:  [BuiltinsCombinedImplementationGenerator.generate_output] Instance of 'BuiltinsCombinedImplementationGenerator' has no 'generate_license' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_combined_implementation.py:56:  [BuiltinsCombinedImplementationGenerator.generate_output] Instance of 'BuiltinsCombinedImplementationGenerator' has no 'generate_primary_header_includes' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_combined_implementation.py:59:  [BuiltinsCombinedImplementationGenerator.generate_output] Instance of 'BuiltinsCombinedImplementationGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_combined_implementation.py:60:  [BuiltinsCombinedImplementationGenerator.generate_output] Instance of 'BuiltinsCombinedImplementationGenerator' has no 'generate_embedded_code_string_section_for_function' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_combined_implementation.py:61:  [BuiltinsCombinedImplementationGenerator.generate_output] Instance of 'BuiltinsCombinedImplementationGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_combined_implementation.py:63:  [BuiltinsCombinedImplementationGenerator.generate_output] Instance of 'BuiltinsCombinedImplementationGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_combined_implementation.py:100:  [BuiltinsCombinedImplementationGenerator.generate_secondary_header_includes] Instance of 'BuiltinsCombinedImplementationGenerator' has no 'generate_includes_from_entries' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_objc_backend_dispatcher_header.py:50:  [ObjCBackendDispatcherHeaderGenerator.domains_to_generate] Instance of 'ObjCBackendDispatcherHeaderGenerator' has no 'should_generate_commands_for_domain' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_internals_wrapper_implementation.py:42:  [BuiltinsInternalsWrapperImplementationGenerator.output_filename] Instance of 'BuiltinsInternalsWrapperImplementationGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_internals_wrapper_implementation.py:46:  [BuiltinsInternalsWrapperImplementationGenerator.generate_output] Instance of 'BuiltinsInternalsWrapperImplementationGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_internals_wrapper_implementation.py:50:  [BuiltinsInternalsWrapperImplementationGenerator.generate_output] Instance of 'BuiltinsInternalsWrapperImplementationGenerator' has no 'generate_license' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_internals_wrapper_implementation.py:52:  [BuiltinsInternalsWrapperImplementationGenerator.generate_output] Instance of 'BuiltinsInternalsWrapperImplementationGenerator' has no 'generate_primary_header_includes' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_internals_wrapper_implementation.py:82:  [BuiltinsInternalsWrapperImplementationGenerator.generate_secondary_header_includes] Instance of 'BuiltinsInternalsWrapperImplementationGenerator' has no 'generate_includes_from_entries' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generator.py:135:  whitespace before '}'  [pep8/E202] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_objc_protocol_type_conversions_header.py:54:  [ObjCProtocolTypeConversionsHeaderGenerator.domains_to_generate] Instance of 'ObjCProtocolTypeConversionsHeaderGenerator' has no 'should_generate_types_for_domain' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_backend_dispatcher_header.py:49:  [CppBackendDispatcherHeaderGenerator.domains_to_generate] Instance of 'CppBackendDispatcherHeaderGenerator' has no 'commands_for_domain' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:347:  [CppProtocolTypesHeaderGenerator._generate_forward_declarations_for_binding_traits] Instance of 'CppProtocolTypesHeaderGenerator' has no 'type_needs_shape_assertions' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_backend_dispatcher_implementation.py:48:  [CppBackendDispatcherImplementationGenerator.domains_to_generate] Instance of 'CppBackendDispatcherImplementationGenerator' has no 'commands_for_domain' member  [pylint/E1101] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 52 in 56 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mike Gorse]

Used webkit-patch upload this time after running git add. Hoping that the bots will be able to apply it.
Also fixed a few PEP errors.

[Konstantin Tokarev]

IMO, this patch is hard to review because it's not clear where files are moved verbatimly and where they are modified. If there are any changes in moved files, it would be better to split patch in two parts: move stuff verbatimly, and then apply modifications.

(There are review systems which are able to track file renames and visualize modifications appropriately; unfortunately our tooling is inferior)

[Drew DeVault]

If you re-create the patch with `git format-patch -M<n>%` you can crank up <n> to adjust the rename detection threshold. It defaults to 50%.

[Michael Catanzaro]

Well a git-formatted patch probably won't work, because it's going to be applied with SVN, not git.

Anyway: thanks a bunch for working on this, Mike!

I agree with Konstantin that splitting out the renames into a separate bug is a good idea.

The style checker can be ignored in the rename bug, but in the bug for the code changes it will need to be placated.

You can click on the red EWS bubbles to see why the build is failing on Apple's ports. Looks like:

make: *** No rule to make target `JavaScriptCore/Scripts/builtins/__init__.py', needed by `JSCBuiltins.h'.  Stop.
make: *** Waiting for unfinished jobs....

[Drew DeVault]

A git formatted patch will just make it easier to review, that's all :)

[Konstantin Tokarev]

git formatted patch should work well with commit queue as well

[Mike Gorse]

Created attachment 350121 [details]
Updated patch.

[EWS Watchlist]

Attachment 350121 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_frontend_dispatcher_header.py:49:  [CppFrontendDispatcherHeaderGenerator.domains_to_generate] Instance of 'CppFrontendDispatcherHeaderGenerator' has no 'events_for_domain' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_objc_backend_dispatcher_header.py:50:  [ObjCBackendDispatcherHeaderGenerator.domains_to_generate] Instance of 'ObjCBackendDispatcherHeaderGenerator' has no 'should_generate_commands_for_domain' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_objc_frontend_dispatcher_implementation.py:48:  [ObjCFrontendDispatcherImplementationGenerator.domains_to_generate] Instance of 'ObjCFrontendDispatcherImplementationGenerator' has no 'should_generate_events_for_domain' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_objc_protocol_type_conversions_header.py:54:  [ObjCProtocolTypeConversionsHeaderGenerator.domains_to_generate] Instance of 'ObjCProtocolTypeConversionsHeaderGenerator' has no 'should_generate_types_for_domain' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_backend_dispatcher_header.py:49:  [CppBackendDispatcherHeaderGenerator.domains_to_generate] Instance of 'CppBackendDispatcherHeaderGenerator' has no 'commands_for_domain' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:347:  [CppProtocolTypesHeaderGenerator._generate_forward_declarations_for_binding_traits] Instance of 'CppProtocolTypesHeaderGenerator' has no 'type_needs_shape_assertions' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_combined_header.py:161:  [BuiltinsCombinedHeaderGenerator.generate_section_for_global_private_code_name_macro] Instance of 'BuiltinsCombinedHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
Total errors found: 7 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Konstantin Tokarev]

Comment on attachment 350121 [details]
Updated patch.

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

> Source/JavaScriptCore/Scripts/cssmin.py:51
> +        sys.stdout = io.TextIOWrapper(sys.stdout.buffer, encoding='UTF-8')

This is duplication of code above

> Source/JavaScriptCore/Scripts/generate-js-builtins.py:50
> +def doopen(file, mode):

do_open?

[Michael Catanzaro]

Comment on attachment 350121 [details]
Updated patch.

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

> Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:32
> +from .models import PrimitiveType, ObjectType, ArrayType, EnumType, AliasedType, Frameworks

Mac bots say:

ValueError: Attempted relative import in non-package

It's probably easier to test locally with python2 to make sure it still works there, rather than relying on the Mac bots to do it.

[Mike Gorse]

Created attachment 350303 [details]
Updated patch.

It looks like the codegen directory is flattened on MacOS/IOS but not on gtk, so imports need to work in both cases.

[EWS Watchlist]

Attachment 350303 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_frontend_dispatcher_header.py:55:  [CppFrontendDispatcherHeaderGenerator.domains_to_generate] Instance of 'CppFrontendDispatcherHeaderGenerator' has no 'events_for_domain' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_objc_backend_dispatcher_header.py:57:  [ObjCBackendDispatcherHeaderGenerator.domains_to_generate] Instance of 'ObjCBackendDispatcherHeaderGenerator' has no 'should_generate_commands_for_domain' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_objc_frontend_dispatcher_implementation.py:54:  [ObjCFrontendDispatcherImplementationGenerator.domains_to_generate] Instance of 'ObjCFrontendDispatcherImplementationGenerator' has no 'should_generate_events_for_domain' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_objc_protocol_type_conversions_header.py:60:  [ObjCProtocolTypeConversionsHeaderGenerator.domains_to_generate] Instance of 'ObjCProtocolTypeConversionsHeaderGenerator' has no 'should_generate_types_for_domain' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_backend_dispatcher_header.py:55:  [CppBackendDispatcherHeaderGenerator.domains_to_generate] Instance of 'CppBackendDispatcherHeaderGenerator' has no 'commands_for_domain' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:353:  [CppProtocolTypesHeaderGenerator._generate_forward_declarations_for_binding_traits] Instance of 'CppProtocolTypesHeaderGenerator' has no 'type_needs_shape_assertions' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_combined_header.py:161:  [BuiltinsCombinedHeaderGenerator.generate_section_for_global_private_code_name_macro] Instance of 'BuiltinsCombinedHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
Total errors found: 7 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Comment on attachment 350303 [details]
Updated patch.

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

OK, thanks!

The try/except imports are a bit yucky, but I guess that's what we're stuck with if we have XCode doing something different than CMake.

> Source/JavaScriptCore/Scripts/wkbuiltins/builtins_model.py:159
> +    def __lt__(self, other):
> +        return self.function_name < other.function_name

Why is it needed?

> Source/JavaScriptCore/Scripts/wkbuiltins/builtins_model.py:309
> +        return list(map(BuiltinFunction.fromString, functionStrings))

You've done this in a couple other places. Why do you need to wrap the map in a list?

[Mike Gorse]

(In reply to Michael Catanzaro from comment #28)
> Comment on attachment 350303 [details]
> Updated patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350303&action=review
> 
> OK, thanks!
> 
> The try/except imports are a bit yucky, but I guess that's what we're stuck
> with if we have XCode doing something different than CMake.
> 
> > Source/JavaScriptCore/Scripts/wkbuiltins/builtins_model.py:159
> > +    def __lt__(self, other):
> > +        return self.function_name < other.function_name
> 
> Why is it needed?

I added that in order to get sort to work on python 3. http://python3porting.com/problems.html#comparisons

> > Source/JavaScriptCore/Scripts/wkbuiltins/builtins_model.py:309
> > +        return list(map(BuiltinFunction.fromString, functionStrings))
> 
> You've done this in a couple other places. Why do you need to wrap the map
> in a list?

Map returns an iterator in python 3, not a list as in python 2. Those lines were changed automatically when I ran 2to3, though, so it's possible that the wrapping isn't needed in every place where I have it.

[WebKit Commit Bot]

Comment on attachment 350303 [details]
Updated patch.

Clearing flags on attachment: 350303

Committed r236321: <https://trac.webkit.org/changeset/236321>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44682001>

[Truitt Savell]

Looks like https://trac.webkit.org/changeset/236321/webkit

has caused a builtins-generators failure: https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK1%20%28Tests%29/builds/12934

Log:
https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK1%20%28Tests%29/builds/12934/steps/builtins-generator-tests/logs/stdio

[Konstantin Tokarev]

The only one test failure is caused by changed order of lines where order is not significant

[Truitt Savell]

Rebaselined tests using run-builtins-generator-tests --reset-results

Committed: https://trac.webkit.org/changeset/236351/webkit

[Tomas Popela]

It still fails to compile without python2 installed because of:

    find_package(PythonInterp 2.7.0 REQUIRED)

in Source/cmake/WebKitCommon.cmake

Mike, can you create a patch for that?

[Tomas Popela]

I also got this error:

Traceback (most recent call last):
  File "/builddir/build/BUILD/webkitgtk-2.22.2/Source/JavaScriptCore/Scripts/generateYarrUnicodePropertyTables.py", line 953, in <module>
    propertyDataHFile.write(header)
TypeError: a bytes-like object is required, not 'str'

[Mike Gorse]

(In reply to Tomas Popela from comment #37)
> I also got this error:
> 
> Traceback (most recent call last):
>   File
> "/builddir/build/BUILD/webkitgtk-2.22.2/Source/JavaScriptCore/Scripts/
> generateYarrUnicodePropertyTables.py", line 953, in <module>
>     propertyDataHFile.write(header)
> TypeError: a bytes-like object is required, not 'str'

I wonder if you have the changes to that file in your build, since it has been moved upstream, along with hasher.py, so you would have needed to explicitly tell patch where to find it. Specifically, the header should now be opened with "w", rather than "wb".
If you do, in fact, have that change, then I'm puzzled as to why python would be wanting a bytes object.

[Mike Gorse]

Created attachment 350816 [details]
Patch for cmake to use its python module

Tomas, could you try this patch, for cmake? I see that PythonInterp is deprecated in favor of Python/Python2/Python3.
Also, is there a minimum version of cmake that we should support?

[Michael Catanzaro]

We declare the minimum at the top of the toplevel CMakeLists.txt. Currently it's at 3.3. I think we can bump it as high as CMake 3.5 (the version in Ubuntu 16.04) as I assume that's old enough for other ports to depend on.

[Michael Catanzaro]

Best create a new bug for the new patch, and be sure to set the r? and cq? flags.

[Mike Gorse]

I just checked cmake 3.10.2, and it doesn't have those modules, so I suspect that my last patch would bump the requirement to 3.12.0.
Might be a good thing to do in the long term, but I guess we don't want to commit it right now.

[Tomas Popela]

(In reply to Mike Gorse from comment #39)
> Created attachment 350816 [details]
> Patch for cmake to use its python module
> 
> Tomas, could you try this patch, for cmake? I see that PythonInterp is
> deprecated in favor of Python/Python2/Python3.
> Also, is there a minimum version of cmake that we should support?

As you already said later, I do have the cmake 3.11 here and the build fails because of the missing FindPython.cmake module. Requiring 3.12 is a NO-GO. We have to find a solution for the cmake < 3.3 (or 3.5 if we raise the requirement).

[Mike Gorse]

I've filed bug 190075 for the cmake issue.
It seems like we can just add
set(Python_ADDITIONAL_VERSIONS 3)
before the find_package line.

[Mike Gorse]

I've noticed that, if LANG and LC_TYPE are unset, then the build still fails with python 3. A few more changes are needed. Filed bug 190258.

[Alicia Boya García]

(In reply to Mike Gorse from comment #45)
> I've noticed that, if LANG and LC_TYPE are unset, then the build still fails
> with python 3. A few more changes are needed. Filed bug 190258.

Python uses these flags to determine the file system encoding. In its absence, it assumes ASCII (which is never what you want). Therefore it's important that bots or any software running Python set it appropriately.

[Alicia Boya García]

Also, even when you have WebKit in a full ASCII path, the default text file encoding is derived from these environment variables too; so it's very easy that a script fails while reading any file containing non ASCII characters e.g. contributors.json or the Changelog unless it does so in binary mode or with a explicit encoding (e.g. `file = io.open("path", "r", encoding="UTF-8")`).

http://python-notes.curiousefficiency.org/en/latest/python3/text_file_processing.html