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

[gh-8] Fixes for Windows #9

Merged
merged 1 commit into from
Aug 8, 2017
Merged

Conversation

ashmind
Copy link
Contributor

@ashmind ashmind commented Aug 5, 2017

Notes:

@ds300
Copy link
Owner

ds300 commented Aug 5, 2017

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.

@ds300
Copy link
Owner

ds300 commented Aug 5, 2017

Hmm my bad. It seems to be working after all. Not sure what I was doing wrong when testing before.

@ds300
Copy link
Owner

ds300 commented Aug 5, 2017

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.

@ashmind
Copy link
Contributor Author

ashmind commented Aug 5, 2017

@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.

@ashmind
Copy link
Contributor Author

ashmind commented Aug 5, 2017

@ds300 Recreated the commit -- now checks for status and logs the stderr on failure.

@ds300
Copy link
Owner

ds300 commented Aug 7, 2017

Looks good, thanks! I'll test again tonight.

Copy link
Owner

@ds300 ds300 left a 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
export default function spawnSafeSync(command: string, args?: string[], options?: object) {
const result = spawnSync(command, args, options)
if (result.error || result.status !== 0) {
console.error(result.stderr.toString())
Copy link
Owner

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.
@ashmind
Copy link
Contributor Author

ashmind commented Aug 8, 2017

@ds300 Sure, just applied the changes.

@ashmind
Copy link
Contributor Author

ashmind commented Aug 8, 2017

@ds300 Please feel free to rename the option name if you can think of a better one.

Copy link
Owner

@ds300 ds300 left a comment

Choose a reason for hiding this comment

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

Yay, thanks again! 🎉

@ds300 ds300 merged commit 57b8aa0 into ds300:master Aug 8, 2017
@ds300
Copy link
Owner

ds300 commented Aug 8, 2017

image

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.

None yet

2 participants