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

scripts: don't refresh the pacman repository on Arch #12194

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

Conversation

noncombatant
Copy link
Contributor

Fixes #12186

@noncombatant noncombatant requested review from awly and knyar May 19, 2024 21:08
Comment on lines 516 to 519
# https://github.com/tailscale/tailscale/issues/12186
if [ "$OS" != "arch" ]; then
$SUDO pacman -Sy
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like the issue is not specific to Arch, but any distro using pacman.
We should probably never run pacman -Sy.

Copy link

Choose a reason for hiding this comment

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

Why does it need to use pacman -Sy anyway?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's similar to all other repos - refresh the index to fetch the latest version. Arch is the only one that actually discourages this practice.

It matters more on rpm and deb-based distros, where we own the repo and add it in this same installer script. Without a refresh, apt/dnf/yum wouldn't know about the tailscale package.

I agree that it is probably not necessary on pacman-based distros, unless you have a fresh install where pacman -Sy has never ran before (e.g. when you run a fresh archlinux docker container`).

@sylv256
Copy link

sylv256 commented May 19, 2024

The Arch CI is broken because it does not already have the package database downloaded like a regular system would. pacman -S and pacman -Syu (this one is debatable. who would want to upgrade during an installation script?) are safe, but pacman -Sy is not.

@awly
Copy link
Contributor

awly commented May 19, 2024

@noncombatant we need to update the CI config to special-case archlinux like we do here with others

- name: install dependencies (yum)
# tar and gzip are needed by the actions/checkout below.
run: yum install -y --allowerasing tar gzip ${{ matrix.deps }}
if: |
contains(matrix.image, 'centos')
|| contains(matrix.image, 'oraclelinux')
|| contains(matrix.image, 'fedora')
|| contains(matrix.image, 'amazonlinux')

@knyar
Copy link
Contributor

knyar commented May 20, 2024

I've just checked, and the installer script will fail on a fresh archlinux:latest image, with the error message advising to run pacman -Sy:

# bash install.sh
Installing Tailscale for arch, using method pacman
+ pacman -S tailscale --noconfirm
warning: database file for 'core' does not exist (use '-Sy' to download)
warning: database file for 'extra' does not exist (use '-Sy' to download)
error: target not found: tailscale

If it's not always safe to run that command, I think we have two options:

  1. Only run the command when we need to (e.g. when database files are missing), assuming there is a stable way to determine that.
  2. Completely remove the command from the installer script, and run it on CI before the script. This would not be my first choice, since ideally we'd want the script to do whatever is necessary (but not more) to install Tailscale.

@noncombatant
Copy link
Contributor Author

We could do if !pacman -S tailscale ...; pacman -Sy ... (it does return with status 1 when it fails — for any reason, though, not just unknown packages), but I think that still might surprise people who don't want -Sy to happen without their intent. So @knyar 's option 2 might make CI most like what Arch users (and other pacman-users?) expect, which seems better.

.github/workflows/installer.yml Outdated Show resolved Hide resolved
@sylv256
Copy link

sylv256 commented May 22, 2024

  1. Only run the command when we need to (e.g. when database files are missing), assuming there is a stable way to determine that.

On Arch and other rolling release pacman distributions, you'd still cause a partial upgrade. By using pacman -Sy, you promise that you will finish the upgrade.
Here's a better explanation of why you should never, ever, ever do this on an Arch system: https://bbs.archlinux.org/viewtopic.php?id=241092
The best thing to do would be to ensure the CI's database is up-to-date.

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.

Installer script automatically does a partial upgrade on Arch Linux.
4 participants