For each new JS builtin file, there are a few edits that are needed in the build system, WebCoreJSClientData.h and WebCoreJSBuiltins.cpp. It might be good if only editing the build system would be needed. That would mean auto generating WebCoreJSBuiltins.cpp and the part of WebCoreJSClientData.h related to JS builtins.
Created attachment 262436 [details] Patch
Attachment 262436 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/JSDOMWindowBase.cpp:89: Missing space after , [whitespace/comma] [3] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 262436 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262436&action=review > Source/WebCore/bindings/js/WebCoreJSClientData.h:33 > class WebCoreJSClientData : public JSC::VM::ClientData { Since this is in the WebCore namespace, it seems a little excessive to include the words “WebCore” in the name of the class. > Source/WebCore/generate-js-builtins-allinone:134 > +class WebCoreJSBuiltinFunctions { Since this is in the WebCore namespace, it seems a little excessive to include the words “WebCore” in the name of the class.
Created attachment 262526 [details] Patch After talking to Youenn we thought it was better to deambiguate the class names from the JSC ones and have the WebCore prefix in them. Anyway, we renamed them from WebCoreJS to WebCore.
It seems I misunderstood Youenn, maybe he can clarify how he wants the names and Darin can bless it so that we can land it :)
(In reply to comment #5) > It seems I misunderstood Youenn, maybe he can clarify how he wants the names > and Darin can bless it so that we can land it :) Meaning the reverse, disambiguate the filenames, not the class names. I will update the patch accordingly today or tomorrow if there is no objection.
Created attachment 262593 [details] Patch Changed the class names, according to the conversation with Youenn.
Comment on attachment 262593 [details] Patch Rejecting attachment 262593 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 262593, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/253723
Created attachment 262594 [details] Patch for landing
(In reply to comment #9) > Created attachment 262594 [details] > Patch for landing Name changes - PrivateWebCoreJSBuiltinFunctions -> JSBuiltinInternalFunctions - WebCoreJSBuiltinFunctions -> JSBuiltinFunctions - WebCoreJSClientData -> JSClientData For the latter, JSVMClientData might have been somehow better.
Comment on attachment 262594 [details] Patch for landing Clearing flags on attachment: 262594 Committed r190664: <http://trac.webkit.org/changeset/190664>
All reviewed patches have been landed. Closing bug.
My build is broken after this: CpHeader PrivateWebCoreJSBuiltins error: /Volumes/DataSSD/Development/apple/webkit/OpenSource/WebKitBuild/Debug/DerivedSources/WebCore/PrivateWebCoreJSBuiltins.h: No such file or directory
(In reply to comment #13) > My build is broken after this: > CpHeader PrivateWebCoreJSBuiltins > error: > /Volumes/DataSSD/Development/apple/webkit/OpenSource/WebKitBuild/Debug/ > DerivedSources/WebCore/PrivateWebCoreJSBuiltins.h: No such file or directory Probably related to DerivedSources.make. Can you try a clean build?
Re-opened since this is blocked by bug 149877
I don’t think it’s reasonable to ask everyone to do a clean build. Perhaps this would be fixed by touching the generate-js-builtins-allinone source file one more time? We should look at his build log to see if generate-js-builtins-allinone failed?
(In reply to comment #16) > I don’t think it’s reasonable to ask everyone to do a clean build. Perhaps > this would be fixed by touching the generate-js-builtins-allinone source > file one more time? We should look at his build log to see if > generate-js-builtins-allinone failed? Actually it is not reasonable at all as you'd need a half built WebKit to build from scratch. The problem happens at: CpHeader /Users/calvaris/WebKit/WebKitBuild/Release/DerivedSources/WebCore/PrivateWebCoreJSBuiltins.h /Users/calvaris/WebKit/WebKitBuild/Release/WebCore.framework/Versions/A/PrivateHeaders/PrivateWebCoreJSBuilt\ ins.h cd /Users/calvaris/WebKit/Source/WebCore builtin-copy -exclude .DS_Store -exclude CVS -exclude .svn -exclude .git -exclude .hg -strip-debug-symbols -strip-tool /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/\ strip -resolve-src-symlinks /Users/calvaris/WebKit/WebKitBuild/Release/DerivedSources/WebCore/PrivateWebCoreJSBuiltins.h /Users/calvaris/WebKit/WebKitBuild/Release/WebCore.framework/Versions/A/PrivateHeaders error: /Users/calvaris/WebKit/WebKitBuild/Release/DerivedSources/WebCore/PrivateWebCoreJSBuiltins.h: No such file or directory I tried to add: diff --git a/Source/WebCore/DerivedSources.make b/Source/WebCore/DerivedSources.make index a5967e7..8d12cc6 100644 --- a/Source/WebCore/DerivedSources.make +++ b/Source/WebCore/DerivedSources.make @@ -1260,7 +1260,9 @@ WEBCORE_JS_BUILTINS = \ $(WebCore)/Modules/streams/ReadableStreamReader.js \ # -all : WebCoreJSBuiltins.cpp $(WEBCORE_JS_BUILTINS:%.js=%Builtins.cpp) +all : PrivateWebCoreJSBuiltins.h WebCoreJSBuiltins.cpp $(WEBCORE_JS_BUILTINS:%.js=%Builtins.cpp) + +PrivateWebCoreJSBuiltins.h: WebCoreJSBuiltins.cpp WebCoreJSBuiltins.cpp: $(WEBCORE_JS_BUILTINS) $(WebCore)/generate-js-builtins-allinone $(PYTHON) $(WebCore)/generate-js-builtins-allinone $(WEBCORE_JS_BUILTINS) --output_dir . but it didn't work either
Created attachment 262639 [details] Updating DerivedSources.make rules
Created attachment 262677 [details] Generating one file at a time in generate-jsbuiltins-allinone
The updated DerivedSources.make rules should hopefully cover the issue encountered by smfr. I played with the generated Builtins.h/.cpp files and they seem to be correctly regenerated when needed. That said, last patch failed on mac due to a different issue from the one reported by smfr: --------------------- Traceback (most recent call last): File "WebCore/generate-js-builtins-allinone", line 232, in <module> copytempfile(output_base + ".h") File "WebCore/generate-js-builtins-allinone", line 225, in copytempfile if (not os.path.exists(output)) or (not filecmp.cmp(output + ".tmp", output, shallow=False)): File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/filecmp.py", line 42, in cmp rm -f CommandLineAPIModuleSource.min.js s1 = _sig(os.stat(f1)) OSError: [Errno 2] No such file or directory: './WebCoreJSBuiltins.h.tmp' --------------------- This is somehow strange since the script should have generated that .h.tmp file a few lines before. I added file flushing in the script just in case.
Comment on attachment 262677 [details] Generating one file at a time in generate-jsbuiltins-allinone Clearing flags
Not sure why it was set as RESOLVED. Reopening it manually.
(In reply to comment #16) > I don’t think it’s reasonable to ask everyone to do a clean build. Perhaps > this would be fixed by touching the generate-js-builtins-allinone source > file one more time? We should look at his build log to see if > generate-js-builtins-allinone failed? From IRC discussions, a header file was not generated, meaning that generate-js-builtins-allinone was probably not kicking in. I am not exactly sure why since mac bots were fine. I am now relanding an updated patch: - Some changes in DerivedSources.make rules that hopefully will fix smfr issue. - Renaming WebCoreJSClientData as JSVMClientData. - Generating one file at a time in generate-js-builtins-allinone
Comment on attachment 262677 [details] Generating one file at a time in generate-jsbuiltins-allinone Clearing flags on attachment: 262677 Committed r190716: <http://trac.webkit.org/changeset/190716>
Re-opened since this is blocked by bug 149924
(In reply to comment #26) > Re-opened since this is blocked by bug 149924 The following issue is now happening from time to time on Mac bots. --------------------- Generating WebCore JS builtins wrapper files. Traceback (most recent call last): File "WebCore/generate-js-builtins-allinone", line 127, in <module> copytempfile(output_base + ".cpp") File "WebCore/generate-js-builtins-allinone", line 74, in copytempfile os.remove(output + ".tmp") OSError: [Errno 2] No such file or directory: './WebCoreJSBuiltins.cpp.tmp' make: *** [WebCoreJSBuiltins.h] Error 1 --------------------- generate-js-builtins-allinone should be generating WebCoreJSBuiltins.cpp.tmp. Then it should try to compare it with WebCoreJSBuiltins.cpp If they are the same, then WebCoreJSBuiltins.cpp.tmp is deleted. This step seemed to fail.
Another trace found in an EWS mac bot: Traceback (most recent call last): File "WebCore/generate-js-builtins-allinone", line 127, in <module> copytempfile(output_base + ".cpp") File "WebCore/generate-js-builtins-allinone", line 69, in copytempfile if (not os.path.exists(output)) or (not filecmp.cmp(output + ".tmp", output, shallow=False)): File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/filecmp.py", line 42, in cmp s1 = _sig(os.stat(f1)) OSError: [Errno 2] No such file or directory: './WebCoreJSBuiltins.cpp.tmp'
Some build log links: https://webkit-queues.webkit.org/results/257622 https://build.webkit.org/builders/Apple%20Mavericks%20Release%20%28Build%29/builds/19177/steps/compile-webkit/logs/stdio
A comment about the WebCoreJSBuiltins.cpp.tmp strategy (not an explanation of the build failures, but a separate issue): This technique of only touching a file if there is a change doesn’t work well with makefiles. If a build rule doesn’t regenerate the file then it will keep getting triggered over and over again each time you make, so it’s typically not practical to optimize the case where there is no change to have it not touch the output file. The desire to optimize in this way comes up over and over again, so we’ve run into that problem over and over again. Maybe there’s some trick here used to avoid that problem?
(In reply to comment #29) > Some build log links: > https://webkit-queues.webkit.org/results/257622 > https://build.webkit.org/builders/Apple%20Mavericks%20Release%20%28Build%29/ > builds/19177/steps/compile-webkit/logs/stdio Problem is probably again in DerivedSources.make It seems generate-js-builtins-allinone is called twice, explaining the issue.
(In reply to comment #30) > A comment about the WebCoreJSBuiltins.cpp.tmp strategy (not an explanation > of the build failures, but a separate issue): > > This technique of only touching a file if there is a change doesn’t work > well with makefiles. If a build rule doesn’t regenerate the file then it > will keep getting triggered over and over again each time you make, so it’s > typically not practical to optimize the case where there is no change to > have it not touch the output file. > > The desire to optimize in this way comes up over and over again, so we’ve > run into that problem over and over again. Maybe there’s some trick here > used to avoid that problem? We do something similar in the Inspector protocol bindings generator, and it has worked pretty well once the bugs were flushed out. I'll comment specifically on the patch in a minute.
I may retry landing it tomorrow, but only the part that does not changing DerivedSources.make. Basically that means upstreaming the refactoring, the files generated by generate-js-builtins-allinone but not the generate-js-builtins-allinone script. This will allow starting working on WritableStream as well as reducing the patch that would upstream generate-js-builtins-allinone. Please let me know whether the reduced patch would need a new review cycle. Otherwise, I will land it directly tomorrow.
(In reply to comment #32) > (In reply to comment #30) > > A comment about the WebCoreJSBuiltins.cpp.tmp strategy (not an explanation > > of the build failures, but a separate issue): > > > > This technique of only touching a file if there is a change doesn’t work > > well with makefiles. If a build rule doesn’t regenerate the file then it > > will keep getting triggered over and over again each time you make, so it’s > > typically not practical to optimize the case where there is no change to > > have it not touch the output file. > > > > The desire to optimize in this way comes up over and over again, so we’ve > > run into that problem over and over again. Maybe there’s some trick here > > used to avoid that problem? > > We do something similar in the Inspector protocol bindings generator, and it > has worked pretty well once the bugs were flushed out. I'll comment > specifically on the patch in a minute. That sounds interesting. Using CMake (with ccache bonus) would also help reducing this issue and maybe remove the need for this particular optimization.
(In reply to comment #33) > I may retry landing it tomorrow, but only the part that does not changing > DerivedSources.make. > Basically that means upstreaming the refactoring, the files generated by > generate-js-builtins-allinone but not the generate-js-builtins-allinone > script. > > This will allow starting working on WritableStream as well as reducing the > patch that would upstream generate-js-builtins-allinone. > > Please let me know whether the reduced patch would need a new review cycle. > Otherwise, I will land it directly tomorrow. I would recommend proceeding with the "manual" approach for the time being. I'm looking into making generate-js-builtins look more like the inspector generator (i.e., has regression tests, doesn't use .tmp pattern), but this will be a bit of work.
Reopening it manually...
Created attachment 262762 [details] Removing automatic generation and adding previously generated files in trunk
Attachment 262762 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/WebCoreJSBuiltinInternals.h:30: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/WebCore/bindings/js/WebCoreJSBuiltinInternals.h:39: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebCore/bindings/js/WebCoreJSBuiltinInternals.h:44: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebCore/bindings/js/WebCoreJSBuiltinInternals.h:53: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 4 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 262762 [details] Removing automatic generation and adding previously generated files in trunk Rejecting attachment 262762 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: /usr/bin/strip -resolve-src-symlinks /Volumes/Data/EWS/WebKit/Source/WebCore/Modules/indexeddb/IDBDatabaseMetadata.h /Volumes/Data/EWS/WebKit/WebKitBuild/Release/WebCore.framework/Versions/A/PrivateHeaders ** BUILD FAILED ** The following build commands failed: CpHeader /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/WebCoreJSBuiltinInternals.h /Volumes/Data/EWS/WebKit/WebKitBuild/Release/WebCore.framework/Versions/A/PrivateHeaders/WebCoreJSBuiltinInternals.h (1 failure) Full output: http://webkit-queues.webkit.org/results/260970
Created attachment 262766 [details] Fixing mac build
Attachment 262766 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/js/WebCoreJSBuiltinInternals.h:30: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/WebCore/bindings/js/WebCoreJSBuiltinInternals.h:39: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebCore/bindings/js/WebCoreJSBuiltinInternals.h:44: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebCore/bindings/js/WebCoreJSBuiltinInternals.h:53: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 4 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 262766 [details] Fixing mac build Clearing flags on attachment: 262766 Committed r190794: <http://trac.webkit.org/changeset/190794>