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

Atom (Electron?) sets bogus execPath #1

Closed
wooorm opened this issue Oct 23, 2016 · 6 comments
Closed

Atom (Electron?) sets bogus execPath #1

wooorm opened this issue Oct 23, 2016 · 6 comments

Comments

@wooorm
Copy link

wooorm commented Oct 23, 2016

Hi! 👋

Working on AtomLinter/linter-markdown#115, I found that Atom is setting an execPath to something without the normal npm stuff. Maybe Electron is the culprit. In my case: '/Applications/Atom.app/Contents/Frameworks/Atom Helper.app/Contents/MacOS/Atom Helper', when popping the last two path-parts, we get something without the npm stuff:

🚀  Atom: pwd
/Applications/Atom.app/Contents/Frameworks/Atom Helper.app/Contents
🚀  Atom: l
total 16
-rw-r--r--  1 tilde  staff  942 17 Oct 15:09 Info.plist
drwxr-xr-x  3 tilde  staff  102 17 Oct 15:10 MacOS/
-rw-r--r--  1 tilde  staff    8 17 Oct 15:09 PkgInfo
drwxr-xr-x  3 tilde  staff  102 17 Oct 15:10 _CodeSignature/
🚀  Atom: 

I’m not sure if we can work around this, or want to fix it. I’d like to hear your thoughts on this though!

Cheers!

@eush77
Copy link
Owner

eush77 commented Oct 23, 2016

Hi! 👋

Oh, hi! 👋

I’m not sure if we can work around this, or want to fix it.

Why wouldn't we want to fix it? If there's something we can do to make it usable in Electron apps, I think it's best to at least consider the fix.

I'm not very familiar with Electron myself. Do you know if there is a standard way to detect if we run inside Electron? And also, how could we get correct npm prefix for Electron?

Lastly, I think it's probably a good idea to check for such cases in the future so that npm-prefix does not just return incorrect answers in different environments. Maybe we could drop a simple test for path.basename(process.execPath) and issue some kind of a warning if it doesn't look right. What do you think?

@wooorm
Copy link
Author

wooorm commented Oct 23, 2016

Why wouldn't we want to fix it? If there's something we can do to make it usable in Electron apps, I think it's best to at least consider the fix.

You’re right. Let’s!

I'm not very familiar with Electron myself. Do you know if there is a standard way to detect if we run inside Electron?

So, my use case is load-plugin, which is used in remark’s CLI, to load for remark . -u lint the remark-lint plugin, optionally globally.

this behaviour is enabled by default when an executable is installed globally, in this case, or if in Electron, by checking process.versions.electron.

And also, how could we get correct npm prefix for Electron?

No idea. I do know that nvm (which I use) adds a location which we can use in process.env. But I don’t know of anything in there when not using nvm.

Lastly, I think it's probably a good idea to check for such cases in the future so that npm-prefix does not just return incorrect answers in different environments. Maybe we could drop a simple test for path.basename(process.execPath) and issue some kind of a warning if it doesn't look right. What do you think?

Maybe the substring Contents/MacOS? Or Atom Helper.app?

@eush77
Copy link
Owner

eush77 commented Oct 26, 2016

Lastly, I think it's probably a good idea to check for such cases in the future so that npm-prefix does not just return incorrect answers in different environments. Maybe we could drop a simple test for path.basename(process.execPath) and issue some kind of a warning if it doesn't look right. What do you think?

Maybe the substring Contents/MacOS? Or Atom Helper.app?

What I had in mind was the last component of the path. It seems that Node.js executable is usually named node or node.exe. Would it be feasible to just check for those (and also for iojs/iojs.exe)? The risk is that this check could occasionally lead to false negatives (due to e.g. vendor suffixes). Or alternatively, we could detect just Electron. The downside to that is that we might miss something else (nw.js?).

And also, how could we get correct npm prefix for Electron?

No idea.

Is npm bundled with Electron's Node? Is it possible to install new modules with it globally? If yes, there must be a separate npm executable that allows to install new modules for Electron apps. What does npm config get prefix say then, if executed on the command line? (I'm speculating, need to dig deeper into Electron.)

I do know that nvm (which I use) adds a location which we can use in process.env. But I don’t know of anything in there when not using nvm.

I'm not sure about this. Part of the job of this module is to find npm prefix for Node.js instance it is running in, not just any Node.js on the system. Wouldn't you get a prefix for Node.js managed by nvm in this case, and not for one that is comes with Electron?

@wooorm
Copy link
Author

wooorm commented Feb 20, 2017

@eush77 I forgot about this issue, sorry!

I'm not sure about this. Part of the job of this module is to find npm prefix for Node.js instance it is running in, not just any Node.js on the system. Wouldn't you get a prefix for Node.js managed by nvm in this case, and not for one that is comes with Electron?

I don’t think we can fix this issue in this module. Atom explicitly sets one, so it would be weird if this module (being made for finding that local one) would return another.

Maybe we should create a new package, system-npm-prefix or something, that returns the npm prefix by the user?

@eush77
Copy link
Owner

eush77 commented Feb 20, 2017

I'm not sure about this. Part of the job of this module is to find npm prefix for Node.js instance it is running in, not just any Node.js on the system. Wouldn't you get a prefix for Node.js managed by nvm in this case, and not for one that is comes with Electron?

I don’t think we can fix this issue in this module. Atom explicitly sets one, so it would be weird if this module (being made for finding that local one) would return another.

Yeah, I think this issue is not this module's fault, nor Electron's fault. The behavior is kind of expected actually. I mean, this module assumes that execPath points to a “real” Node.js executable, but Electron apps carry a minimalist Node of their own, with their own global node_modules path, which some applications may be interested in.

Maybe we should create a new package, system-npm-prefix or something, that returns the npm prefix by the user?

The question then is, how do we define what's its job? Users may have different Node instances installed in different locations (I surely have). A particular instance can be controlled by nvm or n defaults as well as global shell configs, /etc/environment, per-user shell configs, per-directory configs, user-invocable config managers and a thousand of different things. If there is an .npmrc in the home directory, then sure, we know the path that would be used by all of these installations, but if not — then what? I don't think there is a clean approach to this.

For AtomLinter/linter-markdown#115, the problem is then to find a piece of an environment info that would point to the location of Remark plugins. We could use $PATH for example, resolving the location of a node executable, or even all the locations. That would work for some cases, but it could be dangerous because the versions of Node in $PATH and bound with Electron could differ (say, 7.0.0 and 4.0.0) which can be a source of nasty syntax or even semantic errors. I find the cleanest solution would be to find a way to install Remark plugins at the prefix that is bound with Electron (/Applications/Atom.app/Contents/Frameworks/Atom Helper.app/Contents/lib/node_modules, as you shared in AtomLinter/linter-markdown#115 (comment)), but I don't know if that's feasible. Alternatively, users can set up the prefix in .npmrc and that would work.

So, in a nutshell, I think the problem should be formulated in a more precise terms before we can take a shot at it.

wooorm added a commit to wooorm/load-plugin that referenced this issue Jul 8, 2017
Previously, Electron was supported for global modules, but only
if a `prefix` is set (e.g., in `.npmrc`).  If you’re using nvm,
there’s no need to set-up a `prefix`, and luckily nvm exposes
environment things to let Electron pick up on those modules.

Closes eush77/npm-prefix#1.
Closes AtomLinter/linter-markdown#115.
Closes remarkjs/remark-lint#147.
@wooorm
Copy link
Author

wooorm commented Jul 8, 2017

Took an incredible while, but I found the fix, or, mostly:

  • If someone sets up a prefix themselves, npm-prefix picks it up, even in Electron
  • If someone uses nvm, load-plugin now picks it up
  • And otherwise I’m not sure how to handle this. I’ll just add docs for people to set-up either the prefix or use nvm.

Cheers! 👋

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

No branches or pull requests

2 participants