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

Allow for easier custom builds #1979

Merged
merged 29 commits into from Apr 29, 2014
Merged

Allow for easier custom builds #1979

merged 29 commits into from Apr 29, 2014

Conversation

tschaub
Copy link
Member

@tschaub tschaub commented Apr 10, 2014

This changes a number of things in support of a configurable build tool:

  • *.exports files are removed in favor of @todo api annotation in the source files (this will become @api when we move on from Plovr)
  • a tasks/generate-symbols.js task produces a build/symbols.json file with metadata about exportable symbols
  • a tasks/generate-exports.js task produces a build/exports.js script with goog.exportProperty and goog.exportSymbol calls
  • the build.py script has been modified to call the above tasks

Currently, the way the generate-exports.js task is called, it generates an exports.js file that exports all exportable symbols. This task also supports generating exports for a subset of all symbols by passing it a JSON configuration file with a list of symbols or symbol patterns. An example configuration file might look like this:

{
  "exports": [
    "ol.Map",
    "ol.layer.Vector",
    "ol.layer.Vector#*",
    "ol.BrowserFeature.*"
  ]
}

Items in the exports array that end with * are patterns. Patterns match symbol names that start with the string before the *. The # indicates a prototype property. So the above configuration would generate exports for the ol.Map constructor, the ol.layer.Vector constructor, all vector layer methods (including inherited ones that are also exportable), and all constants in the ol.BrowserFeature namespace.

This is the first part of a build configuration. The next part is a set of options for the compiler. A config file with both of these parts will get passed to a tasks/build.js script that 1) generates exports, 2) sorts lib files in dependency order, and 3) drives the compiler.

@tschaub
Copy link
Member Author

tschaub commented Apr 10, 2014

Other things that can be supported with this setup:

  • building the library together with 3rd party code
  • organizing exportable symbols into packages (making for easier bulk export)
  • a web hosted build service that provides users options based on the symbols.json file

@tschaub
Copy link
Member Author

tschaub commented Apr 10, 2014

Regarding the build failure, I likely missed an export or two. It is tedious work. Will get back to it later.

This was referenced Apr 10, 2014
@tschaub
Copy link
Member Author

tschaub commented Apr 11, 2014

Another thing to note (based on the build failures). This means you can no longer make a symbol or property exportable without documenting it. Previously, it was possible to add something to an exports file that didn't appear anywhere in the source. For example, ol.format.WMSCapabilities.prototype.read was exported, but not documented.

@ahocevar mentioned that this discussion has happened before, but here's another summary:

If you want to export a method, it needs to be documented in the source.

Put another way, there is no longer a way to assign and document Parent#foo but only export Child#foo. If you don't want to export a method on a base class and only want to export methods on child classes (even if they don't override the parent), you need to document the child methods. See cc90d59 for an example of how to do this.

@ahocevar
Copy link
Member

dc82784 gets rid of regex parsing of source files for collecting exports.

@tschaub
Copy link
Member Author

tschaub commented Apr 11, 2014

Added a build task and documentation. Feedback welcome.

@marcjansen
Copy link
Member

My feedback so far: awesome!

On 11. April 2014 21:35:39 MESZ, Tim Schaub notifications@github.com wrote:

Added a build task and
documentation.
Feedback welcome.


Reply to this email directly or view it on GitHub:
#1979 (comment)

Marc Jansen, terrestris GmbH & Co. KG

http://www.terrestris.de/

@tschaub
Copy link
Member Author

tschaub commented Apr 11, 2014

The build task now lets you build 3rd party code together with the library. I updated the doc to describe the src config property. Below is a config that builds the library together with the simple.js example:

{
  "exports": [],
  "src": ["src/**/*.js", "examples/simple.js"],
  "compile": {
    "js": ["externs/olx.js"],
    "externs": [
      "externs/bingmaps.js",
      "externs/geojson.js",
      "externs/oli.js",
      "externs/proj4js.js",
      "externs/tilejson.js",
      "externs/topojson.js",
      "externs/vbarray.js"
    ],
    "define": [
      "goog.dom.ASSUME_STANDARDS_MODE=true",
      "goog.DEBUG=false"
    ],
    "compilation_level": "ADVANCED_OPTIMIZATIONS",
    "output_wrapper": "(function(){%output%})();",
    "use_types_for_optimization": true
  }
}

