dmenu-rs: add package #200977

benjaminedwardwebb · 2022-11-13T03:17:36Z

dmenu-rs is a pixel perfect port of dmenu, rewritten in Rust with extensive plugin support.

See: https://github.com/Shizcow/dmenu-rs

Description of changes

Things done

benjaminedwardwebb · 2022-11-20T21:57:33Z

Ran nix-env -iA dmenu-rs -f . on the nixos-unstable branch at 52b2ac8ae18bbad4374ff0dd5aeee0fdf1aea739 in my local nixpkgs repository on my x86_64-linux laptop. Installed successfully and was usable.

benjaminedwardwebb · 2022-11-20T21:59:22Z

Also ran the above with nix.useSandbox = true;.

benjaminedwardwebb · 2022-11-20T22:16:05Z

Also, anecdotally, I've been using this (or a largely similar derivation) as my daily driver launcher for about a week and have observed no issues.

The overlay I use is here.

benjaminedwardwebb · 2022-11-20T22:47:40Z

pkgs/applications/misc/dmenu-rs/default.nix

+  # Tests require an X11 server and yields a cannot open display error. It's
+  # probably possible to get an X server running in the build but seems hard.
+  doCheck = false;
+  checkPhase = ''
+    make test
+  '';


While I'd like to enable this, the test triggered by make test invokes dmenu directly.

It runs

seq 1 100 | target/dmenu $(ARGS)

This not only requires an X11 server but it also hangs waiting for user input.

I was able to hack together the following checkPhase that works locally using timeout

timeout --preserve-status 5s xvfb-run make test

however simply hoping that 5 seconds is long enough for xvfb-run to spin up and the test to complete on every machine seems like an awful idea. I'm not sure how to kill the hanging call to dmenu, no option seems to allow that and I don't know how to feed in a keypress like <RETURN> into the xvfb-run context.

So I'm just leaving it disabled and relying on the other manual tests like installing, running the binary, and using it personally on a regular basis (at least for now).

benjaminedwardwebb

@ElvishJerricco @symphorien thanks for your help in working through some issues that stumped me while working on this derivation.

Of course not a priority, but if either of you have write-access and think the changes are worth merging, I'd appreciate getting this package out of a personal overlay and into nixpkgs where others can use it.

ElvishJerricco · 2022-11-20T23:03:46Z

pkgs/applications/misc/dmenu-rs/default.nix

+  '';
+
+  RUST_BACKTRACE = "full";
+  LIBCLANG_PATH = "${libclang.lib}/lib";


Why are these needed?

RUST_BACKTRACE is not needed, it's just a vestige of development that I've now removed.

LIBCLANG_PATH is required but I'm not well-versed enough in these C libraries to know why. Not including it yields this error

error: failed to run custom build command for `headers v0.1.0 (/build/source/src/headers)` Caused by: process didn't exit successfully: `/build/source/src/target/release/build/headers-ce9f3c3aabdd9ea3/build-script-build` (exit status: 101) --- stderr thread 'main' panicked at 'Unable to find libclang: "couldn't find any valid shared libraries matching: ['libclang.so', 'libclang-*.so', 'libclang.so.*', 'libclang-*.so.*'], set the `LIBCLANG_PATH` environment variable to a path where one of these files can be found (invalid: [])"', /build/cargo-vendor-dir/bindgen-0.53.3/src/lib.rs:1956:31 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace make: *** [makefile:35: dmenu] Error 101

So I followed the error's advice and looked at the dependencies to see where I could find the .so files mentioned -- I found them and set LIBCLANG_PATH to the place where they lived.

to provide clang and the right flags to bindgen, use rustPlaftorm.bindgenHook instead.

That worked! Thanks for the tip about rustPlatform.bindgenHook.

ElvishJerricco · 2022-11-20T23:04:26Z

pkgs/applications/misc/dmenu-rs/default.nix

+  # and make it writeable.
+  # See: https://github.com/rust-x-bindings/rust-xcb/tree/v0.8.2
+  patchPhase = ''
+    chmod +w /build/cargo-vendor-dir


Do not use /build. It is different depending on the machine. Use $NIX_BUILD_TOP.

ElvishJerricco · 2022-11-20T23:11:56Z

pkgs/applications/misc/dmenu-rs/default.nix

+  buildPhase = ''
+    make
+  '';
+
+  installPhase = ''
+    make install
+  '';


I thought these looked fishy at first, but it actually does make sense. We want the rust platform stuff because it does the vendoring and all that, but it doesn't just do make and make install like stdenv.mkDerivation does by default.

That said, this is very much not going to cross compile correctly. Not critical, but unfortunate, and probably fixable.

I've added a comment that tries to explain these phases' surprising appearance.

I'll be honest, I don't know the exact consequences of this package not cross compiling correctly, but it sure sounds bad. Is it related to not running ./configure in the configurePhase? Should I go read up on Autoconf and figure out how to add that into this derivation?

I wouldn't worry about it not cross compiling; frankly I don't think you can cross compile a desktop environment with nixpkgs right now anyway. It was just something I found noteworthy. But basically the package's makefile is hostile to cross compilation because e.g. it hardcodes CC=cc and doesn't have a way to choose the cargo target. It would need (relatively trivial) changes to their build scripting to fix this.

Thanks for calling it out. I'm not really a contributor to dmenu-rs (I fixed 1 typo) but it's something I'll keep in mind and who knows maybe I can fix it upstream some time in the future.

did you consider using mkDerivation with cargoSetupHook to handle fetching sources?

to quote the manual:

The rustPlatform.buildRustPackage function is split into several hooks: cargoSetupHook to set up vendoring for Cargo-based projects, cargoBuildHook to build a project using Cargo, cargoInstallHook to install a project using Cargo, and cargoCheckHook to run tests in Cargo-based projects. With this change, mixed-language projects can use the relevant hooks within builders other than buildRustPackage. However, these changes also required several API changes to buildRustPackage itself:

I didn't. That sounds like a more natural way to write this derivation -- as a standard make-based build that happens to include cargo deps. I'll take a stab at rewriting it that way later today. Thanks for this!

ElvishJerricco

I'm a little unhappy with the LIBCLANG_PATH thing; I don't really understand why this package is so picky about using clang and having this lib in this particular environment variable. But it's probably not worth blocking this PR over given that it's probably due to the package's excessively wonky build system.

symphorien · 2022-11-22T19:46:33Z

pkgs/applications/misc/dmenu-rs/default.nix

+    cp -r "$NIX_BUILD_TOP/cargo-vendor-dir/xcb-0.8.2/." "$NIX_BUILD_TOP/cargo-vendor-dir/xcb-0.8.2-readwrite"
+    unlink "$NIX_BUILD_TOP/cargo-vendor-dir/xcb-0.8.2"
+    mv "$NIX_BUILD_TOP/cargo-vendor-dir/xcb-0.8.2-readwrite" "$NIX_BUILD_TOP/cargo-vendor-dir/xcb-0.8.2"
+    chmod -R +w "$NIX_BUILD_TOP/cargo-vendor-dir/xcb-0.8.2/"


why do you need the copy step, and not only the recursive chmod?

.../cargo-vendor-dir/xcb-0.8.2/ is a symlink to the nix store. I don't think I can chmod the nix store inside of the build and in general making anything in the nix store writeable seems conceptually wrong?

oh it's originally a symlink, I thought it was a copy. My bad.

you can remove the final chmod -R by using cp --no-preserve=mod then

symphorien · 2022-11-22T19:47:26Z

pkgs/applications/misc/dmenu-rs/default.nix

+  };
+
+  nativeBuildInputs = [
+    clang


with bindgenHook you presumably need neither libclang, llvm nor clang.

I was able to drop llvm and libclang, but it wouldn't build without keeping clang.

please drop clang and modify https://github.com/Shizcow/dmenu-rs/blob/master/config.mk#L14 as suggested by the error message

Are you suggesting I modify this upstream at Shizcow/dmenu-rs or through a patch in the nix derivation?

downstream is ok (either as a patch or as substituteInplace config.mk --replace 'clang' '$(CC)' in postPatch for example)
But hard coding clang as compiler is not distro-friendly in general so you may submit this change upstream if so you wish, after we merge this PR. Using $(CC) is more portable and preferable.

I've added it as a patch here, but also opened this PR upstream.

I think you forgot to remove the dependency on clang. Thank you for reaching upstream :)

Oh you're right, woops

pkgs/applications/misc/dmenu-rs/default.nix

maintainers/maintainer-list.nix

pkgs/applications/misc/dmenu-rs/default.nix

AndersonTorres · 2022-12-08T15:00:20Z

pkgs/applications/misc/dmenu-rs/default.nix

+  # As suggested in the nixpkgs manual section on building rust packages, the
+  # derivation's Cargo.lock file is copied into the source.


Suggested change

# As suggested in the nixpkgs manual section on building rust packages, the

# derivation's Cargo.lock file is copied into the source.

Mentioning the manual is not needed.

AndersonTorres · 2022-12-08T15:01:07Z

pkgs/applications/misc/dmenu-rs/default.nix

+    cp ${./Cargo.lock} src/Cargo.lock
+  '';
+
+  # The cargo project does not live at the repository's top-level directory.


Too obvious to be commented.

Suggested change

# The cargo project does not live at the repository's top-level directory.

Thanks for keeping things concise! :)

While adding dmenu-rs as a package to my distro, some reviewers suggested the hardcoded clang be dropped upstream in favor of using the system default. See: NixOS/nixpkgs#200977

AndersonTorres · 2022-12-15T00:21:03Z

pkgs/applications/misc/dmenu-rs/default.nix

+
+  cargoRoot = "src";
+
+  preConfigure = ''


Some more corners to trim

this should be a postPatch

It is better to remove this setting from config.mk: sed -i "s@PREFIX = /usr/local@@g" config.mk

Way better: remove this preConfigure completely, and use installFlags = [ "PREFIX=$(out)" ]; instead.

I actually was trying to go with something like 3 initially, but then decided to just copy what the vanilla C-based dmenu was doing in its build: https://github.com/NixOS/nixpkgs/blob/d0a1a0f243138978b1d258056eb5988787da7aac/pkgs/applications/misc/dmenu/default.nix#L21-L23

But I agree, installFlags seems more sensible. Let me see if I can get it working.

Cool, updated to use installFlags. I think the first time I was trying to use makeFlags and couldn't figure out why it wasn't working -- thanks for suggesting the right way.

symphorien · 2022-12-15T19:48:43Z

pkgs/applications/misc/dmenu-rs/default.nix

+    runHook postPatch
+  '';
+
+  postPatch = ''


there is no reason to have both postPatch and a patchPhase. As the default patchPhase is still useful if someone downstream wants to add a patch with overrideAttrs, it is probably better to move the content of patchPhase (except runHook) to postPatch.

OK. I've moved everything over into postPatch.

But, any guidance on what is a good definition of a patch is / what should generally go in patchPhase?

Should it only be applying .patch files? Or any change to the project's source code? Any change to the project's build? Any change to the project's dependencies' source code or build?

I guess what this patchPhase was doing was

changing the project's build code (config.mk and copying in Cargo.lock)

subtly changing a vendored dependency so that it can be written to, without actually altering any of that dependency's source code or build code

So if we drew the line at "patches are things that alter a project's source code (but not necessarily build code)" then it seems like it makes sense to keep it all out of patchPhase.

Probably there's no hard rule but just trying to get a feel for what is what in nixpkgs.

But, any guidance on what is a good definition of a patch is / what should generally go in patchPhase?

Should it only be applying .patch files? Or any change to the project's source code? Any change to the project's build? Any change to the project's dependencies' source code or build?

Typically, many distros (if not all!) provide .patch files in order to directly modify the source code of the apps they provide.

A typical case is when a new compiler release does not compile some old code (indeed it happened to me when I tried to compile qemu in Slackware a long time ago...). So, usually a package manager provides patch files for cases like these.
Indeed, many times those patchsets are generated by merely diffing the master git branch with the current stable git branch, maybe because some people like ultra fresh, pre-alpha code...

