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

Add brew standard installation path for gpg common paths #145

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

EmrysMyrddin
Copy link
Contributor

On macOS, we generally use brew to install gpg. This PR aims to add the brew installed gpg binary path to the common paths to look up for.

In my case, for some unknown reason, gpg was not resolved while it is actually part of the $PATH variable.

@maximbaz
Copy link
Member

maximbaz commented Mar 6, 2024

Thanks! This was brought up before, I'm still not sold on including such custom paths in the default fallback list; homebrew is although popular not the only package manager for macOS, and even homebrew has changed the folder they use when M1 was released. I don't think we should keep up with all the package managers in the world, this is really meant more like a fallback list.

You have correctly identified that the actual issue is that /opt/homebrew/bin/ is not in $PATH on your machine, as seen by the browser. Your $PATH is getting updated in the terminal, but the GUI apps don't receive that update (this is why gpg works in the terminal, and why launching your browser from terminal will make your browserpass work without this PR). This is actually not macOS-specific, but somehow it's more "talked about" in the Linux world on how and why environment variables like $PATH must be exported in a way that GUI apps see them.

If I remember correctly, one way to fix this on macOS is to run sudo launchctl config user path $PATH in your terminal - once is enough, though if your $PATH changes you might want to run it again.

Give it a go! It's a useful thing to do potentially even for other apps 🙂

If you can make it work, I'd appreciate a PR for a new FAQ entry for this topic from someone who recently went through the steps and confirmed which exact steps solve the issue 😉

@EmrysMyrddin
Copy link
Contributor Author

EmrysMyrddin commented Mar 7, 2024

I totally understand your point, following all package manager default installation conventions could be a not so fun approach.

On the other hand, the installation process is already a bit opaque and really not user friendly. I found out what was happening but it was really not easy, I've try hard to make it work.

The fact that used to work on my previous mac is even more confusing for me ^^'

I would argue that we should either support most popular package manager as default path, or not provide a default path at all and always require this setting.

Because today, it's confusing and it's difficult to know what is happening.

Another approach could be de show a very clear message that indicate that the configuration is broken and show a clear message about how to fix it.

Because today, the UI will just show a JSON blob error in an ephemeral alert (which is not displayed entirely because the window is generaly too small). We should check upfront if the config is working, because letting user see the passwords list and failing only once you try to retreive it is not a very happy path ^^'

@EmrysMyrddin
Copy link
Contributor Author

Couldn't we spawn a shell and try to get pgp path from the shell session instead of relying on GUI env ?

@maximbaz
Copy link
Member

maximbaz commented Mar 7, 2024

Couldn't we spawn a shell and try to get pgp path from the shell session instead of relying on GUI env ?

It would probably work in even less cases, not only because you need to guess the shell (not just launching /bin/sh), but also because spawning a shell in non-login non-interactive mode would typically not read all your config files, which might mean that the value of PATH would be different compared to you opening a GUI terminal.

Another approach could be de show a very clear message that indicate that the configuration is broken and show a clear message about how to fix it.

This is certainly a good idea to improve the error handling, the error json blob is something that was meant to be improved since the beginning, but never ended up being acted upon 😅

I can totally relate to everything you say about the experience being confusing, difficult and not user-friendly, this deserves to be improved!

This is one reason why I like your idea about better error message, it would help everyone, no matter which funky unpopular distro or package manager they used, everyone deserves to have a good experience to figure out what is wrong and how to make the extension work.

One other reason that again speaks in favour of error messages is that it can be done purely on frontend - the distribution of native component is a painful process, so it was designed with the goal of being as thin of a layer as possible, something that almost never has to be updated - and maintaining lists of package managers would go against that core principle as well.

Do you have the energy or enthusiasm in you to have a look and propose how a better error message might look like? No need to solve everything at once, just getting this one specific error about missing "gpg" presented in a better way would be a very good start?

@EmrysMyrddin
Copy link
Contributor Author

EmrysMyrddin commented Mar 7, 2024

It would probably work in even less cases, not only because you need to guess the shell (not just launching /bin/sh), but also because spawning a shell in non-login non-interactive mode would typically not read all your config files, which might mean that the value of PATH would be different compared to you opening a GUI terminal.

I see :/ I wonder how they found a way to make it work in tools like VSCode ^^'

I see your point about requiring as less frequent updates as possible. You totally don't imagine myself manually update the native app companion :-)

So yeah, better UI with better errors display is probably the way to go with this.

I was imagining a "first experience" screen, with a series of checks, like:

  • Is Native App accessible ?
  • Is password store accessible ?
  • Is GPG found ?
  • Are necessary keys available to decrypt password ?

Not sure where or when this screen could be shown by the way. Perhaps we can keep some state in settings to know if this first checks have been passed or not, and display it instead of the actual password list if not ?

I also remember that extensions can open a page right after its installation. Perhaps this could also be a good place to show this "setup" screen :-)

@maximbaz
Copy link
Member

Let's do it even simpler, we don't need to keep a state, or distinguish first launch; the extension talks to native host and fetches settings on every single interaction, so for example if it couldn't talk to native host, we should show a good and descriptive error about that (instead of the currently generic "Error: Error: ..."), and similarly if gpg was not found, a different descriptive error could be shown. What makes it even better is that native host already sends descriptive errors, so it's purely the frontend part that needs to be improved 🙂

If you want to play around, I believe this place might be a good starting point, to see what kind of data there is, and then how this variable is currently getting used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants