brltty: 6.1 -> 6.3; nixos/brltty: use upstream units #114745

rnhmjoj · 2021-03-01T12:21:11Z

Motivation for this change

Removing systemd-udev-settle as part of #73095.

Upstream has been providing a very thoroughly designed set of systemd units,
udev and polkit rules. With these the brltty daemon is activated
asynchronously via udev, runs as a dedicated user with runtime and state
directories set up using systemd-tmpfiles.

This is much better than the current unit, which runs a single instance
as root and pulls in systemd-udev-settle to wait for the hardware.

Things done

Find someone with a braille display to test this
Build brltty
Build a system with services.brltty.enable = true;
Ensured that relevant documentation is up to date
Fits CONTRIBUTING.md.

r-rmcgibbo · 2021-03-01T12:43:00Z

Result of nixpkgs-review pr 114745 at 07b17a4f run on aarch64-linux 1

3 packages built:

Result of nixpkgs-review pr 114745 at 07b17a4f run on x86_64-linux 1

3 packages built:

pkgs/tools/misc/brltty/default.nix

SuperSandro2000 · 2021-03-01T14:09:53Z

pkgs/tools/misc/brltty/default.nix

-    homepage = "http://www.brltty.com/";
-    license = lib.licenses.gpl2;
+    homepage = "https://brltty.app";
+    license = lib.licenses.gpl2Plus;
    maintainers = [ lib.maintainers.bramd ];


We could shorten this by using meta = with lib; {

SuperSandro2000 · 2021-03-01T14:18:44Z

This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).
If you have any questions or problems please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 114745 run on x86_64-linux 1

3 packages built:

brltty
orca (gnome3.orca)
pantheon.elementary-session-settings

The following issues got detected with the above build packages.
Please fix at least the ones listed with your changed packages:

brltty:

Please consider this feature to be alpha.

A substituteInPlace with an unmatched pattern got detected:

substituteStream(): WARNING: pattern 'install-writable-directory' doesn't match anything in file 'Programs/Makefile.in'
substituteStream(): WARNING: pattern '/usr/lib' doesn't match anything in file 'systemd/system/[email protected]'

Please check the offending substituteInPlace for typos or changes in source.

warning: stale-substitute
Stale substituteInPlace detected.
substituteStream(): WARNING: pattern 'install-writable-directory' doesn't match anything in file 'Programs/Makefile.in'
See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/stale-substitute.md
warning: stale-substitute
Stale substituteInPlace detected.
substituteStream(): WARNING: pattern '/usr/lib' doesn't match anything in file 'systemd/system/[email protected]'
See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/stale-substitute.md

pantheon.elementary-session-settings:

warning: unclear-gpl
lgpl3 is a deprecated license, check if project uses lgpl3Plus or lgpl3Only and change meta.license accordingly.

Near pkgs/desktops/pantheon/desktop/elementary-session-settings/default.nix:158:5:

    |
158 |     license = licenses.lgpl3;
    |     ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/unclear-gpl.md
warning: unused-argument
Unused argument: substituteAll.
Near pkgs/desktops/pantheon/desktop/elementary-session-settings/default.nix:4:3:

  |
4 | , substituteAll
  |   ^

Assertions can break overriding a package, see issue NixOS#73102.

bramd · 2021-03-12T12:49:03Z

@rnhmjoj Thanks for your work. I'm not using NixOS much these days so wasn't aware that upstream has very good systemd units by now.

I just checked this with a braille display. The display does not get activated. Which journalctl or systemctl ouput should I check to debug this?

rnhmjoj · 2021-03-12T14:50:59Z

@bramd Hi, I'm glad you're still using NixOS, even if not much. I would be really stuck without you, thank you.

Uhm, it's possible the units templates don't play well with NixOS. Normally they are broken because they can't be installed declaratively, but here they are being started by udev, so I thought it could still work. Or maybe I missed some file that had to be installed manually.

The starting process is actually pretty involved, it should work like this:

