Add fish support 🐟 #266

chriskrycho · 2019-02-09T00:44:12Z

This PR builds on the work @scalvert started in #171 to add full fish support to Notion.

closes [WIP] First pass at fish shell support #171
resolves fish support #152

Testing

Check out this branch locally. If you're using hub, hub pr checkout 266. (If you're not using hub… you should do that thing. And also alias it to git to make your life wonderful.)
Create a release build: cargo build --release.
Do the equivalent of running the install script: ./dev/unix/build.sh; and ./dev/unix/install.sh.
Open a new shell, or refresh your current shell with the updated config: source ~/.config/fish/config.fish.

Then you should be able to use Notion!

crates/notion-core/src/shell/fish.rs

scalvert · 2019-02-12T22:27:38Z

@chriskrycho thank you, sir!

chriskrycho · 2019-02-13T15:04:45Z

@scalvert @chadhietala @locks can you all do me a favor and make sure this works?

Testing

Check out this branch locally. If you're using hub, hub pr checkout 266. (If you're not using hub… you should do that thing. And also alias it to git to make your life wonderful.)
Create a release build: cargo build --release.
Do the equivalent of running the install script: ./dev/unix/build.sh; and ./dev/unix/install.sh.
Open a new shell, or refresh your current shell with the updated config: source ~/.config/fish/config.fish.

Then you should be able to use Notion!

locks

Works on my machine.

chadhietala · 2019-02-13T18:15:00Z

chadhietala · 2019-02-13T22:38:23Z

I think there might be some issues with deactivate?

set: Warning: $PATH entry "/Applications/Visual Studio Code.app/Contents/Resources/app/bin/:/Users/chietala/Code/binaryen/bin:/Users/chietala/.cargo/bin:/Users/chietala/.config/node/default/bin:/Users/chietala/.yarn/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/linkedin/bin:/export/content/linkedin/bin" is not valid (No such file or directory)
set: Did you mean 'set PATH $PATH /Users/chietala/Code/binaryen/bin:/Users/chietala/.cargo/bin:/Users/chietala/.config/node/default/bin:/Users/chietala/.yarn/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/linkedin/bin:/export/content/linkedin/bin'?

chriskrycho · 2019-02-14T00:38:09Z

Ah, so, I think this is further up than the changes I made, but I know what the issue is: fish expects a path where entries are separated by spaces rather than :. The problem is the path as we're getting it in System (here). And that in turn is from yet further upstream, in envoy, which in turn just uses std::var_os.

I think, to make this do the right thing, I'll just need to add some special handling here, because what we have here is just what the OS reports back to us out of its getenv, so far as I can see. I'll double-check that analysis tomorrow, and add some special handling for it if so.

~~Should just be able to do something like path.replace(":", " ") when we're in fish, but I'm not sure whether locations can includes : if quoted.~~

See below!

chriskrycho · 2019-02-14T01:37:19Z

@chadhietala an update on the above: I can't reproduce the error you're seeing, but also running notion activate or notion deactivate doesn't actually seem to be doing anything. 🤔

chriskrycho · 2019-02-14T01:49:56Z

That path style actually seems to be fine; the real issue is that we're not quoting the value properly! I.e. this is part of #99.

chriskrycho · 2019-02-14T02:37:54Z

@chadhietala the latest commit I pushed should have fixed it (as well as, you know, just making activate and deactivate do what they should) – can you test again and confirm?

scalvert · 2019-02-14T03:50:20Z

Checking!

chadhietala · 2019-02-14T14:28:05Z

~~No dice.~~

I think everything might be hosed from when I tried to deactivate last time. Where does Notion write these shell files? I looked at ~/.config/fish/config.fish does it write anywhere else? I get this same Warning message whenever I open another shell.

I was on a very old version of Fish.

chriskrycho · 2019-02-14T14:38:02Z

Interesting. The quoting should have fixed this. 🤔

It's not writing it to anywhere in the deactivate/activate phase; it's just changing what the exported PATH value is for the shell and any children.

chriskrycho · 2019-02-14T16:03:45Z

@chadhietala and I did some digging, and the issue around notion deactivate is that versions of fish earlier than 3.0 complain if you attempt to set a value of PATH (except, apparently, in .config/fish/config.fish?) that includes a non-existent/non-directory location, it prints a warning, exits with status 1, and does not update the PATH. Fish 3.0.0 changed this behavior so it works correctly.

This isn't something Notion can (or even should) control or work around: the exact same thing happens if you just run set -x PATH on the CLI with an invalid location on older versions of fish.

As such, the best solution here seems to be documentation: let users know that the two solutions are:

to edit their PATH in their ~/.config/fish/config.fish file to remove any items which do not exist on their system
to upgrade to fish ^3.0

@dherman, thoughts on where exactly we should document that? I was thinking in the notion activate and notion deactivate docs, since that's where this comes up?

charlespierce · 2019-02-14T17:38:21Z

@chriskrycho Would it be possible to add something to the load.fish script that checks the exit code after source $NOTION_POSTSCRIPT and print a message if it finds a 1 there? That would be closest to the actual issue and would let a user know after they run notion activate or notion deactivate that it didn't work and they'll need to make changes

chriskrycho · 2019-02-14T19:28:36Z

We’d have to put it in the fish postscript itself, actually; I considered that but wasn’t sure how much value that adds vs. just documenting it clearly and searchably. Let me experiment with doing just that and see how it comes out.

chriskrycho · 2019-02-14T20:36:56Z

Okay, I've made that change and it looks like this:

Wording changes and formatting suggestions very welcome!

scalvert · 2019-02-15T02:29:00Z

Everything is fine until the post-install step. Sourcing ~/.config/fish/config.fish doesn't result in notion having been added to my path. If I cat ~/.config/fish/config.fish, it's unmodified. I would have expected notion's install to modify it.

chriskrycho · 2019-02-15T02:32:05Z

Weird. It worked fine for me and others; I will follow up with you in the morning!

chriskrycho · 2019-02-15T17:23:39Z

@scalvert and I synced up; the problem here was that his $SHELL was /etc/bash instead of fish. It did correctly set up his bash profile! Once he updated his shell everything Just Worked™.

scalvert · 2019-02-15T17:31:45Z

So the issue I had ended up being that my default shell was bash. So, the install was assuming, via the $SHELL env var, that I was using bash, not fish. I use iTerm2, and have it configured to run fish, which is why my env var wasn't set.

Setting $SHELL to fish solved the install problem.

- We cannot use `fish_user_paths`, nice though that would be, because it does not work correctly with our *unset* logic: the `PATH` we set up during installation in the user's profile stays there regardless of what we do with `fish_user_paths`. - We need to properly quote the string we render into the `PATH` value so that when a user has a path entry which has e.g. spaces in it, it works correctly when fish validates it.

stefanpenner · 2019-02-19T18:36:02Z

🕺

benblank reviewed Feb 11, 2019

View reviewed changes

crates/notion-core/src/shell/fish.rs Show resolved Hide resolved

chriskrycho changed the title ~~WIP: fish support~~ Add fish support 🐟 Feb 12, 2019

locks approved these changes Feb 13, 2019

View reviewed changes

scalvert and others added 8 commits February 15, 2019 12:39

[WIP] First pass at fish shell support

6a205e3

Change newline

692ee04

Core: Add load.fish script.

8ca9cfa

Core: colocate bash unit tests with implementation.

35734b8

Core: add fish to current shell.

a4397d5

Core: implement fish postscript.

73eb943

Core: support adding fish during installation.

7461fc3

Core: add error messaging for known fish failure case.

95ef6da

charlespierce merged commit 152e8c4 into volta-cli:master Feb 19, 2019

chriskrycho deleted the fish-support branch February 19, 2019 20:01

ghost mentioned this pull request Feb 27, 2019

tried installing under fish, $PATH not modified #286

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fish support 🐟 #266

Add fish support 🐟 #266

chriskrycho commented Feb 9, 2019 •

edited

scalvert commented Feb 12, 2019

chriskrycho commented Feb 13, 2019

locks left a comment

chadhietala commented Feb 13, 2019

chadhietala commented Feb 13, 2019

chriskrycho commented Feb 14, 2019 •

edited

chriskrycho commented Feb 14, 2019 •

edited

chriskrycho commented Feb 14, 2019

chriskrycho commented Feb 14, 2019 •

edited

scalvert commented Feb 14, 2019

chadhietala commented Feb 14, 2019 •

edited

chriskrycho commented Feb 14, 2019

chriskrycho commented Feb 14, 2019

charlespierce commented Feb 14, 2019

chriskrycho commented Feb 14, 2019

chriskrycho commented Feb 14, 2019

scalvert commented Feb 15, 2019

chriskrycho commented Feb 15, 2019

chriskrycho commented Feb 15, 2019

scalvert commented Feb 15, 2019

stefanpenner commented Feb 19, 2019

Add fish support 🐟 #266

Add fish support 🐟 #266

Conversation

chriskrycho commented Feb 9, 2019 • edited

Testing

scalvert commented Feb 12, 2019

chriskrycho commented Feb 13, 2019

Testing

locks left a comment

Choose a reason for hiding this comment

chadhietala commented Feb 13, 2019

chadhietala commented Feb 13, 2019

chriskrycho commented Feb 14, 2019 • edited

chriskrycho commented Feb 14, 2019 • edited

chriskrycho commented Feb 14, 2019

chriskrycho commented Feb 14, 2019 • edited

scalvert commented Feb 14, 2019

chadhietala commented Feb 14, 2019 • edited

chriskrycho commented Feb 14, 2019

chriskrycho commented Feb 14, 2019

charlespierce commented Feb 14, 2019

chriskrycho commented Feb 14, 2019

chriskrycho commented Feb 14, 2019

scalvert commented Feb 15, 2019

chriskrycho commented Feb 15, 2019

chriskrycho commented Feb 15, 2019

scalvert commented Feb 15, 2019

stefanpenner commented Feb 19, 2019

chriskrycho commented Feb 9, 2019 •

edited

chriskrycho commented Feb 14, 2019 •

edited

chriskrycho commented Feb 14, 2019 •

edited

chriskrycho commented Feb 14, 2019 •

edited

chadhietala commented Feb 14, 2019 •

edited