-
Notifications
You must be signed in to change notification settings - Fork 154
Conversation
Codecov Report
@@ Coverage Diff @@
## master #341 +/- ##
==========================================
+ Coverage 58.14% 58.17% +0.02%
==========================================
Files 158 158
Lines 3314 3309 -5
Branches 459 458 -1
==========================================
- Hits 1927 1925 -2
+ Misses 1184 1182 -2
+ Partials 203 202 -1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I tested your code in my Mac VM (Mojave 10.14) and noticed that it wasn't working because there was no .bashrc
or .bash_profile
in my setup.
After adding export PATH=$PATH
to a new .bash_profile
file it's working as expected.
I'm wondering where does the terminal take the path from or is it because there are just the defaults in path /usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin
It was working with-out the .bash_profile
file if I started Guppy from a terminal with open Guppy.app
- not sure what's different if opened like this. But clicking the Guppy icon I got Node not found.
My node version is installed with the standard node setup - I haven't tested nvm yet.
Calling which node
in a terminal outputs /usr/local/bin/node
so node is available on path.
Hm 🤔 Do you think it's a common case to not have a To the other stuff - tbh the whole path stuff confuses me so I'll have to do some reading. But as long as there's a
Because I have both system + nvm default running and it was finding the path early and returning due to the above, so I had to comment it out to test the rest of the logic in the file. |
This wouldn’t work for fish, which doesn’t support |
@j-f1 Ah, good point. I'll do a check for it, do you know if It looks like && is getting added in 3.0, do you think we have to modify the command? |
I think using the |
So I've run into an issue with $SHELL, it seems that Fish doesn't set the SHELL env variable so This comment has a command that is more accurate (it successfully returns the correct shell for me):
|
@melanieseltzer This is really getting complicated. What do you think should we give shell-path package a try? From the description it is exactly doing what we're trying here - fixing the path for Mac. Not sure if every shell (e.g. fish) is properly handled but if not we could add it to |
@melanieseltzer oh, you're right Maybe we could check if |
@melanieseltzer you should be able to just use
'use strict';
const shellPath = require('shell-path');
module.exports = () => {
if (process.platform !== 'darwin') {
return;
}
process.env.PATH = shellPath.sync() || [
'./node_modules/.bin',
'/.nodebrew/current/bin',
'/usr/local/bin',
process.env.PATH
].join(':');
}; We could just use |
Ah you're right, I totally missed that. Thanks for the clarification. So with this build, everything works alright on Mac in production? Since I'll try and test sometime tomorrow on Mac and see how much I can replicate. |
4666e5e
to
f2ecca3
Compare
It's building from this branch (other than the issue from #370 but that's a separate thing) and I don't see the missing Node error that appears for me in production 0.3.0 otherwise. But I think I agree with @AWolf81 that this PR seems a little convoluted - he even encountered an edge case (documented above) where he had no .bashrc or .bash_profile so my sourcing code failed until he added one. Maybe we should investigate why |
@melanieseltzer Maybe it's related to our webpack env handling in config/env.js. |
Alright, after lots of experimenting I think I've found a solution using @superhawk610 let me know what you think and could you test it as well on Mac? It's building with |
@@ -37,7 +37,12 @@ class Initialization extends PureComponent<Props, State> { | |||
initializePath() | |||
.then(getNodeJsVersion) | |||
.then(nodeVersion => { | |||
this.setState({ wasSuccessfullyInitialized: !!nodeVersion }); | |||
// the following dialog is just for debugging | |||
dialog.showErrorBox('Path', window.process.env.PATH); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove this when it's ready to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this and this is good to go :)
if (process.env.NODE_ENV !== 'development') { | ||
icon256Path = '../build/256x256.png'; | ||
fixPath(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to use fixPath in various locations in this file (outside of the if statement) to see if it was simply not working due to not having access to NODE_ENV
, but no combination was working, so I've just removed it.
src/services/platform.service.js
Outdated
} | ||
); | ||
|
||
// This is basically `fix-path` but we are adding it to window instead | ||
window.process.env.PATH = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to use fixPath here but it was not working at all. The only difference we add it to the window object here, whereas fixPath adds it to process.env.PATH
.
Adding to window seems to do the trick here!
Hmm, I'm getting Node not found in both dev and prod builds. My Node installation is at
Here's the path listed in the alert:
It's interesting that the paths are absolute in dev and relative in prod, perhaps that's something that also needs to be investigated? |
Huh. Darn it! Well, there was actually one other thing that was also working for me, but I wasn't sure what to think of it. It's spawning an interactive shell and running sindresorhus/fix-path#3 (comment):
I did a test and added your n-install stuff into my own .zshrc, and used the above method like so (note the TEST verbiage):
Modified the code to be: const fixedPath = shellPath.sync();
const fixedPathFromInteractiveShell = childProcess
.execFileSync(window.process.env.SHELL, ['-i', '-c', 'echo $PATH'])
.toString()
.trim();
window.process.env.PATH = fixedPath + fixedPathFromInteractiveShell;
resolve(); I successfully get the n-install stuff from my .zshrc into my path in dev and prod: dev: /usr/local/git/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/melleh/Sites/guppy/node_modules/.bin:/Users/melleh/Sites/node_modules/.bin:/Users/melleh/node_modules/.bin:/Users/node_modules/.bin:/node_modules/.bin:/Users/melleh/Sites/guppy/node_modules/electron/dist/Electron.app/Contents/Frameworks/Electron Helper.app/Contents/MacOS:/usr/local/go/bin/Users/melleh/bin:/usr/local/bin:/usr/local/lib/node_modules/npm/node_modules/npm-lifecycle/node-gyp-bin:/Users/melleh/Sites/guppy/node_modules/.bin:/var/folders/m9/rh39vf_j4s3gr52kcqtfcc5h0000gn/T/yarn--1554486651248-0.5182929671326244:/Users/melleh/Sites/guppy/node_modules/.bin:/Users/melleh/.config/yarn/link/node_modules/.bin:/Users/melleh/Sites/guppy/node_modules/.bin:/Users/melleh/.config/yarn/link/node_modules/.bin:/usr/local/libexec/lib/node_modules/npm/bin/node-gyp-bin:/usr/local/lib/node_modules/npm/bin/node-gyp-bin:/usr/local/bin/node_modules/npm/bin/node-gyp-bin:/Users/melleh/bin:/usr/local/bin:/usr/local/git/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/melleh/n/bin/TEST prod: /usr/local/git/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/node_modules/.bin:/Users/melleh/Sites/guppy/dist/mac/Guppy.app/Contents/Frameworks/Guppy Helper.app/Contents/MacOS:/usr/local/go/bin/Users/melleh/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/melleh/n/bin/TEST I'm not sure if using both But nevertheless, does the above code work for you? I didn't want to commit it in case it's another dead end. |
Nice, yeah this works for me. |
Ah! I see what it's doing now, took a bit to wrap my head around. Yes, I think it should be sufficient on its own since it catches everything, including stuff exported from dotfiles. Just not sure why there's so much stuff getting inserted into PATH in the dev environment, is that just some Electron dev specific stuff? dev: prod (system node): prod (nvm node): |
Yeah I'm fairly certain the extra junk in dev is coming from Electron, I get some warning about the Node version in that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, glad we'll finally be able to put this behind us!
@@ -37,7 +37,12 @@ class Initialization extends PureComponent<Props, State> { | |||
initializePath() | |||
.then(getNodeJsVersion) | |||
.then(nodeVersion => { | |||
this.setState({ wasSuccessfullyInitialized: !!nodeVersion }); | |||
// the following dialog is just for debugging | |||
dialog.showErrorBox('Path', window.process.env.PATH); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this and this is good to go :)
Related Issue:
#317
Summary:
There's been some issues with path and @AWolf81 had some suggestions in Gitter, so I've been playing around with it.