Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fixes bug in base_path resolution #20

Open
wants to merge 2 commits into from

2 participants

Ethan J. Brown Aaron D. Valade
Ethan J. Brown

Due to how paths are shipped around,
base_path would not correctly handle files in the root path, nesting them.

For instance, a base_path of 'app/js' has been passed,
which becomes 'app/js/' in the task.

Notice in the output that all files are properly re-based,
except for those directly inside 'app/js/' (namely main.js, app.js):

Running "coffee:app" (coffee) task
Verifying property coffee.app exists in config...OK
>> Note that grunt.utils has been renamed to grunt.util and is now deprecated.
>> Please ensure that your custom tasks are up-to-date. (grunt 0.4.0+)
Reading app/js/app.coffee...OK
Writing generated\js\app\js\app.js...OK
Reading app/js/controllers/auth.coffee...OK
Writing generated\js\controllers\auth.js...OK
Reading app/js/directives/directives.coffee...OK
Writing generated\js\directives\directives.js...OK
Reading app/js/filters/filters.coffee...OK
Writing generated\js\filters\filters.js...OK
Reading app/js/main.coffee...OK
Writing generated\js\app\js\main.js...OK
Reading app/js/services/auth.coffee...OK
Writing generated\js\services\auth.js...OK

With this fixed, we get the following desired output

Running "coffee:app" (coffee) task
Verifying property coffee.app exists in config...OK
>> Note that grunt.utils has been renamed to grunt.util and is now deprecated.
>> Please ensure that your custom tasks are up-to-date. (grunt 0.4.0+)
Reading app/js/app.coffee...OK
Writing generated\js\app.js...OK
Reading app/js/controllers/auth.coffee...OK
Writing generated\js\controllers\auth.js...OK
Reading app/js/directives/directives.coffee...OK
Writing generated\js\directives\directives.js...OK
Reading app/js/filters/filters.coffee...OK
Writing generated\js\filters\filters.js...OK
Reading app/js/main.coffee...OK
Writing generated\js\main.js...OK
Reading app/js/services/auth.coffee...OK
Writing generated\js\services\auth.js...OK
Ethan J. Brown Iristyle Fixes bug in base_path resolution
Due to how paths are shipped around,
base_path would not correctly handle files in the root path, nesting them.

For instance, a base_path of 'app/js' has been passed, 
which becomes 'app/js/' in the task.

Notice in the output that all files are properly re-based,
except for those directly inside 'app/js/' (namely main.js, app.js):

Running "coffee:app" (coffee) task
Verifying property coffee.app exists in config...OK
>> Note that grunt.utils has been renamed to grunt.util and is now deprecated.
>> Please ensure that your custom tasks are up-to-date. (grunt 0.4.0+)
Reading app/js/app.coffee...OK
Writing generated\js\app\js\app.js...OK
Reading app/js/controllers/auth.coffee...OK
Writing generated\js\controllers\auth.js...OK
Reading app/js/directives/directives.coffee...OK
Writing generated\js\directives\directives.js...OK
Reading app/js/filters/filters.coffee...OK
Writing generated\js\filters\filters.js...OK
Reading app/js/main.coffee...OK
Writing generated\js\app\js\main.js...OK
Reading app/js/services/auth.coffee...OK
Writing generated\js\services\auth.js...OK
9b6655c
Aaron D. Valade
Owner

Looks good, except you should use path.sep instead of using the hard-coded / character. If you want to update your commit with that, then I will merge it.

Ethan J. Brown

You're right.. totally slipped my mind. Will update in a few.

Note however that I'm on Windows... and my path chars are '/' ;0

Ethan J. Brown

Ok, so path.sep wouldn't work all of the time, because there is nothing to ensure that base_path has a style that matches the OS.

Even though I'm on Windows, I tend to use / in config files because in my experience, it just works.

So the solution also includes a bit of normalization.. and some tweaks to make sure the resultant RegEx is safe to use - but apparently Travis doesn't like it! Looking into it...

Ethan J. Brown

Not sure what's causing the fails in the tests here... only failing on 0.6

Ethan J. Brown

Any ideas?

Aaron D. Valade
Owner

Looks like path.sep was only introduced in the NodeJS 0.7 development cycle so it's not available in Node 0.6. I've removed the call to path.sep and instead just rely on path.join to "do the right thing" in this commit 0e10ca5. If that looks good to you, I'll merge it in.

Ethan J. Brown

Sorry it took so long to get back with you here.... unfortunately that doesn't work either.

However, I do have a fix -- pretty similar to this one.
arian/wrapup@74fd505

Updating pull in just a sec.

Ethan J. Brown Iristyle Fixes bug in base_path resolution
Due to how paths are shipped around, base_path would not correctly
handle files in the root path, nesting them.

Properly normalizes paths / uses path.sep when available, so that
this will work on all operating systems, regardless of how paths
are configured in the configuration file.

For instance, a base_path of 'app/js' has been passed,
which becomes 'app/js/' in the task.

