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

feat: ipfs on PATH on runtime #906

Merged
merged 37 commits into from
Apr 26, 2019
Merged

feat: ipfs on PATH on runtime #906

merged 37 commits into from
Apr 26, 2019

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Apr 16, 2019

Closes #727.

See #906 (comment)

hacdias added 3 commits April 13, 2019 21:23
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@ghost ghost assigned hacdias Apr 16, 2019
@ghost ghost added the in progress label Apr 16, 2019
@hacdias hacdias changed the title wip: ask to replace 'ipfs' if exists on macOS feat: ask to replace 'ipfs' if exists on macOS Apr 16, 2019
@hacdias hacdias requested review from fsdiogo and olizilla April 16, 2019 09:02
@hacdias
Copy link
Member Author

hacdias commented Apr 16, 2019

Just noticed that if the user says 'No', every time they start IPFS Desktop, they'll be prompted.

What do you think? If we add an option 'No, and don't ask' we should provide a way to undo it if the user happens to change its mind.

Also, keep in mind this only affects macOS users.

@fsdiogo
Copy link
Contributor

fsdiogo commented Apr 16, 2019

Being nagged every time a user starts Desktop is no good.

I'd say we should just ask once, but have an option (a button for example) in the desktop specific settings to trigger the replacement.

@cwaring
Copy link
Member

cwaring commented Apr 16, 2019

This sounds like a good candidate for some kind of status block (and/or switch) in the sidebar, indicating which IPFS binary you are using system wide.

@hacdias
Copy link
Member Author

hacdias commented Apr 16, 2019

Hmm... I'm not sure if the status block would be a good way because this only affects macOS and it could require more work than what it's worth.

What if we show once and, if the user says no (or if /usr/local/bin/ipfs gets manually changed), we add an option (perhaps under Advanced or even in the main menu) to add it to PATH.

What do you think?

@cwaring
Copy link
Member

cwaring commented Apr 17, 2019

That sounds like a good compromise to get this implemented and we can always revisit later.

If you install IPFS Desktop and IPFS isn't in the users path does we automatically set it up?

I ask because presenting people with decisions on initial load can be hostile, especially if it feels permanent and the user doesn't yet understand the benefits.

My suggestion would be:

  • a) Add the configuration option in the settings and default to enabled if IPFS isn't currently found in the users path
  • b) Incentivise them to enable it with a dialog (preferably after an acton is taken): If IPFS is already found, present a prompt explaining the benefit of using the desktop binary as default but with a sentence in the dialog saying: "This can be changed at any time from the settings page."
  • c) Success 🥳

There may be other opportunities on the UI where we can present the option to enable this before an action is performed, such as first starting your node.

@hacdias
Copy link
Member Author

hacdias commented Apr 17, 2019

@cwaring I get you, but let me give you a little more context:

  1. On Windows, we add it to the PATH anyway during installation. No action required from the user.
  2. On Linux, we add it to the PATH if not found already during installation. No action required from the user. Can't be changed later via IPFS Desktop.
  3. On macOS, we do the same as we do on Linux but during runtime because it's possible. This way we have more flexibility and can ask the user if they want to replace it.

I just wanted to get feedback from more people too. /cc @olizilla @fsdiogo

@fsdiogo
Copy link
Contributor

fsdiogo commented Apr 17, 2019

@hacdias my opinion hasn't changed => #906 (comment).

@hacdias
Copy link
Member Author

hacdias commented Apr 18, 2019

So, just made some update: initially asks the user. If no, there will be an option under 'Advanced' to add to the PATH. It won't ask the user more times.

@hacdias
Copy link
Member Author

hacdias commented Apr 22, 2019

As an update: I'll use this PR and do what's described here (#727 (comment)) on this PR. So'll change the status to WIP

@hacdias hacdias changed the title feat: ask to replace 'ipfs' if exists on macOS wip: ask to replace 'ipfs' if exists on macOS Apr 22, 2019
@hacdias hacdias removed request for olizilla and fsdiogo April 22, 2019 16:18
@hacdias hacdias mentioned this pull request Apr 22, 2019
2 tasks
@hacdias hacdias requested review from olizilla and lidel April 23, 2019 08:38
@olizilla
Copy link
Member

The current install prompts you with this

Screenshot 2019-04-23 at 09 45 31

As we only prompt the user about installing the ipfs command if they already have one, this message should talk about the need to replace your existing command... how about:


Allow IPFS Desktop to update the IPFS CLI

IPFS desktop can keep the IPFS command-line interface (CLI) up to date. It will replace the existing ipfs command installed at /usr/local/bin/ipfs, and update it when new versions are available.

[ Update CLI ] [ Keep existing ]


@@ -14,13 +14,9 @@
"clean:webui": "shx rm -rf assets/webui/",
"build": "run-s build:*",
"build:webui": "run-s build:webui:*",
"build:webui:download": "npx ipfs-or-gateway -c QmfQkD8pBSBCBxWEwFSu4XaDVSWK6bjnNuaWZjMyQbyDub -p assets/webui/",
"build:webui:download": "npx ipfs-or-gateway -c QmQz6PVJPqRZZtShVZbc8J3VdZE8pEd7p5RGujvGGUmSyx -p assets/webui/",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This CID should always be the latest release of ipfs-webui unless it depends on a pre-release version.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@olizilla it's from the linked PR (ipfs/ipfs-webui#1016) and needed, yes.

olizilla and others added 2 commits April 23, 2019 12:59
Co-Authored-By: hacdias <hacdias@gmail.com>
Co-Authored-By: hacdias <hacdias@gmail.com>
@hacdias
Copy link
Member Author

hacdias commented Apr 23, 2019

The current install prompts you with this

Screenshot 2019-04-23 at 09 45 31

As we only prompt the user about installing the ipfs command if they already have one, this message should talk about the need to replace your existing command... how about:

Allow IPFS Desktop to update the IPFS CLI

IPFS desktop can keep the IPFS command-line interface (CLI) up to date. It will replace the existing ipfs command installed at /usr/local/bin/ipfs, and update it when new versions are available.

[ Update CLI ] [ Keep existing ]

Kinda. We also show it on Linux because there we always need permission. I'll update this later with a matrix of different messages.

hacdias added 2 commits April 23, 2019 14:50
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Member Author

hacdias commented Apr 23, 2019

@olizilla updated the messages, depending on whether or using Windows, or you already have ipfs, or you don't have it but we need permission.

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did some QA for Linux: exec-root works only with polkit agent running

That is not a big problem, most of user-friendly distributions will have polkit agent running, but we should improve error handling, especially for advanced users without it (see below)

hacdias added 10 commits April 23, 2019 16:59
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
Provides less ambiguity for us.

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Member Author

hacdias commented Apr 26, 2019

I'm merging this and adding the missing Web UI PR to the list of the next release #880.

@hacdias hacdias merged commit 2f9ae66 into master Apr 26, 2019
@ghost ghost removed the in progress label Apr 26, 2019
@hacdias hacdias deleted the feat/ask-to-replace-ipfs branch April 26, 2019 18:05
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 this pull request may close these issues.

Provide ipfs command line tools on install
5 participants