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

earlyoom: use upstream systemd service and add proper test #331580

Merged
merged 3 commits into from
Jan 1, 2025

Conversation

oxalica
Copy link
Contributor

@oxalica oxalica commented Aug 1, 2024

Description of changes

  • earlyoom ships a system service since 1.8 with additional sandboxing and protection rulees. This installs and uses it.
  • A functionality test on earlyoom is added.

It seems that we have withManpage option even though manual is installed to a separate output man. If user only get out in their closure when documentation is disabled, why would we need a separate withManpage flag? Could we remove and always enable it?
Ok, I kept it with a comment. It may be useful when someone is targeting a platform without pandoc.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Aug 1, 2024
@ofborg ofborg bot requested a review from AndersonTorres August 1, 2024 19:45
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Aug 1, 2024
pkgs/by-name/ea/earlyoom/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ea/earlyoom/package.nix Outdated Show resolved Hide resolved
@AndersonTorres
Copy link
Member

AndersonTorres commented Aug 1, 2024

It seems that we have withManpage option even though manual is installed to a separate output man. If user only get out in their closure when documentation is disabled, why would we need a separate withManpage flag? Could we remove and always enable it?

Building the manpage requires extra programs, namely pandoc and its dependencies (including GHC).
Because of it, manpage building and installation are optional.

Further, it is desirable to split outputs. A more restricted host environment can be stripped of some otherwise undesirable files to save space.

@ofborg ofborg bot requested a review from AndersonTorres August 6, 2024 16:31
@h7x4 h7x4 added the 8.has: tests This PR has tests label Aug 21, 2024
@github-actions github-actions bot added 8.has: documentation This PR adds or changes documentation 8.has: changelog labels Oct 3, 2024
@oxalica oxalica requested a review from MinerSebas October 3, 2024 05:35
@ofborg ofborg bot requested a review from AndersonTorres October 3, 2024 06:07
runHook postInstall
'';
"PREFIX=${placeholder "out"}"
] ++ lib.optional withManpage "MANDIR=${placeholder "man"}/share/man";
Copy link
Member

Choose a reason for hiding this comment

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

Does this placeholder work? Usually I bumped into some issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this placeholder work? Usually I bumped into some issues.

I'm not sure what issue are you referring to? With these options, the output layout looks correct for me.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 10, 2024
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 1, 2025
@misuzu misuzu merged commit a222078 into NixOS:master Jan 1, 2025
24 of 25 checks passed
@oxalica oxalica deleted the pkg/earlyoom branch January 1, 2025 10:34
msfjarvis added a commit to msfjarvis/dotfiles that referenced this pull request Jan 4, 2025
Stopped working after changes from NixOS/nixpkgs#331580
default = [ ];
example = [
"-g"
"--prefer '(^|/)(java|chromium)$'"
Copy link
Contributor

Choose a reason for hiding this comment

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

This format seems to no longer work due to the call to lib.escapeShellArgs that wasn't previously there. I had to split out space-separated flags into individual list items: msfjarvis/dotfiles@a711550

Without the change in my configuration, the flag that ended up in EARLYOOM_ARGS was '--avoid '\\''^(gnome.*|firefox.*|pipewire.*|git.*)$'\\''' which was seen by earlyoom as a single parameter not a flag and caused the service to stop running.

Copy link
Contributor

Choose a reason for hiding this comment

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

@msfjarvis Thanks, finally was looking into why my earlyoom service hasn't worked for a few days and saw this PR.

Copy link
Member

@Artturin Artturin Jan 6, 2025

Choose a reason for hiding this comment

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

@oxalica this needs a release note and the example fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: tests This PR has tests 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants