Issue 29556820: Working Draft: CMake build for Emscripten
DescriptionWorking Draft: CMake build for Emscripten
This is a working draft of a CMake build to replace the hard-coded `compile`
script currently in use.
Patch Set 1 #Patch Set 2 : #
Total comments: 10
Patch Set 3 : #
MessagesTotal messages: 10
Current maturity: OK as proof of concept, but not ready for production. The bindings file is being generated identically to the one the compile script generates. The main output is not. It does yield some output, but it's not minified and there are likely other differences. There are also a handful of undefined symbols at the end of compilation; I don't know whether that's to be expected or not.
Patch set 2 gives identical results for "bindings.js" and "compiled.js" generated by "python compile --debug". Current version is hard-coded for debug-only. Next version will have configurations supporting debug and trace. Added Cc: Wladimir, since this version actually works. Known deficiencies are listed at the end of the README.
This definitely has its rough edges... https://codereview.adblockplus.org/29556820/diff/29562630/CMakeLists.txt File CMakeLists.txt (right): https://codereview.adblockplus.org/29556820/diff/29562630/CMakeLists.txt#newc... CMakeLists.txt:19: if (${NODE_JS_EXECUTABLE} MATCHES "NOTFOUND") Could it be that Emscripten configures an external Node instance rather than using its own? That would be suboptimal, and at the very least we'll need to mention it as an additional dependency. https://codereview.adblockplus.org/29556820/diff/29562630/CMakeLists.txt#newc... CMakeLists.txt:132: target_link_libraries(compiled PRIVATE "-s SHELL_FILE=\"\\\"${SHELL_FILE}\\\"\"") What about using single quotation marks? SHELL_FILE=\"'${SHELL_FILE}'\"
On 2017/10/04 12:05:51, Wladimir Palant wrote: > This definitely has its rough edges... CMake always seems to induce rough edges. I don't care for its language; it's like getting in a time machine to 30 years ago. Using it once that's done, though, is quite good.
https://codereview.adblockplus.org/29556820/diff/29562630/CMakeLists.txt File CMakeLists.txt (right): https://codereview.adblockplus.org/29556820/diff/29562630/CMakeLists.txt#newc... CMakeLists.txt:19: if (${NODE_JS_EXECUTABLE} MATCHES "NOTFOUND") On 2017/10/04 12:05:50, Wladimir Palant wrote: > Could it be that Emscripten configures an external Node instance rather than > using its own? That would be suboptimal, and at the very least we'll need to > mention it as an additional dependency. If you do it right, then the Emscripten toolchain file will find its own version of node.js. If you do it wrong though, it might not find anything. As you can probably surmise, I did it wrong at first. You have to be inside a shell with the Emscripten environment when you initially configure CMake. In that case the toolchain file will locate whatever version of node.js is in the PATH, and it should be the one from the Emscripten toolchain. The one it finds goes into the CMake cache and stays there until the cache is cleared. On the other hand, if you're not in an Emscripten environment, the toolchain file isn't smart enough to check to see if its being invoked from within its own distribution tree (instead of, say, being copied elsewhere). In that case, assuming no other copy of node.js, it finds nothing. You can't generate bindings, the build will fail, and so the test above is just a sanity test to make it a bit easier to diagnose not following the directions right. There is a note about this in the README, but rereading it, it could probably stand to be expanded and be more prominent. https://codereview.adblockplus.org/29556820/diff/29562630/CMakeLists.txt#newc... CMakeLists.txt:132: target_link_libraries(compiled PRIVATE "-s SHELL_FILE=\"\\\"${SHELL_FILE}\\\"\"") On 2017/10/04 12:05:50, Wladimir Palant wrote: > What about using single quotation marks? SHELL_FILE=\"'${SHELL_FILE}'\" I'll try that. CMake has some capability to do platform-specific calculations, and I've been thinking that it might be use an ordinary-looking expression as the default and special-case Windows path names. From what I can tell, the problem is that Emscripten treats this parameter as simple text, not as a path name, and also passes it on an internal command line to invoke other tools without requoting it. This definitely wreaks havoc with backslashes, but it's probably not the only character that gets maltreated. It seems that this is related to why Emscripten doesn't run right under CMake with cygwin.
https://codereview.adblockplus.org/29556820/diff/29562630/CMakeLists.txt File CMakeLists.txt (right): https://codereview.adblockplus.org/29556820/diff/29562630/CMakeLists.txt#newc... CMakeLists.txt:132: target_link_libraries(compiled PRIVATE "-s SHELL_FILE=\"\\\"${SHELL_FILE}\\\"\"") It seems that you don't have any escaping issues with single quotation marks above, in the EXPORTED_RUNTIME_METHODS parameter.Either way, single quotation marks would be the correct thing to do here anyway. It's not obvious, but this parameter is actually passing Python code to Emscripten. Python's code style rule mandate single quotation marks.
one of the problems I noticed is that the emscripten output files are not ignored by eslint anymore.
In general good so far, though I have not tried it yet. https://codereview.adblockplus.org/29556820/diff/29562630/CMakeLists.txt File CMakeLists.txt (right): https://codereview.adblockplus.org/29556820/diff/29562630/CMakeLists.txt#newc... CMakeLists.txt:19: if (${NODE_JS_EXECUTABLE} MATCHES "NOTFOUND") On 2017/10/05 13:06:13, Eric wrote: > On 2017/10/04 12:05:50, Wladimir Palant wrote: > > Could it be that Emscripten configures an external Node instance rather than > > using its own? That would be suboptimal, and at the very least we'll need to > > mention it as an additional dependency. > > If you do it right, then the Emscripten toolchain file will find its own version > of node.js. If you do it wrong though, it might not find anything. > > As you can probably surmise, I did it wrong at first. You have to be inside a > shell with the Emscripten environment when you initially configure CMake. In > that case the toolchain file will locate whatever version of node.js is in the > PATH, and it should be the one from the Emscripten toolchain. The one it finds > goes into the CMake cache and stays there until the cache is cleared. > > On the other hand, if you're not in an Emscripten environment, the toolchain > file isn't smart enough to check to see if its being invoked from within its own > distribution tree (instead of, say, being copied elsewhere). In that case, > assuming no other copy of node.js, it finds nothing. You can't generate > bindings, the build will fail, and so the test above is just a sanity test to > make it a bit easier to diagnose not following the directions right. > > There is a note about this in the README, but rereading it, it could probably > stand to be expanded and be more prominent. Which node is found actually depends on the order of paths in the PATH variable, however emscripten configuration has already NODE_JS variable, and it's not clear why this variable is not used if the environment is required anyway. In addition, with node.js particularly it would be better to use the exact version either already configured in emscripten or just mentioned in the documentation, I'm pretty sure that we could call a python script reading the value and printing it, though there can be some nuances when it actually happens. https://codereview.adblockplus.org/29556820/diff/29562630/CMakeLists.txt#newc... CMakeLists.txt:91: COMMAND ${NODE_JS_EXECUTABLE} bindings-generator >bindings.js Is there a way to store both an intermediate and the final results in a special build/${configuration} subdirectory and how should it be used from libadblockplus?
Patch set 3 upgrades the status of this change set from "Work in Progress" to "Working Draft". This version has four configurations available. I've got four build trees set up locally, one for each, and some (Windows) scripts to configure and build everything. All outputs match exactly the outputs of the `compile` script. The important deficiencies of the previous version have been fixed. If we decide to go with CMake, I would recommend splitting the present `CMakeLists.txt` file up and separating out the environment setup pieces. This would improve overall legibility, making clear what's relevant to the ABP code and what's relevant to setting up the tools. I have not yet tested this with Eclipse, but I don't anticipate any particular problems with it. I've been using Eclipse/clang/CMake for some personal projects, and it all works pretty well. On 2017/10/10 14:00:59, hub wrote: > one of the problems I noticed is that the emscripten output files are not > ignored by eslint anymore. For an out-of-source build, this is not a problem. For an in-source build, we can add them to the `.eslintignore` file. I've put a note to this effect in the README so we don't forget. https://codereview.adblockplus.org/29556820/diff/29562630/CMakeLists.txt File CMakeLists.txt (right): https://codereview.adblockplus.org/29556820/diff/29562630/CMakeLists.txt#newc... CMakeLists.txt:19: if (${NODE_JS_EXECUTABLE} MATCHES "NOTFOUND") On 2017/10/17 10:58:42, sergei wrote: > Which node is found actually depends on the order of paths in the PATH variable, > however emscripten configuration has already NODE_JS variable, and it's not > clear why this variable is not used if the environment is required anyway. Yes, exactly. What you are observing is what I've been dealing with in detail, which is that the `.cmake` toolchain file that Emscripten provides is, to be kind about it, immature. It's finicky and has to be called just so. I've expanded on this issue in the README. > In > addition, with node.js particularly it would be better to use the exact version > either already configured in emscripten I agree. If we decide to go with CMake, I think it would be advisable to write a wrapper around their toolchain file that did a better job with everything. It should be possible to set everything up correctly and reliably just knowing the root directory of the SDK, but for god-knows-what-reason the Emscripten developers didn't do it that way. https://codereview.adblockplus.org/29556820/diff/29562630/CMakeLists.txt#newc... CMakeLists.txt:91: COMMAND ${NODE_JS_EXECUTABLE} bindings-generator >bindings.js On 2017/10/17 10:58:42, sergei wrote: > Is there a way to store both an intermediate and the final results in a special > build/${configuration} subdirectory and how should it be used from > libadblockplus? CMake has a way of exporting projects and importing them into a different project, along with all the path names. So we don't need to store files in any particular place; CMake handles that. What we will need to do is to call export() in this CMakeLists.txt to generate a `.cmake` file that can be imported by another project. So you still have to know where the export `.cmake` is, but that's all. And it can just go in the build root (project root for in-source build; somewhere else for out-of-source build). See https://cmake.org/Wiki/CMake/Tutorials/Exporting_and_Importing_Targets in the section "Exporting from a Build Tree" https://codereview.adblockplus.org/29556820/diff/29562630/CMakeLists.txt#newc... CMakeLists.txt:132: target_link_libraries(compiled PRIVATE "-s SHELL_FILE=\"\\\"${SHELL_FILE}\\\"\"") On 2017/10/04 12:05:50, Wladimir Palant wrote: > What about using single quotation marks? SHELL_FILE=\"'${SHELL_FILE}'\" Done. It works fine.
We should close this now. I don't have permissions. Eric? 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