Note that the externs/olx.js file is included as a js option to the compiler. This file is not included as a src file because it doesn't include goog.require calls. The js option can be used to force the inclusion of files after all other source files.

I could use another set of eyes to determine why this is significantly different than the result of build.py build/examples/simple.combined.js (50K bigger). We should expect some difference because this is a different version of Closure Compiler and a different version of Closure Library, but I wouldn't expect a difference as significant as it is. Builds of the whole library with this task and using Plovr are closer in size. Without Plovr, we lose the ability to strip type suffixes and name suffixes, but this difference has been minor when I've compared it in the past. You can run with the log level set to "silly" to see the complete details of the compile command.

node tasks/build.js simple.json simple.min.js --loglevel silly

@ahocevar
Copy link
Member

@tschaub I'll try to help you figure out why the sizes of compiled examples plus library is so significantly different on Monday.

In the meantime, I have changed the JSDoc exports plugin to make it readable by anyone, made the whole documentation story really consistent, and documented the few remaining ol3 specific conventions for JSDoc usage: 494f238. Since this is based on your work here, I think it would make sense to make it either part of this pull request, or rebase it once this pull request is merged. So feel free to cherry-pick or not 😄.

@elemoine
Copy link
Member

@tschaub I'll try to help you figure out why the sizes of compiled examples plus library is so significantly different on Monday.

@ahocevar I'm interested to understand what you mean by that.

@ahocevar
Copy link
Member

I could use another set of eyes to determine why this is significantly different than the result of build.py build/examples/simple.combined.js (50K bigger).

@elemoine This is what I was referring to, and what I wanted to help figure out. However, I'd much appreciate if you could also take a look, because you know more about the Closure compiler than I do.

@ahocevar
Copy link
Member

Another commit that builds on top of this work: 74b525b - it adds usage documentation for oli.js and olx.js.

@ahocevar
Copy link
Member

A far as I can tell, a lot of unused goog.async is included in that build. I have not found out yet why though.

@ahocevar
Copy link
Member

I could use another set of eyes to determine why this is significantly different than the result of build.py build/examples/simple.combined.js (50K bigger).

Success! And the file size is now even smaller than with plovr. All you need to do is add "manage_closure_dependencies": true to the "compile" option. To make the built file actually work with the example, externs/example.js needs to be added to the externs. So the full configuration looks like this:

{
  "exports": [],
  "src": ["src/**/*.js", "examples/simple.js"],
  "compile": {
    "js": ["externs/olx.js"],
    "externs": [
      "externs/example.js",
      "externs/bingmaps.js",
      "externs/geojson.js",
      "externs/oli.js",
      "externs/proj4js.js",
      "externs/tilejson.js",
      "externs/topojson.js",
      "externs/vbarray.js"
    ],
    "define": [
      "goog.dom.ASSUME_STANDARDS_MODE=true",
      "goog.DEBUG=false"
    ],
    "compilation_level": "ADVANCED_OPTIMIZATIONS",
    "output_wrapper": "(function(){%output%})();",
    "use_types_for_optimization": true,
    "manage_closure_dependencies": true
  }
}

@tschaub
Copy link
Member Author

tschaub commented Apr 14, 2014

Awesome @ahocevar - thanks for the find. Previously, I was always compiling with a "main" script that goog.required what it needed, so this same effect was achieved. The closure-util package accepts separate lib and main options when determining dependencies, but I was trying to keep things simpler in the build config here.

With "manage_closure_dependencies": true the dependency resolution (currently done by closure-util) could be removed altogether, speeding up the build time as a result. I'll look into this now.

@tschaub
Copy link
Member Author

tschaub commented Apr 14, 2014

