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

Clean function is broken on Windows #78

Open
jry2 opened this issue Feb 20, 2017 · 4 comments
Open

Clean function is broken on Windows #78

jry2 opened this issue Feb 20, 2017 · 4 comments

Comments

@jry2
Copy link

jry2 commented Feb 20, 2017

I'm testing vinyl-ftp v0.6.0 on Windows 7 and node v7.2.0.

Following code doesn't work at all:

gulp.task('ftp-clean', function () {
    var conn = ftp.create(ftpOptions);
    return conn.clean('./**', 'public', {base:'.'});
});

FTP connection works fine, directories are listed. The first problem is clean function is trying to delete files that should stay (exists in both local and remote directory). Second problem is delete command doesn't work:

DEL   \404.html
Error: Delete operation failed
@jry2
Copy link
Author

jry2 commented Feb 20, 2017

Delete operation works fine from Linux Mint 18.1, Node v7.5.0. There is slash displayed instead of Windows backslash, could be related to problem on Windows:

DEL /404.html

Regarding first mentioned problem it works with

gulp.task('ftp-clean', function () {
    var conn = ftp.create(ftpOptions);
    return conn.clean('/**', 'public', {base:'/'});
});

@jry2 jry2 changed the title Clean function is broken Clean function is broken on Windows Feb 20, 2017
@jry2
Copy link
Author

jry2 commented Feb 20, 2017

With quick and dirty patch in vinyl-ftp/lib/rmdir.js and delete.js I can solve the (second) problem:

path = path.replace( /\\/g,"/" ); // HACK: backslash to slash conversion
self.log( 'RMDIR', path );
ftp.rmdir( path, true, final );

@ajoah
Copy link

ajoah commented Mar 10, 2017

I confirm the problem on Windows 10 : slash from globs parameter are converted to backslash. The @jry2 patch solve the issue for the moment.

@taxibox-leigh
Copy link

Yep, I have encountered the same thing. I've been debugging this for over an hour now and it looks like the issue might be in vinyl.

  1. I can see that within vinyl-ftp the join() function in lib/helpers.js is converting backslashes into foreslashes. This is cool.
  2. This is called by vinylFiles() in lib/ftp.js which then passes that POSIX style path into Vinyl()
  3. Inspecting the value of vinyl.path immediately after it's created, the path now has backslashes again
  4. (getting pretty wild now because I'm a real node.js n00b) looking in the vinyl package dependency index.js I can see that the 'get' method for the 'path' property calls a function called normalize()
  5. Checking out vinyl/lib/normalize.js I can see that this function is a wrapper to Path.normalize()
  6. On Windows, the nodejs Path object calls the Win32 versions of all the functions and is inserting backslashes instead of foreslashes. If I change path.normalize(str) to path.posix.normlize(str) then I get beautiful foreslashes as nature intended.

Now is this really a bug on Vinyl's end? I'm guessing yes? Personally I would expect Vinyl to be agnostic of the OS it's running on. I'd want to be able to tell it which style of path I want to use as a config option.

For now I don't think there's a "clean" way to fix this bug on Windows within vinyl-ftp itself. I'm currently using a modified lib/clean.js inside function check() changing:

if ( method ) {
  self[ method ](remote.path, cb );
 } else {
  cb();
}

to

if ( method ) {
  self[ method ]( self.normalize(remote.path), cb );
 } else {
  cb();
}

Which is just about the tidiest way I can think of to correct this problem.

Last bit of info I have on this:

  • It looks like the upstream issue in vinyl is being looked at here
  • From that discussion it might be worth rolling the dependency back to vinyl version 1.x unless it breaks stuff or there's a compelling reason not to,
  • I can't think of a compelling reason not to fix it in the meantime as the 'for now' fix is pretty tidy/minimal.

I hope this info helps, please let me know if there's anything I can do to get a fix in place (I use vinyl-ftp for deployment from Windows and really want to see this fixed).

Thanks!

@rzer rzer mentioned this issue Jan 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants