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

preload: init at 0.6.4 #141583

Closed
wants to merge 1 commit into from
Closed

Conversation

GustavoPeredo
Copy link

@GustavoPeredo GustavoPeredo commented Oct 13, 2021

Motivation for this change

I'm loving nixos! The only problem is that I've noticed some apps take longer to load, that's why I'm packaging preload, so programs can load faster.

NOTES:
1- This is my first package... I'm trying to learn here, so feel free to criticize as much as it is needed!
2- There are some, simingly, impossible to solve problems:
a) Upstream: The preload package can't create /var/preload/* and /var/log/preload.log, paths and files must be created/touched first (manually)
b) Indexing: I think indexing the entire nixstore would be too much for preload, it would be cool to be able to be able to get the paths of certain packages at build time, but I don't think this is possible. I'm currently indexing my home folder for my flatpaks, seem to be working well.
c) Systemd: preload comes with a service file for systemd, should I package it as a module as well then? Similar to gamemode
3- Where can I find more testing documentation? I'm having trouble finding resources

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.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.

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/misc/preload/default.nix Outdated Show resolved Hide resolved
@Artturin
Copy link
Member

Artturin commented Oct 13, 2021

Please follow https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md

the exeprefix and mapprefix will most likely need some changing in the module

ls /var/run/current-system/sw
bin  etc  lib  sbin  share

@GustavoPeredo
Copy link
Author

Thanks, I'll be working on it

@lucasew
Copy link
Contributor

lucasew commented Oct 14, 2021

BTW there is a guideline about naming PRs and commits

You can rename a commit using git commit --amend or git rebase --interactive. There are lots of tutorials and stuff about this.

If you are unsure about if the PR should be reviewed now or its work in progress you can mark it as draft.

Things can take a lot of time until merge, I already sent PRs that took over a month until merge.

@Artturin
Copy link
Member

@GustavoPeredo
Copy link
Author

If you are unsure about if the PR should be reviewed now or its work in progress you can mark it as draft.

I'm pretty sure it is ready for a merge after I fix the code styling. Can you link me to the guidelines for commits? I couldn't find any...

https://git-rebase.io/#fixup

Thanks for the link! I tried following it, but I'm not sure it worked.

@Artturin
Copy link
Member

I'm pretty sure it is ready for a merge after I fix the code styling.

nixpkgs-fmt can be used to format the files

Can you link me to the guidelines for commits? I couldn't find any...

https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md

@Artturin
Copy link
Member

Artturin commented Oct 15, 2021

all-packages.nix shouldn't be formatted with nixpkgs-fmt

@Artturin
Copy link
Member

Artturin commented Oct 15, 2021

i would recommend that you do git reset --hard 7d88e70cb69e1d6c7924c88d8612bfd1b0d5e8ad to go back to the first commit (--hard will discard the newer commits, dont use --hard if you want to keep the changes, they will be staged)

@GustavoPeredo
Copy link
Author

all-packages.nix shouldn't be formatted with nixpkgs-fmt

Yes, already reverted.

@lucasew
Copy link
Contributor

lucasew commented Nov 6, 2021 via email

@GustavoPeredo
Copy link
Author

What is the state of this package ? This is one of the most important packages for nixos ecosystem

I think it's a matter of waiting, as @lucasew mentioned:

Things can take a lot of time until merge

But I'm not sure

@Artturin Artturin self-requested a review March 11, 2022 19:57
@Artturin Artturin self-assigned this Mar 11, 2022
@Artturin
Copy link
Member

i will do some improvements and tests and force push the changes straight to your branch so remember to pull the changes correctly

@Artturin
Copy link
Member

What is the state of this package ? This is one of the most important packages for nixos ecosystem

Considering the preload program hasn't received an update since 2013 i hope its not one of the most important packages for any distro


Someone needs to test this on a user system instead of a vm https://nixos.wiki/wiki/Nixpkgs/Reviewing_changes#Modules

@bjornfor
Copy link
Contributor

I'm not sure exactly how preload works, but I feel this is relevant: NixOS/patchelf#357.

AFAIU, "shrinkwrapped" binaries will make the dynamic linker startup complexity O(n) instead of O(n^2). (And per-library lookup O(1) instead of O(n).)

@baronleonardo
Copy link
Contributor

@Artturin if you have a better alternative (for improving the startup of apps and is an active project) tell me, and we could happily dismiss this and use the alternative.

@GustavoPeredo
Copy link
Author

Someone needs to test this on a user system instead of a vm https://nixos.wiki/wiki/Nixpkgs/Reviewing_changes#Modules

I just installed it on a system of mine, seems to work just fine. What tests are needed?

