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

Provide ipfs command line tools on install #727

Closed
olizilla opened this issue Dec 5, 2018 · 17 comments · Fixed by #906
Closed

Provide ipfs command line tools on install #727

olizilla opened this issue Dec 5, 2018 · 17 comments · Fixed by #906
Assignees

Comments

@olizilla
Copy link
Member

olizilla commented Dec 5, 2018

Atom.app provides the apm command when it is installed, as do many other apps. It would be rad if installing IPFS Desktop added the ipfs command to your PATH if it's not already present.

We already bundle the binaries, so it's just a matter of figuring out how to make them available.

@olizilla
Copy link
Member Author

Can you tackle this next... it'd really help us with the "easy install" story for ipfs-inactive/website#299

@lidel
Copy link
Member

lidel commented Mar 22, 2019

Some notes from IRC about PATH on Linux:

Symlinking to /usr/local/bin is possible only during install

iirc after install on Linux ipfs-desktop process runs under non-root user and cant write to /usr/local/

$ ln -s ./ipfs /usr/local/bin/ipfs
ln: failed to create symbolic link '/usr/local/bin/ipfs': Permission denied

That being said, we should be fine with symlinking to /usr/local/bin/ipfs during install on Linux. Symlink may conflict with plain go-ipfs package, but if desktop user wants to run a different version of go-ipfs than one shipped with ipfs-desktop, they can do it either by docker run --rm -it --net=host ipfs/go-ipfs:v0.4.18 or pointing at remote node via ~/.ipfs/api

Adding ipfs binary to PATH needs to account for shell quirks

This is alternative to symlinking which would enable us to add/remove ipfs from the PATH of current user via a checkbox in UI, but comes with a lot of edge cases:

The main difference with shell config files is that some are only read by "login" shells (eg. when you login from another host, or login at the text console of a local unix machine). these are the ones called, say, .login or .profile or .zlogin (depending on which shell you're using).

Then you have config files that are read by "interactive" shells (as in, ones connected to a terminal (or pseudo-terminal in the case of, say, a terminal emulator running under a windowing system). these are the ones with names like .bashrc, .tcshrc, .zshrc, etc.

bash complicates this in that .bashrc is only read by a shell that's both interactive and non-login, so you'll find most people end up telling their .bash_profile to also read .bashrc

https://stackoverflow.com/a/415444

@hacdias
Copy link
Member

hacdias commented Mar 22, 2019

Thanks for bringing all the thoughts here @lidel. Another thing I want to mention is about Windows: on Windows we actually need to add a directory to the PATH since there's no default folder to put binaries into on Windows. And I'm not yet sure if we need elevated permissions during installation. Windows actually has two PATHs: a system-wide one and a user-specific one. I'll investigate a bit more about that.

On other news, we probably need to take in account #836. If we are going to add it to the PATH, perhaps we should solve both issues at once since they're closely related. Could you please comment on those? /cc @lidel @olizilla

@lidel
Copy link
Member

lidel commented Mar 22, 2019

I like the general idea from #836! What if we dont execute node_modules/go-ipfs-dep/go-ipfs/ipfs directly but introduce node_modules/{some-ipfs-wrapper}/bin/ipfs and add that to the PATH instead?

  • ipfs would be a small wrapper script that tries to execute ipfs from PATH
    • IF not there OR if entry on PATH point at itself (due to symlink) THEN the script executes binary currently set as default in ipfs-desktop, eg. node_modules/go-ipfs-dep/go-ipfs/ipfs
    • JS script run in Node should be more portable than one in Bash
    • The ipfs wrapper should accept input via parameters and STDIN
      (both ipfs swarm peers --latency and echo 'foo' | ipfs add should work)
    • Cool perk: having this wrapper, when user switched IPFS provider in IPFS Desktop, the ipfs in shell will also reflect that :)
  • Perhaps instead of creating {some-ipfs-wrapper} from scratch, we could tweak @alanshaw's iim and add this wrapper layer there?

@hacdias
Copy link
Member

hacdias commented Mar 23, 2019

I personally enjoy the idea of having that wrapper around the ipfs binary. Just to make sure I understood: if there is a binary in PATH, we use it. If not, we use our binary.

On the first run, we could add "our" script to the PATH if no ipfs binary can be found. Otherwise, should we ask the user if they want to update (if it's a previous version) or not?

@lidel
Copy link
Member

lidel commented Mar 24, 2019

If we manually add ipfs wrapper to PATH, then it should be added at the front of the PATH array to always have the priority. That way it could be used for switching between backends in the future.

We could detect if ipfs is already on PATH before wrapper is installed and display a warning to the user, but this will be tricky on linux if we add symlink during the install (check needs to occur before that).

To keep it simple, let's just assume user wants ipfs-desktop to do the thing (default to bundled version without any prompts).

@hacdias
Copy link
Member

hacdias commented Mar 25, 2019

@hacdias
Copy link
Member

hacdias commented Mar 27, 2019

@olizilla did you have the chance of reading @lidel's comments?

Overall I like the wrapper idea, although it adds the complexity of adding a directory to the PATH which is not straightfoward

@olizilla
Copy link
Member Author

olizilla commented Apr 3, 2019

Great stuff! To check we are on the same page, I don't think we should change the definition of the users PATH. I have mine tweaked and it's manged by git, so having a process change it would be a suprise. I don't think thats what is being suggested here, but I just wanted to check.

I strongly agree that we should keep this simple, and for the default case where ipfs is not on the PATH already, then we should just install it without prompting. I think we should start by looking into what Atom does: https://github.com/atom/atom/blob/cbcad274a7cb68774b9467decc7dee39c13d8e19/script/lib/install-application.js

On first run it attempt to symlink a shell script atom.sh to /usr/local/bin/atom. if it needs priviledges to do so, then it'll prompt the user.

$ ls -la `which atom`
lrwxr-xr-x  1 oli  admin  53 Oct 22 10:07 /usr/local/bin/atom -> /Applications/Atom.app/Contents/Resources/app/atom.sh

I'm in favour of @lidel's suggestion of adding a wrapper that we can use to manage which ipfs is being used, and I agree that we should use some of the logic from iim. Of note Atom also installs apm on first run, and it uses npm to fetch it... https://github.com/atom/atom/blob/cbcad274a7cb68774b9467decc7dee39c13d8e19/script/lib/install-apm.js

@hacdias
Copy link
Member

hacdias commented Apr 5, 2019

So, let's come to an agreement of what we need to do to close this issue.

@olizilla

To check we are on the same page, I don't think we should change the definition of the users PATH

You don't agree on changing the PATH right? I agree we shouldn't change in on macOS and Linux and just add it to /usr/local/bin. On Windows we are required to.

Of note Atom also installs apm on first run, and it uses npm to fetch it...

Does it bundle npm with it? We don't know if the users have it. I agree that once Windows is supported by iim, we should install it if the user has npm installed.


My suggestion for now:

  1. On Windows, add to PATH during installation (only option I can see). Ref for myself on prior work I've done on this and I just found out!!.
  2. On macOS, symlink to /usr/local/bin during first run if there is no binary in there yet. If there is, ask the user?
  3. On Linux, add it during installation to /usr/local/bin.

Other option I like is: to make iim support Windows. Then, use it as the ipfs dependency manager of IPFS Desktop (do not bundle go-ipfs nor anything) and let the user decide the version through iim. We'd, of course, get the latest first. This would, although, require users to have Node installed. What do you think?

Or make an iim like tool in other language that can be compiled such as Go. It just wouldn't support JS IPFS if the user didn't have Node. But it'd still support go-ipfs.

@olizilla
Copy link
Member Author

olizilla commented Apr 5, 2019

@hacdias please review the code in atom that does this. You get to decide how it works on windows. I recommend checking how some other apps do it. As much as possible the process should be the same on macOS vs Linux, so there are fewer variations to test.

@hacdias
Copy link
Member

hacdias commented Apr 5, 2019

@olizilla on Windows it seems they just don't add anything to the PATH.

On macOS, they add it on the first run. See here..

On Linux, they add it during installation. See here.

Basically what I've suggested.

As much as possible the process should be the same on macOS vs Linux, so there are fewer variations to test.

As @lidel mentioned, you can't modify /usr/local/bin after installation. And on macOS you can't modify it during installation so it's still different.


What's still to decide is what to put there: the bundled version? Ignore anything that already exists? We could assume that someone using IPFS Desktop doesn't install ipfs from the command line. We could also have a way to install multiple versions through IPFS Desktop afterwards.

This would not take in account #836 though.

@olizilla
Copy link
Member Author

olizilla commented Apr 5, 2019

Get the simplest thing working first. We're gonna need tests for the 3 different paths. Link to a shellscript that gives us enough indirection to change how it works later / update the binary without needing to update the link.

@hacdias
Copy link
Member

hacdias commented Apr 9, 2019

After #896 and #897, we now add ipfs to the PATH on Windows, macOS and Linux (deb and rpm only). What's missing:

@hacdias
Copy link
Member

hacdias commented Apr 22, 2019

So, we now have this enabled on all packages we ship except AppImage. It isn't possible with AppImage unfortunately. At least I haven't found a way to do so.

Although I've found an interesting solution that could help us making the process more uniform on Linux and macOS. If we did it on runtime, we could use this package (sudo-prompt) to ask for permissions on Linux. That way, the logic on macOS and Linux could be shared and we'd relieve the pain that the build of installers is.

What do you think @lidel? I believe it's not common on Linux to ask on runtime for permissions, but it'd be much easier and give more flexibility (like reverting the option in the interface). And it would work with all installers.

@lidel
Copy link
Member

lidel commented Apr 22, 2019

@hacdias I must agree, unifying everything around sudo-prompt sounds like a neat way to simplify codebase and remove Linux maintenance burden.

The only ask for Linux runtime would be to remove the "surprise factor" caused by sudo call.
We could

  • make it opt-in via tray menu (disabled by default on Linux)
  • explicitly ask during first time app starts ("Do you want to add ipfs to your PATH Y/n?").

Linux context: I was against adding the symlink at runtime, and we were trying to come up with electron-builder way to add post-install hooks that create symlinks and work the same in every Linux package type and it turned out to be an error prone process and a huge time sink. As noted, in the end it does not work everywhere anyway. We'd have to either drop electron builder or spend significant time on patching it and maintaining QA for multiple package types. I believe the time could be better spent elsewhere given our limited resources.

@hacdias
Copy link
Member

hacdias commented Apr 22, 2019

@lidel yes, the point was to ask first and merge this on #906. What do you think?

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 a pull request may close this issue.

3 participants