Closed melanieseltzer closed 5 years ago
Merging #341 into master will increase coverage by
0.02%
. The diff coverage is0%
.
@@ 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
Impacted Files | Coverage Î | |
---|---|---|
src/electron.js | 46.31% <ø> (-1.11%) |
:arrow_down: |
src/components/Initialization/Initialization.js | 27.02% <0%> (ø) |
:arrow_up: |
src/services/platform.service.js | 50% <0%> (+5.17%) |
:arrow_up: |
it wasn't working because there was no .bashrc or .bash_profile in my setup.
Hm đ¤ Do you think it's a common case to not have a .[shell]rc
or even a .bash_profile
? I'm wondering if we should create a .bash_profile
if an rc file cannot be found, and output export PATH=$PATH
. Or do you think it's not good to force the .bash_profile
and we should provide a good error message/let the user decide what to use?
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 .bash_profile
then it was finding the Node path for you right? I think if you're going to test nvm then you will need to either remove your system Node or comment this out:
if (nodePath) {
// Node found
return resolve();
}
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 &&
(;
might be more portable) and reads from ~/.config/fish/config.fish
.
@j-f1 Ah, good point. I'll do a check for it, do you know if echo $SHELL
will produce a result like /bin/fish? I guess I can install it and run it as my shell temporarily for testing (or do you know an easier way?).
It looks like && is getting added in 3.0, do you think we have to modify the command?
I think using the ;
is a good idea, especially since our target audience may not have the latest versions of everything.
You should definitely try fish :)
So I've run into an issue with $SHELL, it seems that Fish doesn't set the SHELL env variable so echo $SHELL
is returning Zsh for me (despite installing and setting Fish as default).
This comment has a command that is more accurate (it successfully returns the correct shell for me):
dscl . -read /Users/${username} UserShell
@j-f1 @AWolf81 What do you think?
@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 shell-path
.
@AWolf81 What's the difference between that one and fix-path recommended in #317? fix-path seems to use shell-path under the hood but can't really tell when to use one or the other.
@melanieseltzer oh, you're right fix-path
is using shell-path
. I thought that it's a different package.
Maybe we could check if fix-path
is called at the right position and we're not accidentially overriding the fix from fix-path
with something else.
@melanieseltzer you should be able to just use fix-path
when the Electron app launches, it uses the common platform binary env
to log out all environment variables on the host and then grab the $PATH
from there, or use a sensible set of defaults ('./node_modules/.bin', '/.nodebrew/current/bin','/usr/local/bin').
fix-path
is just a small abstraction on top of shell-path
that calls the sync export from shell-path
and then appends the result to process.env.PATH
. Here's the entire source:
'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 shell-env
directly and set our own list of default binary locations if we expect them to differ from the list above.
Ah you're right, I totally missed that. Thanks for the clarification.
So with this build, everything works alright on Mac in production? Since fix-path
relies on env
under the hood, it's possible that env
also doesn't load the correct shell config files in a production environment for whatever, in which case this PR is the correct solution.
I'll try and test sometime tomorrow on Mac and see how much I can replicate.
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 fix-path
is not working in the production distribution only, I don't know too much about it so I could only play around a bit. No combination of fix-path, shell-path, or shell-env was working for me (I played around with adding it to this block). I'm stumped! Appreciate the extra eyes on this :)
@melanieseltzer Maybe it's related to our webpack env handling in config/env.js. I think we could check if there is something happening with the filtering or we could add the path fix for production there. I haven't tried anything here as I think this path issue is just happening on Mac with production build.
Alright, after lots of experimenting I think I've found a solution using shell-path
. I think fix-path
is not working in our case due to Mac apps not getting access to env variables if launched from GUI. When launched from terminal everything is fine, so that's why things worked in dev here (since running Guppy in dev launches it from terminal via yarn start
).
@superhawk610 let me know what you think and could you test it as well on Mac? It's building with yarn dist
for me and I'm no longer receiving the Node missing error (I'm using NVM). All Node related things seem to be working fine (create new project, dev server, etc).
Hmm, I'm getting Node not found in both dev and prod builds. My Node installation is at ~/n/bin
and $PATH
configuration is done in my .zshrc
with this script:
export N_PREFIX="$HOME/n"; [[ :$PATH: == *":$N_PREFIX/bin:"* ]] || PATH+=":$N_PREFIX /bin" # Added by n-install (see http://git.io/n-install-repo).
Here's the path listed in the alert:
// DEV
/.cargo/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/aaronross/code/guppy/node_modules/.bin:/Users/aaronross/code/node_modules/.bin:/Users/aaronross/node_modules/.bin:/Users/node_modules/.bin:/node_modules/.bin:/Users/aaronross/code/guppy/node_modules/electron/dist/Electron.app/Contents/Frameworks/Electron Helper.app/Contents/MacOS
// PROD
/.cargo/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/node_modules/.bin:/Users/aaronross/code/guppy/dist/mac/Guppy.app/Contents/Frameworks/Guppy Helper.app/Contents/MacOS
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 echo $PATH
. Actually the more I'm reading, the more I think this might be the issue for you:
https://github.com/sindresorhus/fix-path/issues/3#issuecomment-120547010:
I have a feeling this might be that you have those $PATH additions exported in .zshrc and not .zshenv. Is this the case? The .zshrc file is only loaded in interactive terminals, which exec isn't, and you should not be used for exporting environment variables, like $PATH.
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):
export N_PREFIX="$HOME/n"; [[ :$PATH: == *":$N_PREFIX/bin:"* ]] || PATH+=":$N_PREFIX/bin/TEST"
# Added by n-install (see http://git.io/n-install-repo).
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 shellPath
and spawning the interactive shell is overkill, it seems to return a lot in dev compared to prod, and I'm really confused why that is.
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. shellPath
uses shell-env
under the hood, which in turn just wraps the output from env
in a non-interactive shell, so I wonder if just using echo $PATH
in an interactive shell like you've proposed wouldn't be a sufficient solution on its own?
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: /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--1554580655799-0.4203236455267696:/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/TESTINGZSHRC
prod (system node): /Users/melleh/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/melleh/n/bin/TESTINGZSHRC
prod (nvm node): /Users/melleh/.nvm/versions/node/v11.4.0/bin:/Users/melleh/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/melleh/n/bin/TESTINGZSHRC
Yeah I'm fairly certain the extra junk in dev is coming from Electron, I get some warning about the Node version in that /var/folders/m9/...
not matching every time I launch with yarn start
.
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.