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

Detects if running in an electron asar file. #894

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rathboma
Copy link

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.

@mscdex
Copy link
Owner

mscdex commented May 21, 2020

I'm not really interested in adding custom checks like this, I'd prefer a more general change that works everywhere.

@rathboma
Copy link
Author

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

@rathboma
Copy link
Author

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?

@Timmmm
Copy link

Timmmm commented May 21, 2020

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.

lib/agent.js Outdated
var ERROR = {};
var EXEPATH = path.resolve(__dirname, '..', 'util/pagent.exe');
var EXEPATH = RAWPATH.includes('app.asar') ? RAWPATH.replace('app.asar', 'app.asar.unpacked') : RAWPATH;
Copy link

@mofux mofux May 21, 2020

Choose a reason for hiding this comment

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

This check looks incorrect. What if the path already is inside app.asar.unpacked? It will still hit this code path and result in app.asar.unpacked.unpacked

Copy link
Author

Choose a reason for hiding this comment

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

I mostly added this code as a proof of concept to start the discussion. I agree it needs a little tuning, but figured you might want a more general solution.

@rathboma
Copy link
Author

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

@mscdex
Copy link
Owner

mscdex commented May 21, 2020

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?

@rathboma
Copy link
Author

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.

@mscdex
Copy link
Owner

mscdex commented May 22, 2020

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

@Timmmm
Copy link

Timmmm commented May 22, 2020

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.

@mscdex
Copy link
Owner

mscdex commented May 22, 2020

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

@rathboma
Copy link
Author

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.

@rathboma
Copy link
Author

rathboma commented Mar 8, 2022

@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

@mscdex
Copy link
Owner

mscdex commented Mar 9, 2022

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

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.

4 participants