GNOME Bugzilla – Bug 777724
[RFC] bootstrap system
Last modified: 2017-09-23 04:13:55 UTC
This idea is from Jasper and I resurrected it from an old branch of his [1]. He was going to use it to rewrite the importer code but I'd like to consider this idea separately from the imports. It could be used to define the Promise object more easily. [1] https://git.gnome.org/browse/gjs/log/?h=wip/imports-rewrite
Created attachment 344191 [details] [review] Add a bootstrap system, allowing us to write JS code at early boot This will be used to write a CommonJS style imports system for JS, but it could also be used for other things, like rewriting window.log, window.logError, etc. in JS.
Created attachment 344192 [details] [review] bootstrap: Define Promise in bootstrap This takes advantage of the new bootstrap stage to import the promises module and define the Promise global object.
Comments welcome! There are a few problems I've found with this implementation: - Errors in the bootstrap code are swallowed. (This shouldn't matter to end users, but it makes GJS development difficult.) - It might be good to make this per-compartment so we can use it for bootstrapping the test coverage stuff as well.
Review of attachment 344192 [details] [review]: We have native promises instead of a promises module now, so this is not needed.
Created attachment 359452 [details] [review] global: Add a bootstrap system Based on a patch by Jasper St. Pierre. This allows executing a JS script (which must be a resource in the directory resource:///org/gnome/gtk/modules/_bootstrap) in order to define extra properties on a global object when the global object is created. This is currently not used for anything, but will be used for the code coverage compartment, and likely for a debugger as well. It can also be used to implement things in JS that are currently implemented in C++, like the log() and logError() functions.
Created attachment 359453 [details] [review] coverage: Use global bootstrap to add JS coverage code This takes advantage of the new bootstrap stage of gjs_define_global_properties() to run the JS coverage code in the coverage compartment. This moves imports.coverage to imports._bootstrap.coverage, which makes sense because the code in imports.coverage was not useful for GJS scripts.
I'm expecting to have to use this for the debugger, so I fixed this up. With the new global.cpp I did end up rewriting it completely. One thing that Jasper's patch did that this one doesn't do, is allow an "environment object" with native properties to be passed in, and have the bootstrap code evaluated in its scope. That's currently not possible with JS::CloneAndExecuteScript() which we use to make sure the file is evaluated in the correct compartment. But, then again, it's also not currently needed. This is also an API break in that imports.coverage is now imports._bootstrap.coverage. The coverage module should really have been more clearly marked as internal-only (which I hope the underscore in _bootstrap now indicates!) I could set up a dummy imports.coverage module that just imported the real one, but I doubt any scripts out there actually use the coverage module, since it's not really useful for client code.
Review of attachment 359452 [details] [review]: LGTM ::: gjs/global.cpp @@ +48,3 @@ + gjs_throw_g_error(cx, error); + return false; + Optional: use g_resources_lookup_data() and a GBytes to make this code a bit cleaner.
Review of attachment 359453 [details] [review]: Nice cleanup! ::: modules/coverage.js @@ +975,3 @@ } + +printerr('executed the thing'); No newline at end of file Hmm...
Comment on attachment 359452 [details] [review] global: Add a bootstrap system Attachment 359452 [details] pushed as 881e6b2 - global: Add a bootstrap system
Review of attachment 359453 [details] [review]: Actually I found out that this makes (unrelated!) tests fail. I originally didn't notice because 'make check-code-coverage' doesn't fail if tests fail. But regular 'make check' also fails when code coverage is enabled.
After digging some more, I found that these exceptions during code coverage collection were always there, just that they were swallowed before! So I'm going to push this anyway (without the debug print of course) and fix the code coverage separately. Exceptions thrown for three reasons: - Coverage code tries to load "files" for scripts fed programmatically to the JS engine and fails (was harmless, only it's now logged) - Coverage code tries to parse files that start with #! and fails (happened before, and caused files starting with #! to be ignored in coverage reports, only it's now logged) - Function finder doesn't recurse into ES6 class bodies (happened before, so coverage reports have been inaccurate since adding ES6 classes to the code, only the exception is now thrown and fails the tests)
Attachment 359453 [details] pushed as 9b0beff - coverage: Use global bootstrap to add JS coverage code