Notice in the output that all files are properly re-based,
except for those directly inside 'app/js/' (namely main.js, app.js):

Running "coffee:app" (coffee) task
Verifying property coffee.app exists in config...OK
>> Note that grunt.utils has been renamed to grunt.util and is now deprecated.
>> Please ensure that your custom tasks are up-to-date. (grunt 0.4.0+)
Reading app/js/app.coffee...OK
Writing generated\js\app\js\app.js...OK
Reading app/js/controllers/auth.coffee...OK
Writing generated\js\controllers\auth.js...OK
Reading app/js/directives/directives.coffee...OK
Writing generated\js\directives\directives.js...OK
Reading app/js/filters/filters.coffee...OK
Writing generated\js\filters\filters.js...OK
Reading app/js/main.coffee...OK
Writing generated\js\app\js\main.js...OK
Reading app/js/services/auth.coffee...OK
Writing generated\js\services\auth.js...OK
3913a7b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 10, 2012
  1. Ethan J. Brown

    Fixes bug in base_path resolution

    Iristyle authored
    Due to how paths are shipped around,
    base_path would not correctly handle files in the root path, nesting them.
    
    For instance, a base_path of 'app/js' has been passed, 
    which becomes 'app/js/' in the task.
    
    Notice in the output that all files are properly re-based,
    except for those directly inside 'app/js/' (namely main.js, app.js):
    
    Running "coffee:app" (coffee) task
    Verifying property coffee.app exists in config...OK
    >> Note that grunt.utils has been renamed to grunt.util and is now deprecated.
    >> Please ensure that your custom tasks are up-to-date. (grunt 0.4.0+)
    Reading app/js/app.coffee...OK
    Writing generated\js\app\js\app.js...OK
    Reading app/js/controllers/auth.coffee...OK
    Writing generated\js\controllers\auth.js...OK
    Reading app/js/directives/directives.coffee...OK
    Writing generated\js\directives\directives.js...OK
    Reading app/js/filters/filters.coffee...OK
    Writing generated\js\filters\filters.js...OK
    Reading app/js/main.coffee...OK
    Writing generated\js\app\js\main.js...OK
    Reading app/js/services/auth.coffee...OK
    Writing generated\js\services\auth.js...OK
Commits on Oct 5, 2012
  1. Ethan J. Brown

    Fixes bug in base_path resolution

    Iristyle authored
    Due to how paths are shipped around, base_path would not correctly
    handle files in the root path, nesting them.
    
    Properly normalizes paths / uses path.sep when available, so that
    this will work on all operating systems, regardless of how paths
    are configured in the configuration file.
    
    For instance, a base_path of 'app/js' has been passed,
    which becomes 'app/js/' in the task.
    
    Notice in the output that all files are properly re-based,
    except for those directly inside 'app/js/' (namely main.js, app.js):
    
    Running "coffee:app" (coffee) task
    Verifying property coffee.app exists in config...OK
    >> Note that grunt.utils has been renamed to grunt.util and is now deprecated.
    >> Please ensure that your custom tasks are up-to-date. (grunt 0.4.0+)
    Reading app/js/app.coffee...OK
    Writing generated\js\app\js\app.js...OK
    Reading app/js/controllers/auth.coffee...OK
    Writing generated\js\controllers\auth.js...OK
    Reading app/js/directives/directives.coffee...OK
    Writing generated\js\directives\directives.js...OK
    Reading app/js/filters/filters.coffee...OK
    Writing generated\js\filters\filters.js...OK
    Reading app/js/main.coffee...OK
    Writing generated\js\app\js\main.js...OK
    Reading app/js/services/auth.coffee...OK
    Writing generated\js\services\auth.js...OK
This page is out of date. Refresh to see the latest.
Showing with 6 additions and 2 deletions.
  1. +6 −2 tasks/coffee.js
8 tasks/coffee.js
View
@@ -8,6 +8,8 @@
module.exports = function(grunt) {
var path = require('path');
+ var isWindows = process.platform === "win32";
+ var pathsep = path.sep || isWindows ? '\\' : '/';
// Please see the grunt documentation for more information regarding task and
// helper creation: https://github.com/cowboy/grunt/blob/master/docs/toc.md
@@ -44,9 +46,11 @@ module.exports = function(grunt) {
extension = typeof extension === "undefined" ? '.js' : extension;
if( destPath && options.preserve_dirs ){
- var dirname = path.dirname(src);
+ var dirname = path.normalize(path.dirname(src)) + pathsep;
if ( options.base_path ) {
- dirname = dirname.replace(new RegExp('^'+options.base_path), '');
+ //on Windows, path.sep must be escaped in a regex
+ var safeRegex = path.normalize(options.base_path).replace(/\\/g, "\\\\");
+ dirname = dirname.replace(new RegExp('^' + safeRegex), '');
}
destPath = path.join(destPath, dirname);
} else if( !destPath ){
Something went wrong with that request. Please try again.