Comment on lines +33 to +35
mapprefix = "/run/current-system/sw/share;/run/current-system/sw/lib;!/";
exeprefix = "!/run/current-system/sw/sbin/;/run/current-system/sw/bin;!/";
Copy link
Member

Choose a reason for hiding this comment

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

@GustavoPeredo are you sure it works fine? the things that i am most concerned about are these 2

Copy link
Author

@GustavoPeredo GustavoPeredo Mar 15, 2022

Choose a reason for hiding this comment

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

They are following expected behavior, but it would be more useful if the nix store was added to these paths.

@GustavoPeredo
Copy link
Author

There is now a problem with how the module writes the configuration file: It only adds modified values to the file (excludes defaults).

Does that mean the modules file has to be rewritten with each value as it's own mkOption? Like so:

settings.cycle = mkOption {
  type = ...;
  default = ...;
  description = ...;
};

@Artturin
Copy link
Member

There is now a problem with how the module writes the configuration file: It only adds modified values to the file (excludes defaults).

Does that mean the modules file has to be rewritten with each value as it's own mkOption? Like so:

settings.cycle = mkOption {
  type = ...;
  default = ...;
  description = ...;
};

I think we just need to add
apply = recursiveUpdate default;

doCheck = false;

meta = with lib; {
homepage = "http://sourceforge.net/projects/preload";
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
homepage = "http://sourceforge.net/projects/preload";
homepage = "https://sourceforge.net/projects/preload/";

Comment on lines +39 to +44

postPatch = ''
# "--logdir=/var/log/preload" failed with unknown option
substituteInPlace configure.ac \
--replace "logdir='\''${localstatedir}/log'" "logdir='\''${localstatedir}/log/preload'"
'';
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
postPatch = ''
# "--logdir=/var/log/preload" failed with unknown option
substituteInPlace configure.ac \
--replace "logdir='\''${localstatedir}/log'" "logdir='\''${localstatedir}/log/preload'"
'';

Copy link
Author

@GustavoPeredo GustavoPeredo Mar 24, 2022

Choose a reason for hiding this comment

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

Were these formatting changes tested? I want to commit them, but had no time to test.

Copy link
Member

Choose a reason for hiding this comment

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

No but I am highly confident that they just work. Nix does not care about the ordering but it is very good practice to use somewhat standard ordering.

src = fetchurl {
url = "mirror://sourceforge/${pname}/${pname}-${version}.tar.gz";
sha256 = "d0a558e83cb29a51d9d96736ef39f4b4e55e43a589ad1aec594a048ca22f816b";
};
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
};
};
postPatch = ''
# "--logdir=/var/log/preload" failed with unknown option
substituteInPlace configure.ac \
--replace "logdir='\''${localstatedir}/log'" "logdir='\''${localstatedir}/log/preload'"
'';

version = "0.6.4";

src = fetchurl {
url = "mirror://sourceforge/${pname}/${pname}-${version}.tar.gz";
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
url = "mirror://sourceforge/${pname}/${pname}-${version}.tar.gz";
url = "mirror://sourceforge/preload/preload-${version}.tar.gz";

Comment on lines +1 to +10
{
lib,
stdenv,
fetchurl,
glib,
pkg-config,
help2man,
autoreconfHook,
}:
stdenv.mkDerivation rec {
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,
fetchurl,
glib,
pkg-config,
help2man,
autoreconfHook,
}:
stdenv.mkDerivation rec {
{ lib
, stdenv
, fetchurl
, glib
, pkg-config
, help2man
, autoreconfHook
}:
stdenv.mkDerivation rec {

Comment on lines +1 to +11
{
config,
lib,
pkgs,
...
}:
with lib; let
cfg = config.programs.preload;
settingsFormat = pkgs.formats.ini {};
configFile = settingsFormat.generate "preload.conf" cfg.settings;
in {
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
{
config,
lib,
pkgs,
...
}:
with lib; let
cfg = config.programs.preload;
settingsFormat = pkgs.formats.ini {};
configFile = settingsFormat.generate "preload.conf" cfg.settings;
in {
{ config
, lib
, pkgs
, ...
}:
with lib;
let
cfg = config.programs.preload;
settingsFormat = pkgs.formats.ini {};
configFile = settingsFormat.generate "preload.conf" cfg.settings;
in {

type = types.submodule {
freeformType = settingsFormat.type;
};
apply = recursiveUpdate default;
Copy link
Contributor

Choose a reason for hiding this comment

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

default can't be referenced here. Just moving it to the let block works.

Also settings.default should be {} to allow just using the default config.

@AndersonTorres
Copy link
Member

Closing as dead and with conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants