Summary: | REGRESSION(r238016)[GTK][TestWebKitAPI][Ninja] TestJSC can't compile "Bytecodes.h: No such file or directory" | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||
Component: | JavaScriptCore | Assignee: | Michael Catanzaro <mcatanzaro> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aboya, annulen, bugs-noreply, calvaris, commit-queue, don.olmstead, Hironori.Fujii, jeremyhu, mcatanzaro, pgriffis, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=182757 | ||||||||||
Attachments: |
|
Description
Michael Catanzaro
2018-11-15 08:36:46 PST
Created attachment 354937 [details]
Patch
Dunno about this. It looks like the directory is listed under private include paths for a reason...? Don? Note this error is a flake in the build system. Often after retrying build a couple of times, it works. We'd rather not have to deal with that of course. Possibly related: https://bugs.webkit.org/show_bug.cgi?id=161477 I don't think this is the right fix. WEBKIT_MAKE_FORWARDING_HEADERS(JavaScriptCore TARGET_NAME JavaScriptCorePrivateForwardingHeaders FILES ${JavaScriptCore_PRIVATE_FRAMEWORK_HEADERS} DERIVED_SOURCE_DIRECTORIES ${DERIVED_SOURCES_DIR}/JavaScriptCore ${DERIVED_SOURCES_DIR}/JavaScriptCore/inspector FLATTENED ) Is in CMakeLists.txt and it should be copying the contents of that derived sources directory. TestWebKitAPI_DEPENDENCIES has JavaScriptCorePrivateForwardingHeaders as a dependency though so that should be happening. There might be a bug in what Fujii did with the copying of the dependencies. What happens if you just generate the project and then go into the build directory and do ninja TestJSC? (In reply to Don Olmstead from comment #4) > What happens if you just generate the project and then go into the build > directory and do ninja TestJSC? $ mkdir -p WebKitBuild/Debug $ cd WebKitBuild/Debug $ ../../Tools/jhbuild/jhbuild-wrapper --gtk run cmake -DPORT=GTK -DCMAKE_BUILD_TYPE=Debug -DDEVELOPER_MODE=ON -GNinja ../.. $ ../../Tools/jhbuild/jhbuild-wrapper --gtk run ninja TestJSC Without my patch, it fails as shown in comment #0. $ ../../Tools/jhbuild/jhbuild-wrapper --gtk run ninja JavaScriptCorePrivateForwardingHeaders ninja: no work to do. So it's getting executed properly. My guess is that it's perhaps running before Bytecodes.h is generated? Here's where the header is generated during my build: [1114/1352] cd /home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/Source/JavaScriptCore && /usr/bin/ruby /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/generator/main.rb --bytecodes_h /home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/DerivedSources/JavaScriptCore/Bytecodes.h --init_bytecodes_asm /home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/DerivedSources/JavaScriptCore/InitBytecodes.asm --bytecode_structs_h /home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/DerivedSources/JavaScriptCore/BytecodeStructs.h --bytecode_indices_h /home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/DerivedSources/JavaScriptCore/BytecodeIndices.h /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/bytecode/BytecodeList.rb But all of my JSC forwarding headers were already copied before this point. The last one was copied here: [1096/1352] cd /home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/DerivedSources && /usr/bin/cmake -E copy_if_different /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/Scripts/make-js-file-arrays.py /home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/DerivedSources/ForwardingHeaders/JavaScriptCore/Scripts/make-js-file-arrays.py Note 1096 < 1114 So it seems to be a dependency issue. I have a feeling that it'd be safer if we enumerated the DerivedSources headers that need to be copied so CMake understands all the dependencies. I'll let Fujii chime in there. *** Bug 161477 has been marked as a duplicate of this bug. *** This is the same issue with Bug 182757. This problem happens if a generated header is used as a forwarding header. There are two approach. 1. Explicitly list derived headers to copy (Bug 182757 Comment 5) 2. Introduce intermediate target between TestJSC and JavaScriptCore to ensure JavaScriptCore is finished. (Bug 182757 Comment 8) > +add_custom_target(pre-TestJSC) > +add_dependencies(TestJSC pre-TestJSC) > +add_dependencies(pre-TestJSC JavaScriptCore) I'll take a look more deeply today. I'm testing with Ubuntu 18.04.1 LTS. Here is the command to reproduce. > rm -rf WebKitBuild/Debug && ./Tools/Scripts/build-webkit --gtk --debug --cmakeargs=-DENABLE_BUBBLEWRAP_SANDBOX=OFF --makeargs=Tools/TestWebKitAPI/CMakeFiles/TestJSC.dir/Tests/JavaScriptCore/glib/TestJSC.cpp.o This has happened since trunk@238016 (Bug 191439) which adds '#include "Lexer.h"' in ParseInt.h. Created attachment 355023 [details]
Patch
Comment on attachment 355023 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355023&action=review Wow, great investigation. Thanks Fujii. > Tools/TestWebKitAPI/PlatformGTK.cmake:137 > +# Add an intermediate target between TestJSC and JavaScriptCore to ensure derived headers are copied into the forwarding header directory. > +add_custom_target(pre-TestJSC) > +add_dependencies(TestJSC pre-TestJSC) > +add_dependencies(pre-TestJSC JavaScriptCore) Hmm. At a minimum, it would need to be done for PlatformWPE.cmake as well. I'm not sure why the custom target is required. If the JavaScriptCore target is built, then the pre-TestJSC target is immediately satisfied and then TestJSC is ready to run, right? Would it work if we remove the pre-TestJSC target and just use add_dependencies(TestJSC, JavaScriptCore)? Or is that not enough to ensure that the headers are copied? I fear we're going to have this same problem in other tests, on other ports. Maybe we could instead ensure the target is not linked until after the forwarding headers are copied? (In reply to Michael Catanzaro from comment #13) > I fear we're going to have this same problem in other tests, on other ports. I guess bug #182757 is evidence enough of that. (In reply to Michael Catanzaro from comment #13) > Hmm. At a minimum, it would need to be done for PlatformWPE.cmake as well. I will fix it next Monday. Feel free to take over the patch if someone can't wait. > I'm not sure why the custom target is required. It makes compile time dependency. > If the JavaScriptCore target > is built, then the pre-TestJSC target is immediately satisfied and then > TestJSC is ready to run, right? No. As I mentioned in the ChangeLog entry, it is "link time dependency which means compiling source files of TestJSC starts before the POST_BUILD event of JSC". > Would it work if we remove the pre-TestJSC > target and just use add_dependencies(TestJSC, JavaScriptCore)? I don't think so because TestJSC already has a dependency to JavaScriptCore. > Or is that > not enough to ensure that the headers are copied? It is possible by taking the approach#1. But, it had a problem for VS generator builds (Bug 182757 Comment 5). The CMake issue has been addressed(Bug 182757 Comment 36). I will try the approach#1. > I fear we're going to have this same problem in other tests, on other ports. Year. The approach#1 is the ideal solution. > Maybe we could instead ensure the target is not linked until after the > forwarding headers are copied? `linked`? This is a compilation error. It is compile time dependency issue. Created attachment 355247 [details]
Patch
(In reply to Michael Catanzaro from comment #14) > (In reply to Michael Catanzaro from comment #13) > > I fear we're going to have this same problem in other tests, on other ports. > > I guess bug #182757 is evidence enough of that. JavaScriptCore is the only target which is using WEBKIT_MAKE_FORWARDING_HEADERS and needs to copy derived headers at the moment. I hope I will fix this issue properly soon. Thanks! Comment on attachment 355247 [details] Patch Clearing flags on attachment: 355247 Committed r238374: <https://trac.webkit.org/changeset/238374> All reviewed patches have been landed. Closing bug. |