-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Conversation
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.
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. |
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: Not a huge problem, just wanted to mention it. |
Also general code clean up. This change temporarily disables the almond loader.
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. |
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 |
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.
Okay, I made everything work under Node 0.10 and also fixed some issues with |
Also clean up exec usage.
As expected, the problem with the almond loader was stupid, I needed to specify 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:
The only thing I think I have left from my side is to update the Eclipse triggers, which I may need some advice on. |
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 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. |
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 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. |
It was only exposed during the release tests.
Updates needed for the release guide |
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 startedGet the code
Build the codePrerequisites:
Cesium uses npm modules for development, so after syncing, you need to run
Once all modules have been installed, run
Cesium ships with a simple HTTP server for testing, run
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 development server has a few other options as well, which you can see by pasing the
While you can use the editor of your choice to develop Cesium, certain files, such as Build scriptsCesium uses gulp for build tasks, but they are all abstracted away by npm run scripts.
Specify the target(s) at the command line:
Next StepsAt 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 ExpertThis section has additional information on our development tools. Test Tips
Then, to step into the test, step into ResourcesContribute CodeSee CONTRIBUTING.md. Configure an IDEWe 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
The nice thing about WebStorm is that very little configuration is needed out of the box. Just browse to and open Gulp IntegrationAs we mentioned above, all of Cesium's build scripts use gulp. WebStorm has excellent gulp integration to make running WebStorm Shortcuts
Set up EclipseWhile primarily known as a Java IDE, Eclipse can be configured to work well for web development too.
Also consider the Optional Eclipse Configuration options below. Optional Eclipse ConfigurationThese steps are optional depending on your preference. GLSL PluginIf 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. GitMost of us use Git from the command-line, but there is also Eclipse integration:
Eclipse Tips
|
It would be nice to get this into master before Monday, that was it has a week to stew before the release. |
Note |
Thanks, fixed. |
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( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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('.')); |
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Clean up file path handling.
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'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should probably be clocPath
There was a problem hiding this comment.
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.
Okay, hopefully that was the last typo. |
|
||
function combineCesium(debug, optimizer, combineOutput) { | ||
return requirejsOptimize({ | ||
wrap : true, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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/')
There was a problem hiding this comment.
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.
ready (hopefully). |
Initial port of build process to gulp
I also updated the contributor's and release guides. |
Can you email the forum? |
Will do |
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.
npm
scripts for all gulp tasks so no one needs to install gulp globally.jshint-watch
task for continually running jsHint on changed files and writing the results to the console..jsHintrc
in addition tojsHintOptions
.TODO
Cesium
andCesiumUnminified
when only running combine.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: