-
Notifications
You must be signed in to change notification settings - Fork 71
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
fix(msvs): fix paths again in action command arguments #121
Conversation
This is a revert of nodejs#84 with a change to the _FixPath function allowing to change the separator used. Fixes: nodejs#120 Fixes: nodejs/node-gyp#2485
@lkallas can you confirm this fixes your issue? |
@targos this also fixes: nodejs/node-gyp#2399 |
I think this is what's causing non-path arguments to get "../" prepended in https://github.com/signalapp/libsignal-client/runs/3657024122 (search for "../x64", see also signalapp/libsignal#354). Does that sound like something this commit could have caused? This binding.gyp worked with node-gyp 7.1.2. |
That's surprising. This PR essentially restored the behavior to what was in node-gyp 7. |
@jrose-signal looking at another PR run in the libsignal-client repo, it looks like the command was already invoked the wrong way before, but that it was working with backslashes and is now broken because of the forward slashes? https://github.com/signalapp/libsignal-client/pull/346/checks?check_run_id=3386910497
I think this can be fixed on your side. Arguments that start with |
That's a good workaround, and I'm kicking myself for not thinking of that, but something has still changed since node-gyp 7. (The failure with forward slashes was an attempted run with node-gyp 8.1.0, just to see what would happen.) |
Yes. What changed is that now command arguments are "fixed" using |
Oh, you're right. 🤦 I misread what the build is currently doing and now I'm curious about how it's working at all. Thank you. I'll use the |
That said, I'll note it's still inconsistent with the behavior on non-Windows systems. |
I agree, but I tried to remove this behavior and it broke projects that relied on it :( |
This is a revert of #84
with a change to the _FixPath function allowing to change the
separator used.
Fixes: #120
Fixes: nodejs/node-gyp#2485