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

tmpfile writer to solve rsync problem on windows - issue #42 #79

Merged
merged 1 commit into from
Mar 28, 2015
Merged

tmpfile writer to solve rsync problem on windows - issue #42 #79

merged 1 commit into from
Mar 28, 2015

Conversation

gfilardo
Copy link
Contributor

Hi pstadler,
I forked your project to address the rsync problem on windows reported on #42.
To achieve the cross-platform compatibility, I had to change the path of the file used by rsync listing all the files to transfer, since rsync does not work with path containing the drive letter followed by colon.
On unix-like systems the behavior is completely unaltered. On the other hand, on windows, the cwd is used to contain the temporary file.
I wrote the new temp-file-write module, which is similar to temp-write, but allows you to specify the destination directory (the os temp dir is used by default). I used it in place of the original one in the shell.js file, together with the os module to recognize the platform.
The system works fine, but I would like to highlight two potential problems:

  • on windows the process using flightplan has to have writing capabilities on the current working directory
  • if a problem should arise during the transfer, the tmp file would not be deleted

That being said, I would like to state that in my work I have tested the system and I haven't encountered such problems, using both deltacopy and cwRsync. There are three major requirements on windows, though: git has to be in the path, as well as python 2, and fibers have to be supported. An installation of Visual Studio 2013 provides all the required dlls to support them.

@gfilardo gfilardo changed the title tmpfile writer to solve rsync problem on windows tmpfile writer to solve rsync problem on windows - issue #42 Mar 17, 2015
@pstadler
Copy link
Owner

Hi Giovanni

I'd like to ask you to rename lib/temp-file/temp-file-write.js to lib/utils/index.js instead and to move the OS check inside the writer function, so that the interface is clean:

var writeTempFile = require('./utils').writeTempFile
var tmpFile = writeTempFile(files);

Besides that:

  • I don't see why we need graceful-fs?
  • Why is python required?

Please let me know if you're tired of doing this, so I will do the changes followed by a single commit, which I want to avoid, because you did the work.

Thanks!

@gfilardo
Copy link
Contributor Author

Hi Patrick,
I will refactor the temp file writer as you suggested tomorrow.
In answer to your two questions:

  • given the very specific nature of the refactored temp file writer, the graceful-fs module should not be needed anymore, I think the dependency could be safely removed in this case, but I would prefer to do some tests on my windows computers at work on Monday
  • I think that pyhton is required by node-gyp which is in turn required by node-fibers

Expect the changes done and tested by Monday.
Thank you

@gfilardo
Copy link
Contributor Author

Hi Patrick,
I have refactored the temp file writer as you asked and successfully tested it on mac and on windows.

var isWin = /^win/i.test(os.platform());
var fullpath = isWin ? tempFilePath('.') : tempFilePath();

mkdirp.sync(path.dirname(fullpath));
Copy link
Owner

Choose a reason for hiding this comment

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

I think it's not necessary to create folders at either . or whatever require('os').tmpdir() returns, as they already exist.

@pstadler
Copy link
Owner

Thanks man, please see my comment above.

@@ -0,0 +1,20 @@
var path = require('path')
, tmpdir = require('os').tmpdir()
Copy link
Owner

Choose a reason for hiding this comment

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

please move this below line 5, and use tmpdir = os.tmpdir()

@gfilardo
Copy link
Contributor Author

Done!

@pstadler
Copy link
Owner

Thanks man, this is great.

One last task, could you squash this into a single commit?

Cheers
Patrick

@pstadler
Copy link
Owner

One more thing, sorry for being so picky, but having less dependencies is a good thing.
uuid could be replaced with something like this:

require('crypto').randomBytes(16).toString('hex')

@gfilardo
Copy link
Contributor Author

Sure, I will push the changes tomorrow morning.

@pstadler
Copy link
Owner

Looks good, can you rebase / squash this into a single commit?

@gfilardo
Copy link
Contributor Author

Done!

@pstadler
Copy link
Owner

Thanks man, will try to land a new version this weekend!

pstadler added a commit that referenced this pull request Mar 28, 2015
tmpfile writer to solve rsync problem on windows - issue #42
@pstadler pstadler merged commit 44934ef into pstadler:master Mar 28, 2015
@pstadler
Copy link
Owner

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.

2 participants