Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

closurebuilder.py doesn't track requireType calls #1076

Open
niloc132 opened this issue May 23, 2020 · 4 comments
Open

closurebuilder.py doesn't track requireType calls #1076

niloc132 opened this issue May 23, 2020 · 4 comments

Comments

@niloc132
Copy link

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 803aa52#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 803aa52 locally.

@12wrigja
Copy link
Collaborator

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)?

@niloc132
Copy link
Author

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
Copy link
Author

...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.

@shicks
Copy link
Member

shicks commented Jun 2, 2020

@tjgq maybe you have some insight into this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants