Bug 156674 - Build tools should work when the /usr/bin/python is python3
Summary: Build tools should work when the /usr/bin/python is python3
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: All Linux
: P2 Minor
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 189552
Blocks: 184986
  Show dependency treegraph
 
Reported: 2016-04-17 06:08 PDT by Drew DeVault
Modified: 2018-10-07 05:38 PDT (History)
14 users (show)

See Also:


Attachments
First pass (95.12 KB, patch)
2018-03-06 14:38 PST, Mike Gorse
no flags Details | Formatted Diff | Diff
Revised patch. (182.64 KB, patch)
2018-08-06 11:54 PDT, Mike Gorse
no flags Details | Formatted Diff | Diff
Updated patch. (191.56 KB, patch)
2018-08-23 14:01 PDT, Mike Gorse
no flags Details | Formatted Diff | Diff
Patch (279.04 KB, patch)
2018-08-27 09:51 PDT, Mike Gorse
mcatanzaro: review-
mcatanzaro: commit-queue-
Details | Formatted Diff | Diff
Updated patch. (96.04 KB, patch)
2018-09-19 08:56 PDT, Mike Gorse
mcatanzaro: review-
mcatanzaro: commit-queue-
Details | Formatted Diff | Diff
Updated patch. (101.26 KB, patch)
2018-09-20 19:17 PDT, Mike Gorse
no flags Details | Formatted Diff | Diff
Patch for cmake to use its python module (25.15 KB, patch)
2018-09-25 17:17 PDT, Mike Gorse
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Drew DeVault 2016-04-17 06:08:53 PDT
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.
Comment 1 Alexey Proskuryakov 2016-04-18 20:56:40 PDT
FWIW, there is no python2 on OS X.
Comment 2 Mike Gorse 2018-03-06 14:38:30 PST
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.
Comment 3 Michael Catanzaro 2018-03-06 16:44:57 PST
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!
Comment 4 Michael Catanzaro 2018-03-06 16:48:03 PST
(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.
Comment 5 Build Bot 2018-03-06 16:49:50 PST
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.
Comment 6 Michael Catanzaro 2018-03-06 17:23:07 PST
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
Comment 7 Mike Gorse 2018-03-06 17:45:47 PST
(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.
Comment 8 Michael Catanzaro 2018-03-08 09:06:06 PST
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.
Comment 9 Mike Gorse 2018-08-06 11:54:19 PDT
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.
Comment 10 Michael Catanzaro 2018-08-06 17:09:19 PDT
Please also use Tools/Scripts/prepare-ChangeLog -b 156674 to generate appropriate ChangeLog entries
Comment 11 Michael Catanzaro 2018-08-20 06:47:13 PDT
(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.
Comment 12 Mike Gorse 2018-08-23 14:01:01 PDT
Created attachment 347950 [details]
Updated patch.
Comment 13 Michael Catanzaro 2018-08-23 15:25:46 PDT
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'
Comment 14 Mike Gorse 2018-08-27 09:51:12 PDT
Created attachment 348154 [details]
Patch
Comment 15 Build Bot 2018-08-27 09:53:36 PDT
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.
Comment 16 Mike Gorse 2018-08-27 09:54:52 PDT
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.
Comment 17 Konstantin Tokarev 2018-08-27 09:57:49 PDT
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)
Comment 18 Drew DeVault 2018-08-27 10:03:26 PDT
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%.
Comment 19 Michael Catanzaro 2018-08-27 13:34:19 PDT
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....
Comment 20 Drew DeVault 2018-08-27 13:37:20 PDT
A git formatted patch will just make it easier to review, that's all :)
Comment 21 Konstantin Tokarev 2018-08-28 04:54:35 PDT
git formatted patch should work well with commit queue as well
Comment 22 Mike Gorse 2018-09-19 08:56:30 PDT
Created attachment 350121 [details]
Updated patch.
Comment 23 Build Bot 2018-09-19 08:59:46 PDT
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.
Comment 24 Konstantin Tokarev 2018-09-19 09:10:45 PDT
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?
Comment 25 Michael Catanzaro 2018-09-19 11:23:00 PDT
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.
Comment 26 Mike Gorse 2018-09-20 19:17:31 PDT
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.
Comment 27 Build Bot 2018-09-20 19:22:17 PDT
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.
Comment 28 Michael Catanzaro 2018-09-20 20:39:46 PDT
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?
Comment 29 Mike Gorse 2018-09-21 08:39:38 PDT
(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.
Comment 30 WebKit Commit Bot 2018-09-21 09:20:14 PDT
Comment on attachment 350303 [details]
Updated patch.

Clearing flags on attachment: 350303

Committed r236321: <https://trac.webkit.org/changeset/236321>
Comment 31 WebKit Commit Bot 2018-09-21 09:20:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 32 Radar WebKit Bug Importer 2018-09-21 09:21:24 PDT
<rdar://problem/44682001>
Comment 34 Konstantin Tokarev 2018-09-21 11:10:42 PDT
The only one test failure is caused by changed order of lines where order is not significant
Comment 35 Truitt Savell 2018-09-21 13:06:34 PDT
Rebaselined tests using run-builtins-generator-tests --reset-results

Committed: https://trac.webkit.org/changeset/236351/webkit
Comment 36 Tomas Popela 2018-09-24 03:19:46 PDT
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?
Comment 37 Tomas Popela 2018-09-24 05:29:14 PDT
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'
Comment 38 Mike Gorse 2018-09-24 07:37:39 PDT
(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.
Comment 39 Mike Gorse 2018-09-25 17:17:14 PDT
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?
Comment 40 Michael Catanzaro 2018-09-25 17:22:16 PDT
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.
Comment 41 Michael Catanzaro 2018-09-25 17:23:15 PDT
Best create a new bug for the new patch, and be sure to set the r? and cq? flags.
Comment 42 Mike Gorse 2018-09-25 17:32:35 PDT
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.
Comment 43 Tomas Popela 2018-09-26 06:59:29 PDT
(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).
Comment 44 Mike Gorse 2018-09-28 08:46:13 PDT
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.
Comment 45 Mike Gorse 2018-10-03 12:53:57 PDT
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.
Comment 46 Alicia Boya García 2018-10-07 04:11:36 PDT
(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.
Comment 47 Alicia Boya García 2018-10-07 05:38:44 PDT
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