Ok, it turns out that the build time is not reduced by having Closure Compiler do all the dependency resolution. Here are the two scenarios I tried:

  1. Throw everything at the Closure Compiler. This is all of our src/**/*.js, all of Closure Library's goog/**/*.js, the third_party/closure/goog/**/*.js sources, our externs/olx.js, and examples/simple.js. That's 1214 sources. Let the compiler figure out what is and what isn't goog.required (using "manage_closure_dependencies": true).
  2. Throw everything at the closure-util library. This is 1213 of the sources mentioned above (externs/olx.js doesn't go to closure-util in this case). Let it figure out what is and what isn't goog.required (this generates a full AST for all sources and walks it for goog.require and goog.provide calls). Then throw a subset of files at Closure Compiler. This turns out to be 349 sources.

Using the same compiler options as above (mentioned in @ahocevar's comment), I timed node tasks/build.js simple.json simple.min.js in the two scenarios above.

Scenario 1:

$ time node tasks/build.js simple.json simple.min.js && ls -l simple.min.js
info ol Compiling 1213 sources

real    0m21.976s
user    0m54.993s
sys 0m1.332s
-rw-r--r--  1 tschaub  staff  118638 Apr 14 10:36 simple.min.js

Scenario 2:

$ time node tasks/build.js simple.json simple.min.js && ls -l simple.min.js
info ol Parsing dependencies
info ol Compiling 348 sources

real    0m17.207s
user    0m44.966s
sys 0m1.253s
-rw-r--r--  1 tschaub  staff  117702 Apr 14 10:35 simple.min.js

(The externs/olx.js "source" isn't included in the count logged here because closure-util doesn't parse it, but the compiler gets it - hence the 1214 and 349 counts above.)

So the second scenario is ~4s faster (10 fewer CPU seconds) and produces a build that is ~1K smaller. This means passing everything to Closure Compiler is not necessarily the best idea. The closure-util package is faster at parsing sources and resolving dependencies. Still not sure what the 1K build diff is about. Need a bit more testing to ensure everything works as expected.

Anyway, this is very promising to me.

@ahocevar
Copy link
Member

abae3e8 inlines typedefs from olx.js into the class documentation. This does not look good currently with nested object literals of object literals, but that should improve with a new template.

@tschaub
Copy link
Member Author

tschaub commented Apr 15, 2014

@ahocevar and I just discussed making another simplification here.

The @stability annotation is going to be removed in favor of the @api annotation. The value of the @api annotation will be a measure of stability. The default stability level will be experimental. Properties that are not exportable will not be documented (i.e. those with no @api annotation). Where we explicitly call goog.exportProperty in the code (e.g. ol.Map#getView), @api annotations will be added. Though we know that users cannot generate builds without these symbols, it is important that they are documented and present in the hosted build tool (otherwise people who use getView et al. would be confused about why it didn't look like it was available when making a custom build).

To support the idea of packages, we can later add a separate @package annotation.

@ahocevar
Copy link
Member

52dadc7 implements the changes discussed above.

@tschaub
Copy link
Member Author

tschaub commented Apr 17, 2014

Thanks for the Herculean effort on 52dadc7. I have a couple questions and then need to look into excluding @enum and @typedef from the symbols.json file.

@ahocevar
Copy link
Member

@typedef, @enum (and @event) are already excluded from symbols.json. Or are you concerned about side effects of this? The only one I could think of is that we won't ever be able to provide something like ol.geom.GeometryType for applications to iterate over. This is related to the decision made in #954.

@ahocevar
Copy link
Member

As discussed, 6227bd2 moves the exclusion of @typedef and @event to buildcfg/jsdoc/symbols/publish.js, so all excludes are now handled in one central place.

@ahocevar
Copy link
Member

Another small commit that addresses formatting issues found by @probins: 5f8a1e8.

@tschaub
Copy link
Member Author

tschaub commented Apr 17, 2014

Remaining tasks:

  • Add goog.require calls to build/exports.js so it can be safely used with the manage_closure_dependencies option. This is addressed in 9613b93 and 378b79c.
  • Add @defines to the build/symbols.json so the hosted build tool can let the user choose which to set.
  • Make generate-exports.js take an output file path (this is required to support concurrent builds in the hosted build tool). This is addressed in 1a73a3a.

Update - I'll handle the defines in a separate pull request after this is merged.

@probins
Copy link
Contributor

probins commented Apr 18, 2014

a couple of comments after playing around with the build task a bit:

  • in the config examples, output_wrapper should surely be quoted (quotes within quotes), or you get a compiler error, no?
    "output_wrapper": "'(function(){%output%})();'"
  • why all the extern files in the simple example? These aren't used/needed by this example, so can surely be removed
  • istm it would be better if the dependency build and the compile were separate tasks; particularly now the api is reasonably stable, most code changes that require a recompile don't affect the dependency tree, so the same output could be used as input to the compiler
  • aiui, this build task is aimed at advanced_optimizations; it's much less suited for simple/whitespace. With closureBuilder, I can define which namespaces should be included, for example in the simple example (which only uses a small number), and it will hunt through the sources to assemble a dependency tree; I can't really do that with this task. It will include all the sources listed in src - or am I missing something?

@tschaub
Copy link
Member Author

tschaub commented Apr 18, 2014

in the config examples, output_wrapper should surely be quoted (quotes within quotes)

Without the extra quotes, I get the expected output (and no errors):

(function(){/**... source here */})();

This may vary by OS. I'm on OSX.

@tschaub
Copy link
Member Author

tschaub commented Apr 18, 2014

With closureBuilder, I can define which namespaces should be included

Are you talking about a specific Closure Compiler option?

With the manage_closure_dependencies option, the behavior is as you describe (the Compiler will remove anything that is not explicitly goog.required). See the comment above for an example.

@tschaub
Copy link
Member Author

tschaub commented Apr 18, 2014

it would be better if the dependency build and the compile were separate tasks

Not sure I understand. This is the way things are currently. As mentioned above, once the generate-exports.js task adds goog.require calls to the exports.js file, this will add more flexibility for using the Compiler independently.

ahocevar and others added 12 commits April 29, 2014 09:53
This commit simplifies the exports.js plugin so it only relies
on the stability notes to generate the documentation, which
completely decouples it from the exportable API.

As a rule of thumb, whenever something has an 'api' annotation,
it should also have a 'stability' annotation. A more verbose
documentation of ol3 specific annotation usage is available in
the new 'apidoc/readme.md' file.

This commit also modifies all source files to implement these
usage suggestions.
This change adds a stability value to the api annotation, with
'experimental' as default value.

enum, typedef and event annotations are never exportable, but
api annotations are needed there to make them appear in the
docs.

Nested typedefs are no longer inlined recursively, because the
resulting tables get too wide with the current template.
Because ol.proj.EPSG4326 et al. extend ol.proj.Projection which has exportable methods, these constructors need to be exportable as well (e.g. so ol.proj.EPSG4326.prototype is defined in exports.js when calling goog.exportProperty on getCode etc.).  If we really don't want these to be exportable, they should be removed or made private (and named like ol.proj.EPSG4326_) for internal use only.
With this change, the only two remaining generated scripts are build/exports.js and build/test/requireall.js.  Both are only required by Plovr.  With the Node based build task, a temporary exports.js file is created.  The Node based server can be used to run the tests without build/test/requireall.js.
The build/exports.js file passes gjslint and jshint, so the separate linting task is not needed.
@tschaub
Copy link
Member Author

tschaub commented Apr 29, 2014

Thanks for the review @elemoine. Rebased with your comments addressed. I'll merge when the build passes.

Thanks for the great collaboration on this @ahocevar. I'm looking forward to making additional changes to better support the hosted build tool, and there will likely be other changes once we further test the builds, but I think this is a solid foundation to work on.

tschaub added a commit that referenced this pull request Apr 29, 2014
Annotation for exportable methods.  Node based tasks for generating exports and a custom build.

Fixes #613.
@tschaub tschaub merged commit 12d4cb5 into openlayers:master Apr 29, 2014
@tschaub tschaub deleted the custom-build branch April 29, 2014 19:01
@bartvde
Copy link
Member

bartvde commented Apr 30, 2014

now that this is in, I'll cut beta5 unless I hear any objections to do so in the next hour or so

@elemoine
Copy link
Member

now that this is in, I'll cut beta5 unless I hear any objections to do so in the next hour or so

+1 for me. Thanks Bart. (The mailing list may have been a better a place for this.)

@elemoine
Copy link
Member

Previously, I was always compiling with a "main" script that goog.required what it needed, so this same effect was achieved. The closure-util package accepts separate lib and main options when determining dependencies, but I was trying to keep things simpler in the build config here.

So what should I do if want to build my application and ol3 together? Should I add both ol3/src/**/*.js and the paths to my script to the "src" array (as done in your example)? Or should I use "main" for my script? Thanks for any response.

@elemoine
Copy link
Member

I built the vector-layer example with the following config:

{
  "exports": [],
  "src": ["src/**/*.js", "examples/vector-layer.js"],
  "compile": {
    "js": ["externs/olx.js"],
    "externs": [
      "externs/bingmaps.js",
      "externs/example.js",
      "externs/geojson.js",
      "externs/oli.js",
      "externs/proj4js.js",
      "externs/tilejson.js",
      "externs/topojson.js",
      "externs/vbarray.js"
    ],
    "define": [
      "goog.dom.ASSUME_STANDARDS_MODE=true",
      "goog.DEBUG=false"
    ],
    "compilation_level": "ADVANCED_OPTIMIZATIONS",
    "output_wrapper": "(function(){%output%})();",
    "use_types_for_optimization": true
  }
}

The resulting build's size is 66K (gzipped), while vector-layer.combined.js built with Plovr is 56K.

Can I get the same build size? How?

Thanks.

@ahocevar
Copy link
Member

@elemoine Add "manage_closure_dependencies": true as additional compiler option at the bottom.

@elemoine
Copy link
Member

Ok, I saw your comment about this indeed. But @tschaub also said that closure-lib is faster than the compiler at resolving dependencies. So I don't know what should be used.

@elemoine
Copy link
Member

But yes using manage_closure_dependencies makes a big difference. I now get 55K. I need to understand what manage_closure_dependencies does and why that significantly reduces the build size in that case.

@ahocevar
Copy link
Member

If you use this setting, the already reduced list of source files (which
would give you the 66k build) is run through another pass at the compiler
level. This slows down things a bit, though of course much less throwing
all source files at the compiler.

@elemoine
Copy link
Member

Thanks Andreas. Interested in information @tschaub may have on this.

@elemoine
Copy link
Member

I also get good results by adding goog.provide('app') to vector-layer.js and with this config file:

{
  "exports": [],
  "src": ["src/**/*.js"],
  "compile": {
    "js": ["examples/vector-layer.js", "externs/olx.js"],
    "closure_entry_point": "app",
    "externs": [
      "externs/bingmaps.js",
      "externs/example.js",
      "externs/geojson.js",
      "externs/oli.js",
      "externs/proj4js.js",
      "externs/tilejson.js",
      "externs/topojson.js",
      "externs/vbarray.js"
    ],
    "define": [
      "goog.dom.ASSUME_STANDARDS_MODE=true",
      "goog.DEBUG=false"
    ],
    "compilation_level": "ADVANCED_OPTIMIZATIONS",
    "output_wrapper": "(function(){%output%})();",
    "use_types_for_optimization": true
  }
}

Here "examples/vector-layer.js" (path to the main app script) is added to the "js" array, and "closure_entry_point" is used.

But that still does not explain why having the compiler "manage" dependencies reduces the build size. That doesn't make too much sense to me.

@probins
Copy link
Contributor

probins commented May 29, 2014

But that still does not explain why having the compiler "manage" dependencies reduces the build size. That doesn't make too much sense to me.

@elemoine manage_closure_dependencies drops files with symbols that are never used. See the --help "If an input provides symbols, and those symbols are never required, then that input will not be included in the compilation." If you add the --output_manifest param, you'll get a list of files; if you run that both with and without manage... and do a diff, you can find which ones are being left out. I just tried a full build on the latest master; build.py output is 348k, tasks/build without manage... 351k and with 350k; the difference is mainly libtess stuff but some others as well. I'm not sure what the diff is with build.py, but it could well simply be because it's using an older version of lib/compiler.

@elemoine
Copy link
Member

elemoine commented Jun 2, 2014

Thanks @probins. I wonder if manage_closure_dependencies is used when using build.py.

@probins
Copy link
Contributor

probins commented Jun 2, 2014

I would imagine that plovr uses it, but I know little about plovr

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

Successfully merging this pull request may close these issues.

None yet

6 participants