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

adaptivemm: init at unstable-20230116 #225782

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

Conversation

cmm
Copy link
Member

@cmm cmm commented Apr 11, 2023

There are proper release tags, but the latest one is from Feb 2022 and there are buffer overflow fixes since.

Description of changes
Things done
  • Built on platform(s)
    • [v] x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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
  • [v] Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 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.

@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 Apr 11, 2023
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Apr 11, 2023
There are proper release tags, but the latest one is from Feb 2022
and there are buffer overflow fixes since.
@cmm cmm force-pushed the pkg/adaptivemm branch from c2823ea to 6c3d98d Compare April 14, 2023 20:22
@cmm cmm requested a review from ocfox April 14, 2023 20:22
@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Apr 14, 2023
@h7x4 h7x4 added 8.has: module (new) This PR adds a module in `nixos/` and removed 8.has: module (update) This PR changes an existing module in `nixos/` labels Oct 16, 2023
Comment on lines +17 to +23

installPhase = ''
mkdir -p $out/bin $out/etc/sysconfig $out/share/man/man8
cp adaptivemmd $out/bin/
cp $src/adaptivemmd.cfg $out/etc/sysconfig/adaptivemmd
cp $src/adaptivemmd.8 $out/share/man/man8/
'';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
installPhase = ''
mkdir -p $out/bin $out/etc/sysconfig $out/share/man/man8
cp adaptivemmd $out/bin/
cp $src/adaptivemmd.cfg $out/etc/sysconfig/adaptivemmd
cp $src/adaptivemmd.8 $out/share/man/man8/
'';
nativeBuildInputs = [ installShellFiles ];
installPhase = ''
runHook preInstall
install -Dm555 adaptivemmd $out/bin/adaptivemmd
install -Dm444 adaptivemmd.cfg $out/etc/sysconfig/adaptivemmd
installManPage adaptivemmd.8
runHook postInstall
'';

@@ -0,0 +1,31 @@
{ lib, stdenv, fetchFromGitHub }:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{ lib, stdenv, fetchFromGitHub }:
{ lib, stdenv, fetchFromGitHub, installShellFiles }:

Comment on lines +25 to +30
meta = with lib; {
description = "A userspace daemon for proactive free memory management";
license = licenses.gpl2;
platforms = platforms.linux;
maintainers = with maintainers; [ cmm ];
};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
meta = with lib; {
description = "A userspace daemon for proactive free memory management";
license = licenses.gpl2;
platforms = platforms.linux;
maintainers = with maintainers; [ cmm ];
};
meta = with lib; {
description = "A userspace daemon for proactive free memory management";
homepage = "https://github.com/oracle/adaptivemm";
license = licenses.gpl2;
platforms = platforms.linux;
maintainers = with maintainers; [ cmm ];
mainProgram = "adaptivemmd";
};

Comment on lines +12 to +19
package = mkOption {
type = types.package;
default = pkgs.adaptivemm;
defaultText = literalExpression "pkgs.adaptivemm";
description = lib.mdDoc ''
Which adaptivemm package to use.
'';
};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
package = mkOption {
type = types.package;
default = pkgs.adaptivemm;
defaultText = literalExpression "pkgs.adaptivemm";
description = lib.mdDoc ''
Which adaptivemm package to use.
'';
};
package = mkPackageOptionMD pkgs "adaptivemm" { };

};

verbosity = mkOption {
type = types.int;
Copy link
Member

Choose a reason for hiding this comment

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

There's some magic stuff in lib that let's us avoid the nasty asserts.

Suggested change
type = types.int;
type = types.int.between 0 5;

};

aggressiveness = mkOption {
type = types.int;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type = types.int;
type = types.int.between 1 3;

Comment on lines +61 to +66
verboseFlag = assert (cfg.verbosity >= 0 && cfg.verbosity <= 5);
if cfg.verbosity == 0 then ""
else lib.concatStrings (["-"] ++ (lib.replicate cfg.verbosity "v"));
aggressivenessFlag = assert (cfg.aggressiveness >= 1 && cfg.aggressiveness <= 3);
"-a ${builtins.toString cfg.aggressiveness}";
gapFlag = "-m ${builtins.toString cfg.maxWatermarkGap}";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
verboseFlag = assert (cfg.verbosity >= 0 && cfg.verbosity <= 5);
if cfg.verbosity == 0 then ""
else lib.concatStrings (["-"] ++ (lib.replicate cfg.verbosity "v"));
aggressivenessFlag = assert (cfg.aggressiveness >= 1 && cfg.aggressiveness <= 3);
"-a ${builtins.toString cfg.aggressiveness}";
gapFlag = "-m ${builtins.toString cfg.maxWatermarkGap}";
verboseFlag = if cfg.verbosity == 0 then ""
else "-${lib.concatStrings (lib.replicate cfg.verbosity "v")}";
aggressivenessFlag = "-a ${toString cfg.aggressiveness}";
gapFlag = "-m ${toString cfg.maxWatermarkGap}";

Comment on lines +83 to +85
meta = {
maintainers = with maintainers; [ cmm ];
};
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
meta = {
maintainers = with maintainers; [ cmm ];
};
meta.maintainers = with maintainers; [ cmm ];

Comment on lines +51 to +59
environment = {
systemPackages = [ cfg.package ];
etc."sysconfig/adaptivemmd".source = "${cfg.package}/etc/sysconfig/adaptivemmd";
};

systemd.services.adaptivemmd = {
serviceConfig = {
Type = "forking";
EnvironmentFile = "/etc/sysconfig/adaptivemmd";
Copy link
Member

Choose a reason for hiding this comment

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

unless you have a strong suspicion that it would be beneficial to the average user to run this manually, I think I would just hide this file and keep the package away from the environment.

Suggested change
environment = {
systemPackages = [ cfg.package ];
etc."sysconfig/adaptivemmd".source = "${cfg.package}/etc/sysconfig/adaptivemmd";
};
systemd.services.adaptivemmd = {
serviceConfig = {
Type = "forking";
EnvironmentFile = "/etc/sysconfig/adaptivemmd";
systemd.services.adaptivemmd = {
serviceConfig = {
Type = "forking";
EnvironmentFile = "${cfg.package}/etc/sysconfig/adaptivemmd";

@cmm
Copy link
Member Author

cmm commented Oct 21, 2023

@h7x4 I packaged adaptivemm speculatively, hoping to solve some problem I don't even remember specifics of at this point (it was a kernel bug in the end, fixed since). should've just abandoned this PR but forgot, sorry!
(so if you are actually interested in packaging this, it's all yours as far as I'm concerned)

@h7x4 h7x4 marked this pull request as draft January 17, 2024 22:53
@@ -0,0 +1,86 @@
{ config, lib, pkgs, ... }:

with lib;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
with lib;

to address #208242 for this module, uses of with such as this should be avoided.
consider inherits where you find yourself using lib.foo excessively.
this nix.dev page on best practices with with may be helpful

in {
options = {
services.adaptivemm = {
enable = mkEnableOption (lib.mdDoc "Proactively tune kernel free memory configuration");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
enable = mkEnableOption (lib.mdDoc "Proactively tune kernel free memory configuration");
enable = mkEnableOption "Proactively tune kernel free memory configuration";

lib.mdDoc is now just an alias and can be safely removed everywhere.
see d36f950 and #237557

@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict This PR has merge conflicts with the target branch labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (new) This PR adds a module in `nixos/` 8.has: package (new) This PR adds a new package 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.

5 participants