Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Build: Migrate to grunt 0.4. #867
Conversation
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
jzaefferer
Dec 23, 2012
Owner
Btw. to test just install grunt-cli
globally, then you can npm install
when switching branches and get the right local grunt version.
Btw. to test just install |
mikesherov
reviewed
Dec 23, 2012
+ jshintrc: ".jshintrc" | ||
+ }, | ||
+ files: { | ||
+ src: [ "grunt.js", "build/**/*.js" ] |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
mikesherov
reviewed
Dec 23, 2012
+ "grunt-css": "0.5.1", | ||
+ "grunt-compare-size": "0.3.1", | ||
+ "grunt-html": "0.3.0", | ||
+ "grunt-git-authors": "1.1.0-beta.1", | ||
"rimraf": "2.0.1", |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
mikesherov
Dec 23, 2012
Member
You should add grunt-cli
to the package.json file so users, including Jenkins, can run grunt
after npm install
and be certain to have the right one.
mikesherov
Dec 23, 2012
Member
You should add grunt-cli
to the package.json file so users, including Jenkins, can run grunt
after npm install
and be certain to have the right one.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
scottgonzalez
Dec 24, 2012
Owner
I'd really like to discuss this with the rest of the team. I can't help but feel this is very wrong. grunt-cli should be installed globally before trying to grunt any project; it should be treated as a system-level dependency, like make or java.
scottgonzalez
Dec 24, 2012
Owner
I'd really like to discuss this with the rest of the team. I can't help but feel this is very wrong. grunt-cli should be installed globally before trying to grunt any project; it should be treated as a system-level dependency, like make or java.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
jzaefferer
Dec 24, 2012
Owner
As I said above: "Btw. to test just install grunt-cli globally, then you can npm install when switching branches and get the right local grunt version.". Jenkins should do the same, then grunt-cli will take care of running whatever grunt version is installed locally. That works great with both 0.3 and 0.4, so I agree with Scott, we shouldn't add grunt-cli as a dependency.
jzaefferer
Dec 24, 2012
Owner
As I said above: "Btw. to test just install grunt-cli globally, then you can npm install when switching branches and get the right local grunt version.". Jenkins should do the same, then grunt-cli will take care of running whatever grunt version is installed locally. That works great with both 0.3 and 0.4, so I agree with Scott, we shouldn't add grunt-cli as a dependency.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
mikesherov
Dec 24, 2012
Member
@jzaefferer, sure, I read the comment. I was saying that Jenkins would have a problem, not me. Are you saying that installing grunt-cli
on the Jenkins box will take care of both Grunt 0.4 and Grunt 0.3? That's good to know.
However, what about the case for npm test
? Is it not good to install grunt-cli
so that npm test
will work out of the box? Not that I feel strongly about it, but I'm curious what your opinion is.
mikesherov
Dec 24, 2012
Member
@jzaefferer, sure, I read the comment. I was saying that Jenkins would have a problem, not me. Are you saying that installing grunt-cli
on the Jenkins box will take care of both Grunt 0.4 and Grunt 0.3? That's good to know.
However, what about the case for npm test
? Is it not good to install grunt-cli
so that npm test
will work out of the box? Not that I feel strongly about it, but I'm curious what your opinion is.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
jzaefferer
Dec 28, 2012
Owner
Yes, grunt-cli
takes care of both versions. As for npm test
, I don't care about that. I consider the grunt
binary a system dependency, just as we expect node
and npm
to exist.
jzaefferer
Dec 28, 2012
Owner
Yes, grunt-cli
takes care of both versions. As for npm test
, I don't care about that. I consider the grunt
binary a system dependency, just as we expect node
and npm
to exist.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
mikesherov
Dec 23, 2012
Member
One other note, while you're in build/release.js
, you might as well change path.existsSync
to fs
to get rid of that node warning when you run grunt md5
.
One other note, while you're in |
mikesherov
reviewed
Dec 23, 2012
- return "<strip_all_banners:" + file + ">"; | ||
- }); | ||
-} | ||
- | ||
function stripDirectory( file ) { | ||
// TODO: we're receiving the directive, so we need to strip the trailing > |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
mikesherov
Dec 23, 2012
Member
Thanks for doing this @jzaefferer ! Also, there was 1 thing that @Krinkle had to modify in Jenkins after we pushed core's upgrade to 0.4. I forgot what it was, but obviously coordinate that with him.
Thanks for doing this @jzaefferer ! Also, there was 1 thing that @Krinkle had to modify in Jenkins after we pushed core's upgrade to 0.4. I forgot what it was, but obviously coordinate that with him. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
mikesherov
Dec 23, 2012
Member
Last thing to note was, when you add grunt-cli
to package.json
, you now have a viable npm test
command to run, we should consider adding that like we did in core's package.json
.
Last thing to note was, when you add |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
Krinkle
Dec 24, 2012
Member
What I changed in Jenkins is to use ./node_modules/.bin/grunt <cmd>
in the Shell Exec build steps of the Jenkins job(s). Since otherwise it will use the global grunt, which is (and should stay for now at) grunt 0.3.x. We want to use the local version anyway for the reason @mikesherov mentioned.
What I changed in Jenkins is to use |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
Krinkle
Dec 24, 2012
Member
Actually:
[..] executables will be added to thePATH
for executing the scripts.
[..] exported into thenode_modules/.bin
directory on npm install.
Testing with scripts.install = "echo $PATH;"
I see that the following is added to the PATH
for npm scripts:
:/usr/local/bin
:/usr/local/lib/node_modules/npm/bin/node-gyp-bin
:/path/to/your-project/node_modules/.bin
:$PATH
So inside the scripts.test
property we don't need to prefix ./node_modules/.bin
, unless a global version may exist that can conflict (since that has preference apparently).
Actually: [..] executables will be added to the Testing with
So inside the |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
scottgonzalez
Dec 24, 2012
Owner
What I changed in Jenkins is to use ./node_modules/.bin/grunt in the Shell Exec build steps of the Jenkins job(s). Since otherwise it will use the global grunt, which is (and should stay for now at) grunt 0.3.x. We want to use the local version anyway for the reason @mikesherov mentioned.
Why is that the case? Why do we not want grunt-cli in there globally right now? I really don't think we should be putting grunt-cli in our dependencies.
Why is that the case? Why do we not want grunt-cli in there globally right now? I really don't think we should be putting grunt-cli in our dependencies. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
scottgonzalez
Dec 24, 2012
Owner
One thing I've been waiting for is the name change from lint -> jshint. Now that we've got it, I'd like to add a new lint
task that lints CSS, HTML, JS.
One thing I've been waiting for is the name change from lint -> jshint. Now that we've got it, I'd like to add a new |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
scottgonzalez
Dec 24, 2012
Owner
I'll try to test this out this week. Still knee-deep in site work right now.
I'll try to test this out this week. Still knee-deep in site work right now. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
jzaefferer
Dec 24, 2012
Owner
Regarding the lint task: Let's make that the default? Or just add it as an extra task? Order should probably be js, css, html.
Regarding the lint task: Let's make that the default? Or just add it as an extra task? Order should probably be js, css, html. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
scottgonzalez
Dec 24, 2012
Owner
I'm fine with making it the default (I'd still like a named task for it though), unless someone strongly objects to removing tests from the default task.
I'm fine with making it the default (I'd still like a named task for it though), unless someone strongly objects to removing tests from the default task. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
mikesherov
Dec 24, 2012
Member
I like running the tests in the default task personally. But I'd be just as fine removing it.
I like running the tests in the default task personally. But I'd be just as fine removing it. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
mikesherov
Dec 28, 2012
Member
Can we land this and then make the minor improvements later? I'm anxious to see this working.
Can we land this and then make the minor improvements later? I'm anxious to see this working. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
scottgonzalez
Dec 28, 2012
Owner
@mikesherov I don't want to land this until I've had time to do a dry run of the release process. That should happen early next week (I'm currently knee-deep in site work).
@mikesherov I don't want to land this until I've had time to do a dry run of the release process. That should happen early next week (I'm currently knee-deep in site work). |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
no problem. I've got an itchy trigger finger. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
jzaefferer
Jan 20, 2013
Owner
Just tested and updated this with rc6 and a rebase against master, no changes required for the new rc, just one merge conflict to resolve (in the generate_themes task).
Just tested and updated this with rc6 and a rebase against master, no changes required for the new rc, just one merge conflict to resolve (in the generate_themes task). |
Krinkle
reviewed
Jan 21, 2013
@@ -18,11 +18,11 @@ var | ||
uiFiles = coreFiles.map(function( file ) { | ||
return "ui/" + file; | ||
- }).concat( grunt.file.expandFiles( "ui/*.js" ).filter(function( file ) { | ||
+ }).concat( expandFiles( "ui/*.js" ).filter(function( file ) { |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
Krinkle
Jan 21, 2013
Member
What's with this? Does grunt.file.expandFiles
no longer exist? What does it not (or no longer) do that grunt.file.expandMapping
(called from expandFiles
) does do?
Krinkle
Jan 21, 2013
Member
What's with this? Does grunt.file.expandFiles
no longer exist? What does it not (or no longer) do that grunt.file.expandMapping
(called from expandFiles
) does do?
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
scottgonzalez
Jan 22, 2013
Owner
expandFiles
is gone and expandMappings
returns a map of src -> dest, not an array.
scottgonzalez
Jan 22, 2013
Owner
expandFiles
is gone and expandMappings
returns a map of src -> dest, not an array.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
jzaefferer
Mar 2, 2013
Owner
I've updated to 0.4 final and rebase against master, its working again but also has too much cruft just to migrate to 0.4. I've file an issue against, hopefully we get some help from there: gruntjs/grunt#700
I also want to look into replacing our custom zip task with grunt-contrib-compress, same for grunt-contrib-clean and test the update for grunt-compare-size. Not sure if we should include those here and combine the testing effort, or land this PR asap and do the other updates separately.
I've updated to 0.4 final and rebase against master, its working again but also has too much cruft just to migrate to 0.4. I've file an issue against, hopefully we get some help from there: gruntjs/grunt#700 I also want to look into replacing our custom zip task with grunt-contrib-compress, same for grunt-contrib-clean and test the update for grunt-compare-size. Not sure if we should include those here and combine the testing effort, or land this PR asap and do the other updates separately. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
mikesherov
Mar 5, 2013
Member
I'm all for land ASAP.
On Sat, Mar 2, 2013 at 7:45 AM, Jörn Zaefferer notifications@github.comwrote:
I've updated to 0.4 final and rebase against master, its working again but
also has too much cruft just to migrate to 0.4. I've file an issue against,
hopefully we get some help from there: gruntjs/grunt#700gruntjs/grunt#700I also want to look into replacing our custom zip task with
grunt-contrib-compresshttps://github.com/gruntjs/grunt-contrib-compress/,
same for grunt-contrib-cleanhttps://github.com/gruntjs/grunt-contrib-cleanand test the update for
grunt-compare-sizehttps://github.com/rwldrn/grunt-compare-size/pull/16#issuecomment-14322746.
Not sure if we should include those here and combine the testing effort, or
land this PR asap and do the other updates separately.—
Reply to this email directly or view it on GitHubhttps://github.com/jquery/jquery-ui/pull/867#issuecomment-14327666
.
Mike Sherov
Lead Developer
SNAP Interactive, Inc.
Ticker: STVI.OB
I'm all for land ASAP. On Sat, Mar 2, 2013 at 7:45 AM, Jörn Zaefferer notifications@github.comwrote:
Mike Sherov |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
jzaefferer
Mar 9, 2013
Owner
@scottgonzalez are you okay with landing this before 1.10.2? I just did another round of testing and it looks good.
@scottgonzalez are you okay with landing this before 1.10.2? I just did another round of testing and it looks good. |
scottgonzalez
reviewed
Mar 11, 2013
@@ -5,6 +5,19 @@ module.exports = function( grunt ) { | ||
var path = require( "path" ), | ||
fs = require( "fs" ); | ||
+function expandFiles( files ) { |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
scottgonzalez
Mar 11, 2013
Owner
Why is this expandFiles()
different than the other? Why don't we just expose it instead of redefining it?
scottgonzalez
Mar 11, 2013
Owner
Why is this expandFiles()
different than the other? Why don't we just expose it instead of redefining it?
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
jzaefferer
Mar 11, 2013
Owner
The other one is used directly, and only points at files. This one is used via tasks and gets folder wildcards as inputs. Folders have to be filtered out.
I'm not sure what you mean with "expose it instead of redefining it".
See also gruntjs/grunt#700
jzaefferer
Mar 11, 2013
Owner
The other one is used directly, and only points at files. This one is used via tasks and gets folder wildcards as inputs. Folders have to be filtered out.
I'm not sure what you mean with "expose it instead of redefining it".
See also gruntjs/grunt#700
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
If you've tested the release script, then we can land it. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
jzaefferer
Mar 11, 2013
Owner
The release script still runs "grunt prepare" inside the DB module, but we don't need that anymore, right? I mean, grunt release_cdn
works fine as long as you install DB first, but there's no need to run its prepare
task. That's the only issue I noticed when running the release script.
The release script still runs "grunt prepare" inside the DB module, but we don't need that anymore, right? I mean, |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
scottgonzalez
Mar 11, 2013
Owner
Ok, so let's remove the invocation for the prepare
task.
Let's just ignore the expandFiles()
until later.
Ok, so let's remove the invocation for the Let's just ignore the |
jzaefferer
added some commits
Dec 21, 2012
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
jzaefferer
Mar 12, 2013
Owner
Probably our fastest build ever, mostly due to the theme generation change. 25 seconds to run the full release_cdn
task.
Probably our fastest build ever, mostly due to the theme generation change. 25 seconds to run the full |
jzaefferer commentedDec 23, 2012
Rename to Gruntfile, upgrade to newer grunt-css and grunt-html, update custom tasks. Drop qunit-junit plugin, not worth the trouble. Update release script to run grunt-prepare after npm-install.
tree diff between old and new dist:
File diff (
diff -r -w -q dist dist-0-3/
): https://gist.github.com/fb016517fa530ca75213The banner creation changed: Whitespace is now more consistent, but slightly different. The "Includes: jquery.ui.[component]" line from non-concat files is gone.
Minifiers changed: grunt-contrib-uglify uses uglify2, grunt-css uses clean-css (instead of squish), so all minified files are different as well.
I've dropped the qunit-junit plugin instead of replacing it with the 0.4 compatible grunt-qunit-junit since that doesn't really work yet and the junit reports have provided no value to us while we were using
I'm done here, but would like someone to review and test before a merge to master.