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

fix(msvs): fix paths again in action command arguments #121

Merged
merged 3 commits into from Aug 22, 2021

Conversation

targos
Copy link
Member

@targos targos commented Aug 22, 2021

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

@targos
Copy link
Member Author

targos commented Aug 22, 2021

@lkallas can you confirm this fixes your issue?

@lkallas
Copy link

lkallas commented Aug 22, 2021

@lkallas can you confirm this fixes your issue?

@targos yes, with this change paths are correct and build succeeds.

@lkallas
Copy link

lkallas commented Aug 22, 2021

@targos this also fixes: nodejs/node-gyp#2399
Tried with given change and build succeeds in windows.

@jrose-signal
Copy link

jrose-signal commented Sep 20, 2021

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.

@targos
Copy link
Member Author

targos commented Sep 21, 2021

That's surprising. This PR essentially restored the behavior to what was in node-gyp 7.

@targos
Copy link
Member Author

targos commented Sep 21, 2021

@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

      Finished release [optimized] target(s) in 3m 52s
  Invoked with '..\node\build_node_bridge.py --out-dir D:\a\libsignal-client\libsignal-client\build\Release" --os-name ..\win32 --configuration Release --cargo-build-dir Release\obj\libsignal_client_win32_x64.node\rust --cargo-target ..\x86_64-pc-windows-msvc --node-arch ..\x64'

I think this can be fixed on your side. Arguments that start with / or - are not modified. Can you try to change lines like '--node-arch', '<(target_arch)' to '--node-arch=<(target_arch)' ?

@jrose-signal
Copy link

jrose-signal commented Sep 21, 2021

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

@targos
Copy link
Member Author

targos commented Sep 21, 2021

but something has still changed since node-gyp 7.

Yes. What changed is that now command arguments are "fixed" using / as the path separator instead of \.
Before: value would become ..\value
Now: value becomes ../value

@jrose-signal
Copy link

jrose-signal commented Sep 21, 2021

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 = form you suggested.

@jrose-signal
Copy link

That said, I'll note it's still inconsistent with the behavior on non-Windows systems.

@targos
Copy link
Member Author

targos commented Sep 21, 2021

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 :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants