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

Initial port of build process to gulp #3106

Merged
merged 22 commits into from
Oct 22, 2015
Merged

Initial port of build process to gulp #3106

merged 22 commits into from
Oct 22, 2015

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Oct 16, 2015

This is still a work in progress but I'm opening up for early feedback. Everything works, I just need to verify correctness and clean up the code.

  1. Move all build related code to gulpfile.js
  2. Add npm scripts for all gulp tasks so no one needs to install gulp globally.
  3. Added new jshint-watch task for continually running jsHint on changed files and writing the results to the console.
  4. Generate Sandcastle specific .jsHintrc in addition to jsHintOptions.

TODO

  • Change Eclipse external tools/builders to use new npm targets
  • Get almond loader working again.
  • Determine if we can have WebStorm FileWatchers use new npm targets (or need another solution)
  • Verify everything
  • Make sure we duplicate unminified Cesium to both Cesium and CesiumUnminified when only running combine.
  • Clean up path manipulation and verify unix works too.
  • Extract out common code and variables (there's currently a lot of duplication).
  • Fix travis.

This is mostly a straight-port with some gulpish-ness added. There's a lot of things we can do to further improve things. There will probably be in future pull requests. Here are some ideas:

1. Move all build related code to gulpfile.js
2. Add `npm` scripts for all gulp tasks so no one needs to install gulp globally.
3. Added new `jshint-watch` task for continually running jsHint on changed files and writing the results to the console.
@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 17, 2015

A big 👍 from me on the general direction. I am also all for incrementally merging build improvements into master as we make them.

@kring or @shunter do you want to look at this?

@mramato captain obvious here, but please update the Contributor's Guide when this is merged. It could be useful to even provide the update as a comment in this PR before merging.

@kring
Copy link
Member

kring commented Oct 18, 2015

I haven't looked at this in a lot of detail yet, but thanks for doing it @mamato!

One thing I noticed in a few minutes playing with it:
The combine task uses execSync, which requires Node 0.12. All my machines have 0.10 because that's the latest included with Ubuntu by default, which is what we use for our servers. If you care, there's an npm module that provides execSync on 0.10:
https://www.npmjs.com/package/execSync

Not a huge problem, just wanted to mention it.

Also general code clean up.

This change temporarily disables the almond loader.
@mramato
Copy link
Contributor Author

mramato commented Oct 18, 2015

Not a huge problem, just wanted to mention it.

Thanks for the heads up. If it's easy enough to support Node 0.10, then I have no problem doing so, but I'll probably wait until everything else is done to verify/test/fix it. I have to imagine 0.10 is close to end of life considering it's age and the fact it's two major versions behind.

@mramato
Copy link
Contributor Author

mramato commented Oct 18, 2015

I just submitted a change that switches over to using the npm module for the r.js optimizer. Calling it programmatically cleans up the code quite a bit. This also has the affect of updating the version we are using, the one we had was pretty old.

I had to temporarily disable the almond loader because it doesn't seem to like being run unless Source is the current working directory and because of the asynchronous nature of node and gulp, setting the current directory temporarily isn't really an option. I'm sure there's a brain-dead setting I'm missing somewhere, so if anyone has any ideas, let me know. @shunter I'll probably bug you at some point if I can't figure it out.

Go back to using it for Travis.
Also fix an issue causing makeZipFile to have incomplete data on Linux.
1. Remove debug code
2. Fix documentation github links
3. Fix accidental include of development gallery directory.
@mramato
Copy link
Contributor Author

mramato commented Oct 18, 2015

Okay, I made everything work under Node 0.10 and also fixed some issues with makeZipFile under Unix.

Also clean up exec usage.
@mramato mramato mentioned this pull request Oct 18, 2015
@mramato
Copy link
Contributor Author

mramato commented Oct 18, 2015

As expected, the problem with the almond loader was stupid, I needed to specify almond instead of almond.js.

I've also determined that WebStorm FileWatchers don't have good cross-platform, so we'll just rely on "run forever" gulp tasks to do what we need for now. There are 2 new tasks for this purpose, they can also be used with any editor or in a terminal window:

  • jsHint-watch runs jsHint on a file after every save.
  • build-watch runs the build target whenever a glsl or Spec .js file is modified (I did this instead of whenever any file was modified to reduce unnecessary CPU use.

The only thing I think I have left from my side is to update the Eclipse triggers, which I may need some advice on.

@mramato
Copy link
Contributor Author

mramato commented Oct 19, 2015

So I've run into the same problem with Eclipse that I did with WebStorm, I can easily configure the Eclipse auto-builder to point to gulp.cmd as an external tool, but this will only work on Windows because on Unix systems I need it to point to gulp. Pointing to just gulp on windows doesn't work because a file named gulp already exists, so it uses that instead of resolving to the cmd version.

Hopefully there's an easy solution here that I'm not seeing. My only idea would be to retain a small (for Eclipse only) ant build script that detects the platform and delegates to the proper gulp command, but that seems like overkill. Is there a way to have platform-dependent external tool targets in Eclipse?

@shunter I'm hoping you can swoop in and save the day here.

@mramato
Copy link
Contributor Author

mramato commented Oct 19, 2015

Talked to @shunter offline, he pointed me in the right direction and all of the Eclipse launch configurations now call gulp tasks instead of ant.

WebStorm has no good way to have cross-platform File Watchers (their term), so that's no solution for now. I did find this issue, which will work when addressed. I also may write up a new issue for them to provide a macro for node executable (which they already have in other places but not File Watchers)..

I also discovered that our old process had a bug regarding BOMs and non-english characters, so the new output differs slightly from the old. I'm going to do a build of both the new and old systems and put them up for anyone that wants to compare them.

Unless bugs are found, I don't expect anymore code changes, so this is ready for a full review.

@mramato
Copy link
Contributor Author

mramato commented Oct 20, 2015

I uploaded two full release zips from this branch (one with gulp and one with ant) in case anyone wanted to compare output.

Gulp
Ant

I found an fixed a few issues while going through the release process. As far as I'm aware, everything is working perfectly.

@mramato
Copy link
Contributor Author

mramato commented Oct 21, 2015

Updates needed for the release guide
7 goes away
8 1.0.0 becomes 1.14.0 for clarity, get rid of second sentence.
12 ./Tools/apache-ant-1.8.2/bin/ant makeZipFile -> npm run makeZipFile
13 node server.js -> npm start
16 node server.js -> npm start

@mramato
Copy link
Contributor Author

mramato commented Oct 21, 2015

I took a pass on updating the Contributor's Guide to discuss both gulp and WebStorm. I also have a lot of ideas for further improvements that I'll write up an issue for at some point (such as discussing how to add a test). Here it is, since you can't do PRs on a wiki.

We are a community that encourages contributions. Join us. Here's how.

Get started

Get the code

  • Setup Git if it isn't already (link).
    • Make sure your SSH keys are configured (linux | mac | windows).
    • Double-check your settings for name and email: git config --get-regexp user.*.
    • Recommended Git settings:
      • git config --global push.default simple - when running git push, only push the current branch (default in Git 2.0).
      • git config --global pull.rebase preserve - when pulling remote changes, rebase your local changes on top of the remote changes, to avoid unnecessary merge commits.
      • git config --global fetch.prune true - when fetching remote changes, remove any remote branches that no longer exist on the remote.
  • Have commit access to cesium?
    • No
      • Fork cesium.
      • Create a local repo of your fork, e.g., git clone git@github.com:yourusername/cesium.git.
      • We always recommend making changes in a branch, e.g., git checkout -b myFeature.
    • Yes
      • Create a local cesium repo, e.g., git clone git@github.com:AnalyticalGraphicsInc/cesium.git
      • Make changes in a branch, e.g., git checkout -b myFeature.

Build the code

Prerequisites:

Cesium uses npm modules for development, so after syncing, you need to run npm install from the Cesium root directory:

npm install

Once all modules have been installed, run npm run build to actually build the code:

npm run build

Cesium ships with a simple HTTP server for testing, run npm start after building to use it:

npm start

Then browse to http://localhost:8080/.

By default, the server only allows connections from your local machine. Too allow connections from other machines, pass
the --public option to npm. Note the extra -- is intentional and required by npm.

npm start -- --public

The development server has a few other options as well, which you can see by pasing the --help parameter:

npm start -- --help

While you can use the editor of your choice to develop Cesium, certain files, such as glsl and new tests, require that
the build task be executed in order for the changes to take affect. You can use the build-watch script to have this
happen automatically.

Build scripts

Cesium uses gulp for build tasks, but they are all abstracted away by npm run scripts.
Here's the full set of scripts and what they do.

  • build - A fast, developer-oriented build that prepares the source tree for use as standard Asynchronous Module Definition (AMD) modules, suitable for running tests and most examples (some Sandcastle examples require running combine). This runs automatically when saving files in Eclipse.
  • build-watch - A never-ending task that watches your file system for changes to Cesium and runs build on the source code as needed.
  • combine - Runs build, plus the the RequireJS optimizer to combine Cesium and the Almond AMD loader to produce all-in-one files in the Build/Cesium directory that expose the entire Cesium API attached to a single global Cesium object. This version is useful if you don't want to use the modules directly with a standard AMD loader.
  • minify - Runs combine, plus minifies Cesium.js using UglifyJS2 for a smaller deployable file.
  • combineRelease - Runs combine, plus uses the optimizer to remove debugging code that validates function input and throws DeveloperErrors. The removed sections are marked with //>>includeStart('debug', pragmas.debug); blocks in the code.
  • minifyRelease - Runs minify, and removes debugging code.
  • buildApps - Builds the example applications (such as Cesium Viewer) to produce self-contained, minified, deployable versions in the Build directory.
  • generateDocumentation - Generates HTML documentation in Build/Documentation using JSDoc 3.
  • release - A full release build that creates a shippable product, including building apps and generating documentation.
  • instrumentForCoverage - Runs JSCoverage on the source tree to allow running tests with coverage information. Use the link in index.html. Currently Windows only.
  • jsHint - Runs JSHint on the entire source tree.
  • jsHint-watch - A never-ending task that watches your file system for changes to Cesium and runs JSHint on any changed source files.
  • makeZipFile - Builds a zip file containing all release files. This includes the source tree (suitable for use from an AMD-aware application), plus the combined and minified Cesium.js files, the generated documentation, the test suite, and the example applications (in both built and source form).
  • clean - Removes all generated build artifacts.
  • cloc - Runs CLOC to count the lines of code on the Source and Specs directories. This requires Perl to execute.
  • sortRequires - Alphabetically sorts the list of required modules in every js file. It also makes sure that the top of every source file uses the same formatting.

Specify the target(s) at the command line:

npm run [target-name]

Next Steps

At this point you are ready to contribute. Continue reading below for more details on the developer setup, or read about Cesium's architecture; check out the roadmap; join the forum; and start hacking.

Become an Expert

This section has additional information on our development tools.

Test Tips

  • To debug an individual test (spec), open the browser's debugger, e.g., Ctrl-Shift-I in Chrome, and click debug to the far right of the test.

Then, to step into the test, step into stepIntoThis()

Resources

Contribute Code

See CONTRIBUTING.md.

Configure an IDE

We encourage contributors to use their IDE of choice, but many of us use WebStorm or Eclipse. Here's how we set then up.

Set up WebStorm

  • Install WebStorm if you don't already have it. While it's a commercial IDE,
    it's pretty cheap and there's a 30-day free trial available.

The nice thing about WebStorm is that very little configuration is needed out of the box. Just browse to and open
the WebStorm project included with Cesium. Most things a Web Developer wants are built in, but whenever you open a file
that has a plugin available, such as md, glsl, or .gitignore, WebStorm will ask if you want to install it.
Simply say yes and off you go. While most of us still use git on the command-line, WebStorm includes excellent tools
for merging, diffing, and managing branches as well.

Gulp Integration

As we mentioned above, all of Cesium's build scripts use gulp. WebStorm has excellent gulp integration to make running
tasks from the IDE simple. Just right-click on gulpfile.js in the Project tree and select Show Gulp Tasks, now
you can simply double click on any task to run it. Even better, perpetual tasks like build-watch and jsHint-watch
will get their own output tab that automatically updates.

WebStorm Shortcuts

  • Use Ctrl-Shift-A to search for settings, shortcuts, or anything else you want to do.
  • Use Ctrl-Shift-N to search and open files in the workspace.
  • Use Ctrl-ALT-L to auto-format an entire file or the selected block of code.
  • Use Ctrl-Shift-F to bring up the global find dialog.

Set up Eclipse

While primarily known as a Java IDE, Eclipse can be configured to work well for web development too.

  • Install Java if it isn't already.
  • Download the Eclipse IDE for Java Developers. Extract to a directory of your choice. Run it.
  • Install additional Eclipse components: JavaScript Development Tools and Eclipse Web Developer Tools.
    • Help - Install New Software. Work with: select Mars from the list.
    • Expand Programming Languages, check JavaScript Development Tools.
    • Expand Web, XML, Java EE and OSGi Enterprise Development, check Eclipse Web Developer Tools.
    • Next, Next, Accept, Finish, wait, No (we have more to install).

The Java Development Tools installation dialog

  • Install the JSHint plugin:
    • Help - Install New Software. Work with: http://github.eclipsesource.com/jshint-eclipse/updates/.
    • Check JSHint. Next, Next, Accept, Finish, wait, OK, wait, Restart.

The JSHint installation dialog

  • Run Eclipse. Close the Welcome page.
  • Window - Preferences:
    • General - Editors - Text Editors. Check Insert spaces for tabs. Apply.
    • Web - CSS Files - Editor. Switch the radio button to "Indent using spaces". Change Indentation size to 4. Apply.
    • Web - HTML Files - Editor. Switch the radio button to "Indent using spaces". Change Indentation size to 4. OK.

Configuring "Insert spaces for tabs"

  • Import Cesium into your workspace: File - Import, General - Existing Projects into Workspace, Next. Fill in the path to the root Cesium directory, Finish.
  • Click the "Open Perspective" button in the upper right and select JavaScript. You can then right-click on the Java perspective and close it.
  • Window - Show View - Console.

Also consider the Optional Eclipse Configuration options below.

Optional Eclipse Configuration

These steps are optional depending on your preference.

GLSL Plugin

If you edit WebGL shader files (.glsl) with Eclipse, install GLShaders for GLSL syntax highlighting. First exit Eclipse, then download GLShaders and extract into Eclipse's dropins directory.

The Eclipse dropins directory

Git

Most of us use Git from the command-line, but there is also Eclipse integration:

  • Window - Preferences: Team - Git - Configuration
    • Verify Location in User Settings tab is set to .gitconfig in the default repository directory.
    • Verify Location in Systems Settings tab is set to {Installed/Git/Location}/etc/gitconfig.
  • Right click on Cesium in the Script Explorer. Team - Share project. Select Git, Next. Check Use or create repository in parent directory of project. Finish.

Eclipse Tips

  • Use Ctrl-Shift-R to search and open files in the workspace.

The Open Resource dialog

  • Use Ctrl-Shift-F to auto format selected code.

@mramato
Copy link
Contributor Author

mramato commented Oct 21, 2015

It would be nice to get this into master before Monday, that was it has a week to stew before the release.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 21, 2015

Not the extra -- is intentional and required by npm.

Note

@mramato
Copy link
Contributor Author

mramato commented Oct 21, 2015

Note

Thanks, fixed.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 21, 2015

Specify the target(s) at the command line:

Perhaps put this at the top of that section.

var cloc_definitions = path.join('Tools', 'cloc-1.60', 'cloc_definitions');

var cmdLine;
return Promise.join(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these be sequential rather than concurrent? Won't it jumble together the output?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The callback isn't called until the process terminates and stdout and stderr will be buffers that contain the complete output of the function, so there's no way for them to be jumbled. However, it is possible for the Specs section to print out before the Source section, so I'll chain them together just so the output is always guaranteed the same.

var moduleId = filePathToModuleId(file);

var baseName = path.basename(file);
var propertyName = baseName.substring(0, baseName.lastIndexOf('.'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably would be more understandable to use the path functions -

var propertyname = path.basename(file, path.extname(file))


gulp.task('build', function(done) {
mkdirp.sync('Build');
glslToJavaScript(false, 'Build/minifyShaders.state');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are always passing in false for shader minification. You can see in the output that all the shader comments are left, even in the minified release build.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this was the very first thing I ported and by the time I was testing it completely slipped my mind.

@mramato
Copy link
Contributor Author

mramato commented Oct 21, 2015

Thanks for the review @shunter, anything else?


gulp.task('cloc', ['build'], function() {
var cmdLine;
var clokPath = path.join('Tools', 'cloc-1.60', 'cloc-1.60.pl');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably be clocPath

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm blaming this one on the Dayquil.

@mramato
Copy link
Contributor Author

mramato commented Oct 22, 2015

Okay, hopefully that was the last typo.


function combineCesium(debug, optimizer, combineOutput) {
return requirejsOptimize({
wrap : true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we bump the logLevel ? As far as I can tell, the default from when we ran as a separate program was level 0 (TRACE) but now the default seems to be silent.

Up to you, but otherwise it would be nice to add some log statements saying what it's doing, because right now it sits silently for 10 minutes with no output at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They still claim the default is 0, but obviously that's not the case. Manually setting the logLevel back to 0 gets us back to the old output.

My one argument for keeping it as is would be to reduce the amount of junk in the travis log file, so that if travis fails you don't have to scroll forever to figure out why. I did a quick experiment where I changed createBoxGeometry to misspell define and then ran combine, the build fails as soon as it his the problem and splits our a callstak with the below error on top.

Error: Error: Parse error using esprima for file: C:/Git/AnalyticalGraphicsInc/cesium/Source/Workers/createBoxGeometry.js

Here's an example of a successful travis in this branch compared to another with the current process.

Ultimately, I don't feel that strongly about it, so if you want me to just set it back to the old loglevel of 0, I can do that. Just let me know either way.


var assignmentName = path.basename(file, path.extname(file));
assignmentName = "['" + assignmentName + "']";
if (moduleId.indexOf('Source/Shaders/') === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check doesn't work. Source/ is not part of the moduleId. Should just be checking for `indexOf('Shaders/')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definitely worked at one point, but I think that's because of another bug that I have since fixed (which broke this). It's fixed now either way.

@mramato
Copy link
Contributor Author

mramato commented Oct 22, 2015

ready (hopefully).

shunter added a commit that referenced this pull request Oct 22, 2015
Initial port of build process to gulp
@shunter shunter merged commit 1ae0fe2 into master Oct 22, 2015
mramato added a commit that referenced this pull request Oct 22, 2015
@mramato
Copy link
Contributor Author

mramato commented Oct 22, 2015

I also updated the contributor's and release guides.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 22, 2015

Can you email the forum?

@shunter shunter deleted the gulp branch October 22, 2015 21:23
@mramato
Copy link
Contributor Author

mramato commented Oct 22, 2015

Will do

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

4 participants