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

docs(#1036): Update linux installation instructions via apt #1037

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

Conversation

Nikolai2038
Copy link

Description

Fix #1036 - I modify instructions in READMEs to avoid two errors when installing Bruno in Linux via Apt (see issue for more info).

Contribution Checklist:

  • The pull request only addresses one issue or adds one feature.
  • The pull request does not introduce any breaking changes
  • I have added screenshots or gifs to help explain the change if applicable.
  • I have read the contribution guidelines.
  • Create an issue and link to the pull request.

Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.

@Nikolai2038
Copy link
Author

  • Added instruction to install gpg (I tried on new Debian WSL, and it is not installed by default).

  • Fixed instruction to create /root/.gnupg directory - with correct rights to avoid this warning:

    gpg: WARNING: unsafe permissions on homedir '/root/.gnupg'
    

@nielsbom
Copy link

Not sure, but feels like #1203 is related.

@antony
Copy link

antony commented Mar 19, 2024

These instructions worked for me after a lot of searching online. Mostly, the sudo mkdir -m 700 -p /root/.gnupg. Not sure why this is needed, but it is.

@janos-r
Copy link
Contributor

janos-r commented Mar 25, 2024

This is definitely not necessary on LinuxMint and should not be added as a general rule.
To add anything manually into /root/ just to satisfy missing configuration to install something sounds crazy!
Find out the cause and what is the standard fix.
Is this happening only on debian?
Is it happening only if you don't have previously gpg installed?

Btw, there is already the instruction
sudo gpg --no-default-keyring --keyring /etc/apt/keyrings/bruno.gpg --keyserver keyserver.ubuntu.com --recv-keys 9FA6017ECABE0266
That should already error out without a working gpg right?

@antony
Copy link

antony commented Mar 26, 2024

My gpg was configured and already in use.

I searched comprehensively for a fix, to no avail.

Only creating this thing in root fixed it - I'm using Pop_Os

@janos-r
Copy link
Contributor

janos-r commented Mar 27, 2024

Ok, I tried on a fresh linuxMint install in a VM and this is my result:
gpg is preinstalled, but if you never ran it before, than it didn't yet create the .gnupg folder in either /root/ or ~/
I don't know why gpg doesn't create it automatically when importing this key, but rather than creating the folder manually, it is better to let gpg create it. It also creates your own public key and other files that belong there! As explained in https://www.redhat.com/sysadmin/getting-started-gpg

So ok, I agree this check can be part of the install script for linux. But instead of doing it with mkdir, it can be checked with just sudo gpg --list-keys. It will either list the subscribed public keys under root (there shouldn't be any, normally you don't need sudo to verify files) or it will create the folder with everything that belongs there:

$ gpg --list-keys
gpg: directory '/home/bestuser/.gnupg' created
gpg: keybox '/home/bestuser/.gnupg/pubring.kbx' created
gpg: /home/bestuser/.gnupg/trustdb.gpg: trustdb created

@kinuax
Copy link

kinuax commented May 20, 2024

I confirm sudo gpg --list-keys creates the needed /root/.gnupg directory, thanks!

readme.md Outdated Show resolved Hide resolved
@@ -61,12 +61,11 @@ flatpak install com.usebruno.Bruno

# On Linux via Apt
sudo mkdir -p /etc/apt/keyrings
sudo apt-get update && sudo apt-get install gpg
sudo gpg --list-keys

Choose a reason for hiding this comment

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

I think it probably is too noisy to add a comment here for why this is needed. But maybe squashing the commits and adding the necessity for this line there?

Copy link
Author

Choose a reason for hiding this comment

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

You mean, something like that?

sudo gpg --list-keys && sudo gpg --no-default-keyring --keyring ...

Choose a reason for hiding this comment

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

The command to list keys looks like a noop, but it actually does something (as discussed in the PR). This is surprising, so a comment may help.

But: a comment pointing out why this command is there, at that place is probably not relevant for 99% of users. So that's why I suggested squashing the commits and adding a line about this to the commit message.

Copy link
Author

Choose a reason for hiding this comment

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

The commit, where I added this command, has message:

In APT instructions: Use "gpg --list-keys" to create ".gnupg" directory

Are you sure squashing needed?

Choose a reason for hiding this comment

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

Ah, didn't see that one, my bad.

I think in general squashing commits from (esp smaller) feature branches is a good thing. But it depends a bit on the practices of the project.

Copy link
Author

Choose a reason for hiding this comment

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

Alright, I've squished commits related to gpg now - you can see commit

readme.md Outdated Show resolved Hide resolved
@Nikolai2038 Nikolai2038 force-pushed the docs/update-linux-installation-instructions-via-apt branch from 4e841e1 to b8a51a8 Compare May 22, 2024 18:42
@Nikolai2038 Nikolai2038 requested a review from nielsbom May 22, 2024 18:57
@helloanoop
Copy link
Contributor

@Nikolai2038

All looks good. I would need to review this part to make sure that all of these are really needed. And that will take time.

deb: {
    // Docs: https://www.electron.build/configuration/linux#debian-package-options
    depends: [
      'libgtk-3-0',
      'libnotify4',
      'libnss3',
      'libxss1',
      'libxtst6',
      'xdg-utils',
      'libatspi2.0-0',
      'libuuid1',
      'libsecret-1-0',
      'libasound2' // #1036
    ]
  },

Can you submit the above part as a separate PR and keep this PR just to update the linux install instructions ?

@helloanoop helloanoop self-assigned this May 22, 2024
- Add instructions to install gpg;
- Use "gpg --list-keys" to let gpg create ".gnupg" directory with correct rights;
- Use "arch=amd64" - see commit 6c8c87f.
@Nikolai2038 Nikolai2038 force-pushed the docs/update-linux-installation-instructions-via-apt branch from b8a51a8 to 5f9c21d Compare May 22, 2024 19:39
@Nikolai2038
Copy link
Author

@helloanoop Thanks for the response! I updated this branch and created separate pull request.

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.

[Docs] Errors when following installation on Linux via apt
7 participants