Another thing that such package managers can do too is run sanitization scripts that clean up some quirks on the original source code; things like removing annoying lines from a Makefile or fixing hardcoded locations (e.g. FreeBSD ports system installs things on /usr/local/ instead of /usr/; so maybe some tool that expects to find things on /usr will not get it without help).

It gets a bit complicated when both things need to be done: .patch files to modify C source code, plus sanitization scripts that cleanup Makefiles.

Now, switch to Nix(pkgs).

In Nixpkgs, we provide both use cases via the same patchPhase. However, as I said above, it is a bit complicated...

The typical, unedited patchPhase usually works like this:

patchPhase = '' runHook prePatch patch -p1 < <the .patch files provided in patches =[ ... ];> runHook postPatch ''

The runHooks are there because of composability:

Nix has the ability of compose overlays of package trees, in a similar manner when you add third party repositories in other package managers (/etc/apt/sources.lst anyone?).
Such pre and post hooks allow the creator of an overlay to easily modify the phases without having to rewrite them completely.

Well, when you customize this phase as above, it would look like

patchPhase = '' runHook prePatch <a giant script modifying the source code> runHook postPatch ''

However, by doing this, you are basically ignoring any .patch files eventually provided in patches list.

So, when you need to use such "sanitization scripts", it is better to use postPatch instead of rewriting the whole patchPhase.

Probably there's no hard rule but just trying to get a feel for what is what in nixpkgs.

Sometimes many things are developed by trial and error.

Ah thank you for all that good background knowledge!

symphorien · 2022-12-16T20:45:41Z

Looks good now! Thank you for bearing with us. One last thing before I merge: this does not build on aarch64 (see https://github.com/NixOS/nixpkgs/pull/200977/checks?check_run_id=10128726771 ). It looks due to upstream but unintended, so please mark it as broken like here

https://github.com/NixOS/nixpkgs/blob/master/pkgs/applications/audio/eq10q/default.nix#L36

You can optionally report it upstream, and if they answer that aarch64 support is not in scope for the project, remove the broken attribute and restrict platforms to x86-64-linux only instead.

dmenu-rs is a pixel perfect port of dmenu, rewritten in Rust with extensive plugin support. See: https://github.com/Shizcow/dmenu-rs

benjaminedwardwebb · 2022-12-20T05:09:13Z

Looks good now! Thank you for bearing with us. One last thing before I merge: this does not build on aarch64 (see https://github.com/NixOS/nixpkgs/pull/200977/checks?check_run_id=10128726771 ). It looks due to upstream but unintended, so please mark it as broken like here

https://github.com/NixOS/nixpkgs/blob/master/pkgs/applications/audio/eq10q/default.nix#L36

You can optionally report it upstream, and if they answer that aarch64 support is not in scope for the project, remove the broken attribute and restrict platforms to x86-64-linux only instead.

@symphorien added aarch64-linux as broken. However, I use an aarch64-linux machine pretty regularly, so am interested in adding support upstream if possible (and filed this issue related to it). I don't know how easy this will be, though, so maybe it will have to wait for another PR to nixpkgs.

@AndersonTorres Anything holding up an approval?

benjaminedwardwebb · 2022-12-21T18:12:21Z

Thanks all for the help reviewing and getting this merged! Adding my first package to nixpkgs was a very pleasant experience.

While adding dmenu-rs as a package to my distro, some reviewers suggested the hardcoded clang be dropped upstream in favor of using the system default. See: NixOS/nixpkgs#200977

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Nov 13, 2022

benjaminedwardwebb force-pushed the dmenu-rs branch from ab727ba to e3699ff Compare November 13, 2022 03:51

benjaminedwardwebb force-pushed the dmenu-rs branch 3 times, most recently from c9e9d92 to 2d3abfb Compare November 20, 2022 04:19

benjaminedwardwebb force-pushed the dmenu-rs branch from 2d3abfb to 9962c44 Compare November 20, 2022 21:49

benjaminedwardwebb force-pushed the dmenu-rs branch from 9962c44 to fd34f69 Compare November 20, 2022 22:00

benjaminedwardwebb marked this pull request as ready for review November 20, 2022 22:00

benjaminedwardwebb force-pushed the dmenu-rs branch from fd34f69 to 546d8a0 Compare November 20, 2022 22:21

benjaminedwardwebb commented Nov 20, 2022

View reviewed changes

ElvishJerricco requested changes Nov 20, 2022

View reviewed changes

benjaminedwardwebb force-pushed the dmenu-rs branch 2 times, most recently from feee0f4 to abfee93 Compare November 21, 2022 02:30

ElvishJerricco approved these changes Nov 21, 2022

View reviewed changes

symphorien reviewed Nov 22, 2022

View reviewed changes

pkgs/applications/misc/dmenu-rs/default.nix Outdated Show resolved Hide resolved

benjaminedwardwebb force-pushed the dmenu-rs branch from abfee93 to 6aa1f88 Compare November 24, 2022 06:44

symphorien reviewed Nov 26, 2022

View reviewed changes

pkgs/applications/misc/dmenu-rs/default.nix Outdated Show resolved Hide resolved

benjaminedwardwebb force-pushed the dmenu-rs branch from 6aa1f88 to 253e7dc Compare December 1, 2022 00:52

AndersonTorres suggested changes Dec 7, 2022

View reviewed changes

maintainers/maintainer-list.nix Show resolved Hide resolved

pkgs/applications/misc/dmenu-rs/default.nix Show resolved Hide resolved

AndersonTorres suggested changes Dec 8, 2022

View reviewed changes

make me a nixpkgs maintainer

e065a54

benjaminedwardwebb force-pushed the dmenu-rs branch 4 times, most recently from cc5e910 to d0a1a0f Compare December 14, 2022 04:59

benjaminedwardwebb mentioned this pull request Dec 14, 2022

use default c compiler instead of hardcoded clang Shizcow/dmenu-rs#44

Merged

AndersonTorres suggested changes Dec 15, 2022

View reviewed changes

benjaminedwardwebb force-pushed the dmenu-rs branch from d0a1a0f to 188b11c Compare December 15, 2022 02:42

symphorien reviewed Dec 15, 2022

View reviewed changes

benjaminedwardwebb force-pushed the dmenu-rs branch 2 times, most recently from 2a3c898 to fb3ac05 Compare December 15, 2022 22:57

symphorien approved these changes Dec 16, 2022

View reviewed changes

dmenu-rs: init at 5.5.1

84edcb0

dmenu-rs is a pixel perfect port of dmenu, rewritten in Rust with extensive plugin support. See: https://github.com/Shizcow/dmenu-rs

benjaminedwardwebb force-pushed the dmenu-rs branch from fb3ac05 to 84edcb0 Compare December 20, 2022 04:24

AndersonTorres approved these changes Dec 20, 2022

View reviewed changes

AndersonTorres merged commit 60d4720 into NixOS:master Dec 20, 2022

benjaminedwardwebb deleted the dmenu-rs branch December 20, 2022 06:06

Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023

		# As suggested in the nixpkgs manual section on building rust packages, the
		# derivation's Cargo.lock file is copied into the source.

dmenu-rs: add package #200977

dmenu-rs: add package #200977

Conversation

benjaminedwardwebb commented Nov 13, 2022 • edited Loading

Description of changes

Things done

benjaminedwardwebb commented Nov 20, 2022 • edited Loading

benjaminedwardwebb commented Nov 20, 2022

benjaminedwardwebb commented Nov 20, 2022

benjaminedwardwebb Nov 20, 2022 • edited Loading

Choose a reason for hiding this comment

benjaminedwardwebb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

symphorien Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminedwardwebb Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvishJerricco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminedwardwebb Dec 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

symphorien commented Dec 16, 2022

benjaminedwardwebb commented Dec 20, 2022

benjaminedwardwebb commented Dec 21, 2022

benjaminedwardwebb commented Nov 13, 2022 •

edited

Loading

benjaminedwardwebb commented Nov 20, 2022 •

edited

Loading

benjaminedwardwebb Nov 20, 2022 •

edited

Loading

symphorien Nov 22, 2022 •

edited

Loading

benjaminedwardwebb Nov 21, 2022 •

edited

Loading

benjaminedwardwebb Dec 15, 2022 •

edited

Loading