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(ublue-brew): add update/upgrade services and timers #296

Merged
merged 3 commits into from
Mar 23, 2025

Conversation

ledif
Copy link
Member

@ledif ledif commented Mar 19, 2025

These services and timers are taken directly from Aurora, with the exception of adding the unlink logic from #269 (comment) to brew-upgrade.service.

Closes #269.

@ledif ledif requested a review from Zeglius as a code owner March 19, 2025 02:33
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. brew homebrew related issues services Service units labels Mar 19, 2025
@ledif ledif requested a review from tulilirockz March 19, 2025 02:40
ExecStart=/usr/bin/bash -c "/home/linuxbrew/.linuxbrew/bin/brew upgrade"
# Prevent Brew from overriding important system binaries like:
# systemctl, loginctl, dbus-*
ExecStartPost=/usr/bin/bash -c "/home/linuxbrew/.linuxbrew/bin/brew unlink systemd dbus"
Copy link
Contributor

@fiftydinar fiftydinar Mar 19, 2025

Choose a reason for hiding this comment

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

If systemd or dbus is not installed in brew, service will report status as failed.
This fixes it.

Suggested change
ExecStartPost=/usr/bin/bash -c "/home/linuxbrew/.linuxbrew/bin/brew unlink systemd dbus"
ExecStartPost=/usr/bin/bash -c "if [[ -n \"\$(/home/linuxbrew/.linuxbrew/bin/brew list --formulae | awk '/(^|\\s)(dbus)($|\\s)/')\" ]]; then /home/linuxbrew/.linuxbrew/bin/brew unlink dbus; fi; if [[ -n \"\$(/home/linuxbrew/.linuxbrew/bin/brew list --formulae | awk '/(^|\\s)(systemd)($|\\s)/')\" ]]; then /home/linuxbrew/.linuxbrew/bin/brew unlink systemd; fi"```

Copy link
Collaborator

Choose a reason for hiding this comment

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

If systemd or dbus is not installed in brew, service will report status as failed.
This fixes it.

Suggested change
ExecStartPost=/usr/bin/bash -c "/home/linuxbrew/.linuxbrew/bin/brew unlink systemd dbus"
ExecStartPost=/usr/bin/bash -c "if [[ -n \"\$(/home/linuxbrew/.linuxbrew/bin/brew list --formulae | awk '/(^|\\s)(dbus)($|\\s)/')\" ]]; then /home/linuxbrew/.linuxbrew/bin/brew unlink dbus; fi; if [[ -n \"\$(/home/linuxbrew/.linuxbrew/bin/brew list --formulae | awk '/(^|\\s)(systemd)($|\\s)/')\" ]]; then /home/linuxbrew/.linuxbrew/bin/brew unlink systemd; fi"```

We can get away with a simpler || true at the end

@castrojo castrojo requested a review from a team March 20, 2025 15:58

[Timer]
OnBootSec=10min
OnUnitInactiveSec=6h
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this is inline with existing update timers (flatpak and bootc) but it seems a bit frequent to update every 6 hours.

Probably not a problem, but do we have an easy way to disable these for users?

Copy link
Member Author

Choose a reason for hiding this comment

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

Users can disable this timer completely with systemctl disable brew-update.timer or they can override the frequency with a drop-in config file

cat <<EOF > /etc/systemd/system/brew-update.timer.conf.d/override.conf
[Timer]
OnUnitInactiveSec=24h
EOF

Note that brew update doesn't actually upgrade any packages, but rather just does a git pull on Brew's repo to pull in new formulae.

ExecStart=/usr/bin/bash -c "/home/linuxbrew/.linuxbrew/bin/brew upgrade"
# Prevent Brew from overriding important system binaries like:
# systemctl, loginctl, dbus-*
ExecStartPost=/usr/bin/bash -c "/home/linuxbrew/.linuxbrew/bin/brew unlink systemd dbus || true"
Copy link
Contributor

Choose a reason for hiding this comment

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

I like how this addresses the concern about brew supplanting system services. It may need to be expanded if other problems are found, but if we're committed to providing brew for users, I think we do need this kind of d of safeguard.

Copy link
Member

Choose a reason for hiding this comment

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

Also instead of running this in a shell. Have the full path prefixed with a - then the service unit will not be marked failed.

Running things in a shell like this is usually not needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

True, running in a shell is not ideal.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I updated the unit to not spawn a shell.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Mar 23, 2025
@castrojo castrojo enabled auto-merge March 23, 2025 22:01
Environment=HOMEBREW_CELLAR=/home/linuxbrew/.linuxbrew/Cellar
Environment=HOMEBREW_PREFIX=/home/linuxbrew/.linuxbrew
Environment=HOMEBREW_REPOSITORY=/home/linuxbrew/.linuxbrew/Homebrew
ExecStart=/usr/bin/bash -c "/home/linuxbrew/.linuxbrew/bin/brew update"
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, do we need to spawn a shell here?

Copy link
Member

@tulilirockz tulilirockz left a comment

Choose a reason for hiding this comment

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

Looks good to me, worst case we can do a follow up as this is kinda hard to test

@tulilirockz tulilirockz added the safe-to-run PR is just fine to run, no RCE vulnerabilities found by manual review label Mar 23, 2025
@castrojo castrojo disabled auto-merge March 23, 2025 22:56
@castrojo castrojo merged commit c845a31 into ublue-os:main Mar 23, 2025
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
brew homebrew related issues lgtm This PR has been approved by a maintainer safe-to-run PR is just fine to run, no RCE vulnerabilities found by manual review services Service units size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unlink core system binaries from Brew
7 participants