-
-
Notifications
You must be signed in to change notification settings - Fork 313
[gh-8] Fixes for Windows #9
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
Conversation
Thanks for the work on this. Unfortunately it breaks patching on macOS :/ the git apply commands exit with 0 status, but the patches don't get applied.
Hmm my bad. It seems to be working after all. Not sure what I was doing wrong when testing before.
The issue was that when the patch can't be applied it fails silently. I think you just need to change the way that the error condition is checked in spawnSafe. Instead of checking for result.error you need to check whether result.status > 0. Maybe both, if failures on windows are encoded in the result.error property.
@ds300 Right -- stupid mistake.
I think error is for system errors (e.g. file not found) -- you are right that I should check both.
I'll update and retest.
0a0a4fb to
9cf0b27
Compare
@ds300 Recreated the commit -- now checks for status and logs the stderr on failure.
Looks good, thanks! I'll test again tonight.
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 is good good stuff, thanks! Just a couple of tiny changes. The logging suppression thing mentioned elsewhere, and also rebase on my master branch and run yarn format to make the code nice and pretty :)
src/spawnSafe.ts
Outdated
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.
Can you add an option to suppress this logging, and make sure that it doesn't happen for any of the patch application commands? It prints errors in cases where the patch was already applied.
...roperly and replaced patch with git apply.
9cf0b27 to
3223c74
Compare
@ds300 Sure, just applied the changes.
@ds300 Please feel free to rename the option name if you can think of a better one.
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.
Yay, thanks again! 🎉
Notes:
cross-spawn, based on Windows doesn't like single quotes xxorax/node-shell-escape#9 (comment) .patchwithgit apply—patchis not installed by default on Windows, whilegitis already used by this package.