an udev rule should fire when the display is detected (I have no idea what its name might be, let's call it /dev/ttyS0)
systemd should react by creating and starting the dev-ttyS0.device and [email protected] units
systemd will then start [email protected], which is the actual service (because it's required by [email protected])

So, first I would try to confirm that the udev rule worked: you should check if your device is listed in the output of

systemctl list-units --type device

Then, systemctl status should show if any units failed to start. A journal log of each of the brltty-* unit would help.
Lastly, the [email protected] unit is asking for these paths to exist before starting:

/run/brltty
/var/lib/brltty
/var/lib/BrlAPI

can you check if they are created?

bramd · 2021-03-13T22:26:30Z

@rnhmjoj OK, first of all the device is showing:

[root@nixos:~]# systemctl status dev-ttyUSB0.device
● dev-ttyUSB0.device - USB1.1 UHCI Controller
   Follow: unit currently follows state of sys-devices-pci0000:00-0000:00:11.0-0000:02:00.0-usb1-1\x2d2-1\x2d2.1-1\x2d2.1:1.0-ttyUSB0-tty-ttyUSB0.device
     Loaded: loaded
     Active: active (plugged) since Sat 2021-03-13 22:09:03 CET; 34min ago
     Device: /sys/devices/pci0000:00/0000:00:11.0/0000:02:00.0/usb1/1-2/1-2.1/1-2.1:1.0/ttyUSB0/tty/ttyUSB0

The brltty-device@ttyUSB0 unit is present, but doesn't seem to get activated:

[root@nixos:~]# systemctl status brltty-device@dev-ttyUSB0
● [email protected] - Braille Device: dev/ttyUSB0
     Loaded: loaded (/nix/store/i6m6ik31fpc3qap5aln9zq9pabwid80c-brltty-6.3/lib/systemd/system/[email protected]; static)
     Active: inactive (dead)
       Docs: man:brltty(1)
             http://brltty.app/

Any ideas what to try next?

rnhmjoj · 2021-03-14T09:57:06Z

That's strange: the creation of .device and activation of the brltty-device@ unit are done by the same rule, not sure how only one would fail.

What does this outputs?

systemctl list-dependencies --reverse dev-ttyUSB0.device

also, this could be useful:

systemctl list-dependencies brltty-device@dev-ttyUSB0

bramd · 2021-03-14T17:06:04Z

Now this is weird, no reverse dependencies on the device, but it is in the dependency list of the brltty service:

[root@nixos:~]# systemctl list-dependencies --reverse dev-ttyUSB0.device
dev-ttyUSB0.device
[root@nixos:~]# systemctl list-dependencies brltty-device@dev-ttyUSB0
[email protected]
● ├─[email protected]
● ├─dev-ttyUSB0.device
● ├─system-brltty\x2ddevice.slice
● └─sysinit.target
●   ├─dev-hugepages.mount
●   ├─dev-mqueue.mount
●   ├─firewall.service
●   ├─kmod-static-nodes.service
●   ├─sys-fs-fuse-connections.mount
●   ├─sys-kernel-config.mount
●   ├─sys-kernel-debug.mount
●   ├─systemd-ask-password-console.path
●   ├─systemd-journal-catalog-update.service
●   ├─systemd-journal-flush.service
●   ├─systemd-journald.service
●   ├─systemd-modules-load.service
●   ├─systemd-random-seed.service
●   ├─systemd-sysctl.service
●   ├─systemd-timesyncd.service
●   ├─systemd-tmpfiles-setup.service
●   ├─systemd-udev-trigger.service
●   ├─systemd-udevd.service
●   ├─systemd-update-done.service
●   ├─systemd-update-utmp.service
●   ├─cryptsetup.target
●   ├─local-fs.target
●   │ ├─-.mount
●   │ ├─systemd-fsck-root.service
●   │ └─systemd-remount-fs.service
●   └─swap.target
●     └─var-swap.swap

rnhmjoj · 2021-03-14T17:25:14Z

Is there anything in the journal for [email protected]?

bramd · 2021-03-14T18:17:01Z

@rnhmjoj No, it's completely empty

rnhmjoj · 2021-03-15T10:33:47Z

Can you run systemctl show dev-ttyUSB0.device? There should be a Wants= line that may be interesting.

rnhmjoj · 2021-03-15T12:10:03Z

I tried to reproduce the setup with a couple of fake units in a VM and the trick brltty is using seems to work: the final service is started at boot.

A final option would be to run systemd in debug mode and check the logs.
You can do that by adding boot.kernelParams = [ "systemd.log_level=debug" ]; to the configuration.nix.

bramd · 2021-04-02T12:16:47Z

@rnhmjoj I had some time to investigate this further.

It seems some generic USB device identifiers are commented in the udev rules and the braille display I used to test this uses one of those identifiers. So, therefore it didn't trigger. I added the following sed command to postInstall to uncomment those:

sed '/^# ENV/s/^# //' -i udev/rules.d/90-brltty-device.rules

This is interesting, since just starting BRLTTY with the driver set to "auto" will detect these displays with a generic identifier. So, for users running my old version of this package without any additional configuration it may mean that they lose braille when upgrading. I'm not sure how we should handle this.

Another issue is that the SupplementaryGroups for the brltty units contains pulse-access, a group that is not present on my NixOS system. This causes systemd to fail and not start the service. I just manually added this group for now and now I get the following output:

Apr 02 14:13:26 nixos systemd[1]: Starting BRLTTY Instance: /sys/devices/pci0000:00/0000:00:11.0/0000:02:00.0/usb2/2-2/2-2.1...
Apr 02 14:13:26 nixos systemd[1]: brltty@-sys-devices-pci0000:00-0000:00:11.0-0000:02:00.0-usb2-2\x2d2-2\x2d2.1.service: Control process exited, code=exited, status=1/FAILU
RE
Apr 02 14:13:26 nixos systemd[1]: brltty@-sys-devices-pci0000:00-0000:00:11.0-0000:02:00.0-usb2-2\x2d2-2\x2d2.1.service: Failed with result 'exit-code'.
Apr 02 14:13:26 nixos systemd[1]: Failed to start BRLTTY Instance: /sys/devices/pci0000:00/0000:00:11.0/0000:02:00.0/usb2/2-2/2-2.1.

So, now it seems it is trying to start, but failing somehow.

bramd · 2021-04-02T12:44:17Z

I turned on debug mode for systemd and found that this problem came from this script:

[root@nixos:~]# cat /nix/store/bdb1qh3rsbsih5dw4x9mqyizmkb0yb2z-unit-script-brltty_-pre-start/bin/brltty_-pre-start
#!/nix/store/f7jzmxq9bpbxsg69cszx56mw14n115n5-bash-4.4-p23/bin/bash -e
if ! test -f /etc/brlapi.key; then
  /nix/store/7wm2z95f6c1dldpwr51w7kgmnwzb5qs3-brltty-6.3/bin/brltty-genkey -f /etc/brlapi.key
fi

It tries to generate a brlapi key in /etc, which is only allowed by root it seems. After generating the key manually:

Apr 02 14:38:26 nixos systemd[1]: brltty@-sys-devices-pci0000:00-0000:00:11.0-0000:02:00.0-usb2-2\x2d2-2\x2d2.1.service: About to execute /nix/store/7wm2z95f6c1dldpwr51w7kg
mnwzb5qs3-brltty-6.3/libexec/brltty/systemd-wrapper
Apr 02 14:38:26 nixos systemd[1]: brltty@-sys-devices-pci0000:00-0000:00:11.0-0000:02:00.0-usb2-2\x2d2-2\x2d2.1.service: Forked /nix/store/7wm2z95f6c1dldpwr51w7kgmnwzb5qs3-
brltty-6.3/libexec/brltty/systemd-wrapper as 1197
Apr 02 14:38:26 nixos systemd[1]: brltty@-sys-devices-pci0000:00-0000:00:11.0-0000:02:00.0-usb2-2\x2d2-2\x2d2.1.service: Can't open PID file /run/brltty/brltty--sys-devices
-pci0000:00-0000:00:11.0-0000:02:00.0-usb2-2\x2d2-2\x2d2.1.pid (yet?) after start: Operation not permitted
Apr 02 14:38:26 nixos systemd[1]: brltty@-sys-devices-pci0000:00-0000:00:11.0-0000:02:00.0-usb2-2\x2d2-2\x2d2.1.service: Main process exited, code=exited, status=127/n/a
Apr 02 14:38:26 nixos systemd[1]: brltty@-sys-devices-pci0000:00-0000:00:11.0-0000:02:00.0-usb2-2\x2d2-2\x2d2.1.service: Failed with result 'exit-code'.

rnhmjoj · 2021-04-02T23:46:54Z

It tries to generate a brlapi key in /etc, which is only allowed by root it
seems. After generating the key manually:

Thank you, I should have fixed this.

Another issue is that the SupplementaryGroups for the brltty units contains
pulse-access

This is a group to control access to a system wide pulseaudio server, which is
not the usual way to run pulseaudio (it's actually not recommended). In NixOS
this doesn't seem to work currently: #114399. Anyway, I don't think pulseaudio
is a requirement (and system wide even more so) and this shouldn't be an error:
this should be reported upstream.

It seems some generic USB device identifiers are commented in the udev rules

We should probably ask about this too.

bramd · 2021-04-03T21:05:25Z

Another issue is that the SupplementaryGroups for the brltty units contains
pulse-access

This is a group to control access to a system wide pulseaudio server, which is
not the usual way to run pulseaudio (it's actually not recommended). In NixOS
this doesn't seem to work currently: #114399. Anyway, I don't think pulseaudio
is a requirement (and system wide even more so) and this shouldn't be an error:
this should be reported upstream.

Based on BRLTTY's docs and mailing list archive, the idea here is that a user might want to run a system-wide Pulseaudio daemon to have seamless BRLTTY speech/audio output in a real text console as well as in a graphical environment. BRLTTY itself does not interface directly to Pulseaudio and will just output to ALSA, which Pulse emulates. Since this is not really recommended/supported, this would be a custom setup by the user anyway. You might argue that NixOS should create this group if the systemwide option for Pulse is enabled, which is not the behavior now. If that was the case, we could conditionally enable this additional group membership. For now I commented it out.

It seems some generic USB device identifiers are commented in the udev rules

We should probably ask about this too.

Based on commit history and old mailing list posts the idea here is that the default option is very safe to just drop in a system. So, it will not mess with any other devices that match these generic identifiers, such as the FTDI usb-serial converters. Some distributions include this in their base installation, so most braille displays can work out of the box when plugged in. I think it is reasonable to be a bit more unsafe and include these braille devices, since the user asked to enable braille support explicitly. I changed the make options to do this. If we ever want to include this in NixOS live CDs/base installations, which would ease the installation dramatically for braille display users, we might want to use the safe version without the generic identifiers and add an option to switch between these versions.

Speaking of options, would it be a good idea to rename the enabled option? We are not really just enabling/disabling a service anymore. Shouldn't it be something like hardware.braille.enable?

I did not change the option naming, but sent a PR to you containing the other changes.

- Use absolute paths to logger and udev. - Pass the absolute path to the brltty binary to the wrapper. - Include generic USB devices in udev rules. - Remove pulse-access group from systemd unit and sysusers.

Upstream has been providing a very thoroughly designed set of systemd units, udev and polkit rules. With these the brltty daemon is activated asynchronously via udev, runs as a dedicated user with runtime and state directories set up using systemd-tmpfiles. This is much better than the current unit, which runs a single instance as root and pulls in systemd-udev-settle to wait for the hardware.

rnhmjoj · 2021-04-11T09:33:33Z

I fixed an evaluation error found by ofBorg: utillinux has been renamed to util-linux.

rnhmjoj · 2021-05-07T14:12:03Z

Shouldn't it be something like hardware.braille.enable?

It's fine for me.

Is there anything else to fix? I'd like to merge this before the feature freeze for NixOS 21.05.

bramd · 2021-05-07T18:26:58Z

@rnhmjoj No, I've tried this with two different displays, even connecting them at the same time, which works great. So that's an improvement from the single service autodetecting a single display.

Doing a quick test with a GNOME desktop I couldn't get braille to work in the Orca screenreader, but that was already broken before. Now we have the brlapi key file in place it at least has a chance of working for other clients that want to connect to the BRLTTY API. My Linux work is usually server stuff, so getting braille to work on the graphical desktop has not that much priority for me. Besides, that would probably need more changes across various packages to play well together.

So, long story short, I think it's fine to merge this either with the name of the option changed as discussed above or not. I'm not that familiar with renaming/breaking options in NixOS.

rnhmjoj · 2021-05-07T21:35:50Z

@rnhmjoj No, I've tried this with two different displays, even connecting them at the same time, which works great. So that's an improvement from the single service autodetecting a single display.

Ok, great to her that!

So, long story short, I think it's fine to merge this either with the name of the option changed as discussed above or not. I'm not that familiar with renaming/breaking options in NixOS.

It can be done without breaking compatibility via a sort of alias, though I'd just keep it the way it is for now.

Thank you again for helping out!

rnhmjoj added 3 commits March 1, 2021 08:57

brltty: 6.1 -> 6.3

6bfdb6b

brltty: fix ambiguous license in meta

ba27976

brltty: install systemd,udev,polkit files

6d1d1fc

rnhmjoj requested review from joachifm, bramd and oxij March 1, 2021 12:21

ofborg 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/` 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 labels Mar 1, 2021

SuperSandro2000 changed the title ~~nixos/brltty: use upstream units~~ brltty: 6.1 -> 6.3; nixos/brltty: use upstream units Mar 1, 2021

SuperSandro2000 reviewed Mar 1, 2021

View reviewed changes

brltty: remove asserts for optional features

6fc8d59

Assertions can break overriding a package, see issue NixOS#73102.

rnhmjoj force-pushed the brltty branch from 07b17a4 to d0e7b0a Compare March 1, 2021 17:09

rnhmjoj force-pushed the brltty branch from d0e7b0a to 2d4fcd8 Compare April 2, 2021 22:40

bramd mentioned this pull request Apr 3, 2021

Brltty rnhmjoj/nixpkgs#1

Merged

bramd and others added 2 commits April 11, 2021 10:35

brltty: fix systemd-wrapper script

1337526

- Use absolute paths to logger and udev. - Pass the absolute path to the brltty binary to the wrapper. - Include generic USB devices in udev rules. - Remove pulse-access group from systemd unit and sysusers.

rnhmjoj force-pushed the brltty branch from 3726ba2 to da2923b Compare April 11, 2021 08:35

bramd approved these changes May 7, 2021

View reviewed changes

rnhmjoj merged commit 4e4869b into NixOS:master May 7, 2021

rnhmjoj deleted the brltty branch July 10, 2023 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

brltty: 6.1 -> 6.3; nixos/brltty: use upstream units #114745

brltty: 6.1 -> 6.3; nixos/brltty: use upstream units #114745

rnhmjoj commented Mar 1, 2021 •

edited

Loading

r-rmcgibbo commented Mar 1, 2021 •

edited

Loading

SuperSandro2000 Mar 1, 2021

SuperSandro2000 commented Mar 1, 2021

bramd commented Mar 12, 2021

rnhmjoj commented Mar 12, 2021

bramd commented Mar 13, 2021

rnhmjoj commented Mar 14, 2021

bramd commented Mar 14, 2021

rnhmjoj commented Mar 14, 2021

bramd commented Mar 14, 2021

rnhmjoj commented Mar 15, 2021

rnhmjoj commented Mar 15, 2021

bramd commented Apr 2, 2021

bramd commented Apr 2, 2021

rnhmjoj commented Apr 2, 2021

bramd commented Apr 3, 2021

rnhmjoj commented Apr 11, 2021

rnhmjoj commented May 7, 2021

bramd commented May 7, 2021

rnhmjoj commented May 7, 2021

brltty: 6.1 -> 6.3; nixos/brltty: use upstream units #114745

brltty: 6.1 -> 6.3; nixos/brltty: use upstream units #114745

Conversation

rnhmjoj commented Mar 1, 2021 • edited Loading

Motivation for this change

Things done

r-rmcgibbo commented Mar 1, 2021 • edited Loading

SuperSandro2000 Mar 1, 2021

Choose a reason for hiding this comment

SuperSandro2000 commented Mar 1, 2021

bramd commented Mar 12, 2021

rnhmjoj commented Mar 12, 2021

bramd commented Mar 13, 2021

rnhmjoj commented Mar 14, 2021

bramd commented Mar 14, 2021

rnhmjoj commented Mar 14, 2021

bramd commented Mar 14, 2021

rnhmjoj commented Mar 15, 2021

rnhmjoj commented Mar 15, 2021

bramd commented Apr 2, 2021

bramd commented Apr 2, 2021

rnhmjoj commented Apr 2, 2021

bramd commented Apr 3, 2021

rnhmjoj commented Apr 11, 2021

rnhmjoj commented May 7, 2021

bramd commented May 7, 2021

rnhmjoj commented May 7, 2021

rnhmjoj commented Mar 1, 2021 •

edited

Loading

r-rmcgibbo commented Mar 1, 2021 •

edited

Loading