ssh2
ssh2 copied to clipboard
mscdex
Detects if running in an electron asar file.
When running in an Electron asar file, spawn is unable to resolve the packed exe files.
Electron builder automatically unpacks exe files for us, so all we need to do is to switch the path to take advantage of that.
I'm not really interested in adding custom checks like this, I'd prefer a more general change that works everywhere.
@mscdex I figured that would be the case -- happy to do the work to make it more general.
A simple approach could be providing the path to pagent.exe as a option. Would you be happy with that?
In contrast - It does feel kind of weird to have a configuration setting to tell the library where an internal EXE is located, so not sure that's perfect either?
What do you think?
I agree, this isn't the right solution. The path should just be configurable. I don't think it is weird to have a configuration option to set the exe location given the variation in how people deliver static assets.
So common in Electron is to use electron-util to determine and fix the path of your EXE. I stole this implementation verbatim from there (see the file node.js)
If you're happy with this special-case approach to app.asar detection, I'd be happy to add the appropriate path checks. If you'd rather something else, please let me know how you think it would be best handled and I can do that too.
One reason I like the app.asar detection is that the library 'just works' without additional config
Having a configurable path for the exe doesn't make sense either since it's a special executable and stays in the same place.
Can someone check if replacing this:
var EXEPATH = path.resolve(__dirname, '..', 'util/pagent.exe');
with this:
var EXEPATH = require.resolve('../util/pagent.exe');
still results in the correct EXEPATH under Electron and Windows?
That has the same problem.... It's because the code is running from one place, but the exe is in another, mostly unrelated place.
Have a think about it. In the meantime I'll fix this branch to do a better path check.
@rathboma The reason I asked is because require.resolve() seems to return the correct absolute path for me with plain node, but I didn't know if it worked the same under Electron.
Having a configurable path for the exe doesn't make sense either since it's a special executable and stays in the same place.
But it doesn't - that's the whole issue. Different build systems put it in different places.
@Timmmm The code was originally using path.resolve(), not require.resolve(), so I wasn't sure if there would be a difference there for Electron.
Having a configurable path for the exe doesn't make sense either since it's a special executable and stays in the same place.
But it doesn't - that's the whole issue. Different build systems put it in different places.
So in 99.9% of cases the path resolve will work fine. Webpacking the asset will work if it is left as an 'external' dependency. Electron is unique in that it is packed into a weird file.
@mscdex I've reverted this to use the regular resolve method. I'll test tonight if this works still. Hope we can eventually get this merged, would love to get back to using the mainline version of the library
@rathboma I'm still not keen on adding kludges like this. However, I'm considering removing support for pageant entirely, which would mean none of this would matter anyway.