closure-library icon indicating copy to clipboard operation
closure-library copied to clipboard

closurebuilder.py doesn't track requireType calls

Open niloc132 opened this issue 4 years ago • 4 comments

I've been using this script to extract the minimal set of JS files required for closure-compiler to run its tests, but a minimal way to reproduce this is to invoke it on events/events.js and compare output with the various dependencies listed in the events/events.js file:

$ closure/bin/build/closurebuilder.py --root . -i closure/goog/events/events.js 
closure/bin/build/closurebuilder.py: Scanning paths...
closure/bin/build/closurebuilder.py: 1640 sources scanned.
closure/bin/build/closurebuilder.py: Building dependency tree..
closure/goog/base.js
closure/goog/dom/nodetype.js
closure/goog/debug/error.js
closure/goog/asserts/asserts.js
closure/goog/events/eventid.js
closure/goog/events/listenable.js
closure/goog/events/listener.js
closure/goog/object/object.js
closure/goog/array/array.js
closure/goog/events/listenermap.js
closure/goog/string/internal.js
closure/goog/string/typedstring.js
closure/goog/string/const.js
closure/goog/fs/url.js
closure/goog/i18n/bidi.js
closure/goog/fs/blob.js
closure/goog/memoize/memoize.js
closure/goog/html/trustedtypes.js
closure/goog/html/safescript.js
closure/goog/html/trustedresourceurl.js
closure/goog/html/safeurl.js
closure/goog/html/safestyle.js
closure/goog/html/safestylesheet.js
closure/goog/dom/tags.js
closure/goog/dom/htmlelement.js
closure/goog/dom/tagname.js
closure/goog/labs/useragent/util.js
closure/goog/labs/useragent/browser.js
closure/goog/html/safehtml.js
closure/goog/html/uncheckedconversions.js
closure/goog/dom/asserts.js
closure/goog/functions/functions.js
closure/goog/dom/safe.js
closure/goog/string/string.js
closure/goog/labs/useragent/platform.js
closure/goog/reflect/reflect.js
closure/goog/labs/useragent/engine.js
closure/goog/useragent/useragent.js
closure/goog/events/browserfeature.js
closure/goog/debug/entrypointregistry.js
closure/goog/events/eventtype.js
closure/goog/debug/errorcontext.js
closure/goog/debug/debug.js
closure/goog/disposable/idisposable.js
closure/goog/disposable/disposable.js
closure/goog/events/event.js
closure/goog/events/browserevent.js
closure/goog/events/events.js

This is broken as of https://github.com/google/closure-library/commit/803aa52f65e59ee354ad40f89f18b5d847523f73#diff-ae4b8959b41e658aae28c9b7e0152987. It appears that prior to this, there was a goog.forwardDeclare on two dependencies (i.e. "this is optional, may not actually be necessary"), and that was changed to goog.requireType, which will now fail in closure-compiler if the dependency is absent:

minimal-dependencies/closure/goog/events/events.js:56: ERROR - [JSC_MISSING_MODULE_OR_PROVIDE] Required namespace "goog.debug.ErrorHandler" never defined.
goog.requireType('goog.debug.ErrorHandler');
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

minimal-dependencies/closure/goog/events/events.js:57: ERROR - [JSC_MISSING_MODULE_OR_PROVIDE] Required namespace "goog.events.EventWrapper" never defined.
goog.requireType('goog.events.EventWrapper');
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Given that goog.forwardDeclare is deprecated, it probably either makes sense to update closurebuilder.py to include these dependencies, or update closure-compiler to not require these apparently optional dependencies at compile time? I tried briefly to modify source.py's regex to match a requireType call as if it were a require, but this leads to cycles (at least in this case), so obviously isn't suitable.


Workaround: for each missing type, add a -i argument for that file to closurebuilder.py, or revert 803aa52f locally.

niloc132 avatar May 23 '20 23:05 niloc132

As mentioned in the script, Closure Compiler itself is better able to understand dependencies than this script. You should be able to migrate this workflow over to a direct compiler invocation by specifying the entry point. We'd prefer not to continue to support this tool when there is better native compiler support for managing dependencies.

Is there a particular reason you need a minimal set of files for your tests (vs making all files available to load in a test via a simple http server and relying on a deps.js file to figure out which files and order to load in)?

12wrigja avatar May 26 '20 18:05 12wrigja

Skipping the py scripts sounds good (if it doesn't work... maybe it is time to remove it? or at least log very loud deprecation warnings?) - is there a convenient way to ask closure-compiler to dump the list of file names, rather than produce any kind of bundle/build? I know I could write a one-off main() to do it, but going from a one-line sh script to a java project is kind of a jolt.

The purpose here is to emit just a subset of closure-library, just the minimal set of files required for

  • j2cl's runtime (5 files, base, long, their dependencies)
  • testcase+testsuite (not counting the above, 97 files)

These files are merely archived into a zip, repackaged and used from other tooling since the other 1500-odd files are not needed by downstream projects, so there is no point for closure-compiler to parse all of them, since they will never matter. Also, isn't generating deps.js also typically done through these python scripts? We've additionally found BUNDLE at a minimum to be required to load moderately large apps - asking base.js to pull in thousands of files (j2cl emits two .js files per java class) to get the app going has been prohibitively slow. I had previously understood that a deps.js file would be generated from another one of these py scripts (depswriter.py? something like that), which would be deprecated as well, right? I am now seeing that there is a DepGenerator.java in the compiler project, albeit without a main() or any references to it from the other command line tools - what is the preferred way to generate that deps.js file without python?

niloc132 avatar May 26 '20 19:05 niloc132

...and since I'm dumb and didn't say this before, the reason we don't see the logging you linked is that we use the default output_mode: list, since we just want a list of files to pack into a zip.

niloc132 avatar May 26 '20 19:05 niloc132

@tjgq maybe you have some insight into this?

shicks avatar Jun 02 '20 07:06 shicks