Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions nixos/doc/manual/release-notes/rl-2311.section.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

- [sitespeed-io](https://sitespeed.io), a tool that can generate metrics (timings, diagnostics) for websites. Available as [services.sitespeed-io](#opt-services.sitespeed-io.enable).

- [tiltfive](https://www.tiltfive.com/), drivers for the Tilt Five™ Glasses.

## Backward Incompatibilities {#sec-release-23.11-incompatibilities}

- `writeTextFile` now requires `executable` to be boolean, values like `null` or `""` will now fail to evaluate.
Expand Down
94 changes: 94 additions & 0 deletions nixos/modules/hardware/tiltfive.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
{ pkgs, lib, config, ... }:

with lib;
Copy link
Copy Markdown
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;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There's a bunch of mkXXX calls in there. Why is this with lib; a problem? Should it be nested somewhere deeper? Should I use lib.mkXXX explicitly?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

with is an anti-pattern too ingrained in old code. Indeed I am cleaning up this mess, and it is hard as hell.

Why is this with lib; a problem?

  1. with works in a similar manner of #include <> from C language, bringing to the scope of your code a whole bunch of things you are not using. This is not so harmful in Nix because it uses lazy evaluation, however it can be a pain for static analysis, and can be worse if you accidentally invokes a functionality from it.
  2. with works in unexpected ways when used more than once.
  3. its scoping rules are obscure: Scoping is unintuitive nix#490

Should I use lib.mkXXX explicitly?

Yes.

Another less verbose alternative is to substitute from with lib; to inherit (lib) mkXXX mkXXY mkXXZ . . .;

(Usually @SuperSandro2000 prefers the explicit lib. usage, but I have no strong opinions between the options above.)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Another less verbose alternative is to substitute from with lib; to inherit (lib) mkXXX mkXXY mkXXZ . . .;

where you need to maintain a list of used functions which is also not that great.

let
cfg = config.hardware.tiltfive;
in
{
options.hardware.tiltfive = {
enable = mkEnableOption (lib.mdDoc "tiltfive driver and service");
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Tilt Five


user = mkOption {
type = types.str;
default = "tiltfive";
description = lib.mdDoc "User account under which the Tilt Five driver service will run.";
};

group = mkOption {
type = types.str;
default = "tiltfive";
description = lib.mdDoc "Group under which the Tilt Five driver service will run.";
};

dataDirectory = mkOption {
type = types.path;
default = "/var/lib/tiltfive";
description = lib.mdDoc ''
Directory where the Tilt Five service will store persistent data, eg.
settings. Make sure it is writable.
'';
};
};

config = mkIf cfg.enable {
systemd.services.tiltfive = {
description = "Tilt Five Service";
serviceConfig = {
Type = "simple";
ExecStart = "${pkgs.tiltfive-driver}/bin/tiltfive-service";
Restart = "always";
RestartSec = 5;

User = cfg.user;
Group = cfg.group;

Environment = [
# The service does not run with telemetry enabled by default, but it
# always attempts to create a telemetry database anyway.
#
# When not specified, the telemetry database defaults to being
# installation-relative, so in our case it attempts to create the
# telemetry database in the nix store. This will naturally fail.
# Setting this environment variable makes it use a different location
# for the database, leading to ugly startup errors.
"TILTFIVE_TELEMDB=${cfg.dataDirectory}/telemdb"

# Same deal for logs.
"TILTFIVE_LOG=${cfg.dataDirectory}/log"
];
};
wantedBy = [ "multi-user.target" ];
};

# We don't use udev rules from the driver package as we want to be able to
# customize the group which has access to the tiltfive devices (ie. the
# group under which the driver service runs).
services.udev.extraRules = ''
# Tilt Five glasses
SUBSYSTEMS=="usb", ATTRS{idVendor}=="32f8", ATTRS{idProduct}=="9200", GROUP="${cfg.group}"

# Tilt Five glasses (wrong speed)
SUBSYSTEMS=="usb", ATTRS{idVendor}=="32f8", ATTRS{idProduct}=="2510", GROUP="${cfg.group}"

# Tilt Five glasses bootloader
SUBSYSTEMS=="usb", ATTRS{idVendor}=="32f8", ATTRS{idProduct}=="424c", GROUP="${cfg.group}"
'';

users.users = mkIf (cfg.user == "tiltfive") {
tiltfive = {
group = cfg.group;
home = cfg.dataDirectory;
createHome = true;
isSystemUser = true;
};
};

users.groups = mkIf (cfg.user == "tiltfive") {
tiltfive = {};
};

environment.systemPackages = [
pkgs.tiltfive-control-panel
];
};
}
1 change: 1 addition & 0 deletions nixos/modules/module-list.nix
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
./hardware/sensor/iio.nix
./hardware/steam-hardware.nix
./hardware/system-76.nix
./hardware/tiltfive.nix
./hardware/tuxedo-keyboard.nix
./hardware/ubertooth.nix
./hardware/uinput.nix
Expand Down
1 change: 1 addition & 0 deletions nixos/tests/all-tests.nix
Original file line number Diff line number Diff line change
Expand Up @@ -765,6 +765,7 @@ in {
tigervnc = handleTest ./tigervnc.nix {};
timescaledb = handleTest ./timescaledb.nix {};
promscale = handleTest ./promscale.nix {};
tiltfive = handleTestOn ["x86_64-linux"] ./tiltfive.nix {};
timezone = handleTest ./timezone.nix {};
tinc = handleTest ./tinc {};
tinydns = handleTest ./tinydns.nix {};
Expand Down
42 changes: 42 additions & 0 deletions nixos/tests/tiltfive.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import ./make-test-python.nix ({ pkgs, lib, ... }: {
name = "tiltfive";
meta.maintainers = with lib.maintainers; [ q3k ];

nodes.machine = { nodes, ... }:
let user = nodes.machine.users.users.alice;
in {
imports = [ ./common/user-account.nix ./common/x11.nix ];

hardware.tiltfive.enable = true;
nixpkgs.config.allowUnfree = true;

test-support.displayManager.auto.user = user.name;
environment.systemPackages = [ pkgs.xdotool ];
};

enableOCR = true;

testScript = { nodes, ... }:
let user = nodes.machine.users.users.alice;
in ''
machine.wait_for_x()
machine.wait_for_file("${user.home}/.Xauthority")
machine.succeed("xauth merge ${user.home}/.Xauthority")

# Start the control panel without the service running and expect error
# message. This ensures that this test exercises connectivity between the
# service and the control panel.
machine.execute("systemctl stop tiltfive")

machine.execute("su - alice -c tiltfive-control-panel >&2 &")
machine.wait_for_text("Service disconnected")
machine.screenshot("controlpanel1")

# Now start the service and expect the wizard to start up.
machine.execute("systemctl start tiltfive")
machine.wait_for_text("Gameboard not set")
machine.screenshot("controlpanel2")

# This is is the best we can do without emulating Tilt Five glasses :).
'';
})
67 changes: 67 additions & 0 deletions pkgs/games/tiltfive/control-panel.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
{ stdenv
, lib
, autoPatchelfHook
, fetchurl
, nixosTests
, gtk3
, glfw
}:

let srcs = import ./srcs.nix { inherit fetchurl; }; in
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Write the sources directly.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Both control-panel.nix/tiltfive-control-panel and driver.nix/tiltfive-driver use the same source (tiltfive_driver_${version}_amd64.unpacked_debs.tar.xz). Inlining srcs.nix would mean having the same source (alongside version, hash, etc) declared twice, once in each file for each derivation. Plus the version would also be duplicated across three derivation sources.

For DRY, I would think it's better to keep it as is (or deduplicate it some other way), no?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can re-use the src and version attr of one package in another without this extra trick. We just need to find one main package and then we could do inherit (tiltfive-control-panel) version src;

stdenv.mkDerivation rec {
pname = "tiltfive-control-panel";
version = srcs.version;
src = srcs.driver;

buildInputs = [
gtk3 glfw
];

nativeBuildInputs = [
autoPatchelfHook
];

installPhase = ''
runHook preInstall

cd tiltfive-control-panel_${version}_amd64/files/

dest=$out/opt/tiltfive/control-panel
pushd opt/tiltfive/control-panel

mkdir -p $dest/data
cp -r data/* $dest/data/

mkdir -p $dest/lib
install -Dm755 lib/* $dest/lib/

install -Dm755 control_panel $dest/


popd
Comment on lines 29 to 41
Copy link
Copy Markdown
Member

@AndersonTorres AndersonTorres Jun 12, 2023

Choose a reason for hiding this comment

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

Suggested change
dest=$out/opt/tiltfive/control-panel
pushd opt/tiltfive/control-panel
mkdir -p $dest/data
cp -r data/* $dest/data/
mkdir -p $dest/lib
install -Dm755 lib/* $dest/lib/
install -Dm755 control_panel $dest/
popd
dest=$out/opt/tiltfive/control-panel
pushd opt/tiltfive/control-panel
mkdir -p $dest/data $dest/lib
cp -r data/* $dest/data/
install -Dm755 lib/* $dest/lib/
install -Dm755 control_panel $dest/
popd

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I spersonally find the version with whitespace more readable. Is there a concrete style recommendation to avoid newlines in this case?

Copy link
Copy Markdown
Member

@AndersonTorres AndersonTorres Jun 14, 2023

Choose a reason for hiding this comment

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

nope.



but using two white lines makes no sense


mkdir -p $out/share/applications
install -Dm644 usr/share/applications/${pname}.desktop -t $out/share/applications
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do not parameterize pname

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

nixpkgs $ grep -r '{pname}.desktop' | wc -l
90

This seems to be a fairly common pattern in nixpkgs. Is there a concrete style recommendation against this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems to be a fairly common pattern in nixpkgs.

That doesn't mean anything since there is lots of code which wasn't touched in a good amount of time and should be updated.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Right, but what is the reason to not do this? I'll gladly change things around, but I'd prefer to know what is the reasoning behind this so that I can then explain the same thing to others :).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

pname could be changed with a prefix which does not break with rec but with finalAttrs. Also it increases code complexity and pname does not change often.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems to be a fairly common pattern in nixpkgs. Is there a concrete style recommendation against this?

90 occurrences in a codebase of 80k+ packages? This is not "fairly common".

Right, but what is the reason to not do this?

  1. it creates a useless dependency link, making static debugging a little more difficult.
  2. sometimes pname is not perfectly parameterizable. E.g. the GitHub repository of openmsx is openMSX/openMSX. What we should do? Pass it under a obfuscated combination of string splitting?
  3. being effectively a constant, using pname makes no real difference to understand the code. It is just harder to read and conveys nothing useful.

substituteInPlace $out/share/applications/${pname}.desktop \
--replace '/opt/tiltfive/control-panel/' "$dest/"

mkdir -p $out/bin
ln -s $dest/control_panel $out/bin/${pname}

cp opt/tiltfive/LICENSE* $out/

runHook postInstall
'';

passthru.tests.tiltfive = nixosTests.tiltfive;

meta = with lib; {
description = "Tilt Five™ Glasses control panel";
homepage = "https://docs.tiltfive.com/index.html";
# Non-redistributable. See: LICENSE.control-panel.txt, 2.1.2.
license = with licenses; unfree;
sourceProvenance = with sourceTypes; [ binaryNativeCode ];
maintainers = with maintainers; [ q3k ];
platforms = [ "x86_64-linux" ];
};
}
57 changes: 57 additions & 0 deletions pkgs/games/tiltfive/driver.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
{ stdenv
, lib
, autoPatchelfHook
, fetchurl
, nixosTests

, glfw
, udev
}:

let srcs = import ./srcs.nix { inherit fetchurl; }; in
stdenv.mkDerivation {
pname = "tiltfive-driver";
version = srcs.version;

srcs = srcs.driver;

buildInputs = [
glfw
];

nativeBuildInputs = [
autoPatchelfHook
];

runtimeDependencies = [
udev
];

installPhase = ''
runHook preInstall

cd tiltfive-service_${srcs.version}_amd64/files/

mkdir -p $out/bin
install -Dm755 opt/tiltfive/bin/* -t $out/bin/

mkdir -p $out/var/opt/tiltfive/firmware/
install -Dm644 var/opt/tiltfive/firmware/* -t $out/var/opt/tiltfive/firmware/

cp opt/tiltfive/LICENSE* $out/

runHook postInstall
'';

passthru.tests.tiltfive = nixosTests.tiltfive;

meta = with lib; {
description = "Tilt Five™ Glasses driver / userspace service";
homepage = "https://docs.tiltfive.com/index.html";
# Non-redistributable. See: LICENSE.service.txt, 2.1.2.
license = with licenses; unfree;
sourceProvenance = with sourceTypes; [ binaryNativeCode ];
maintainers = with maintainers; [ q3k ];
platforms = [ "x86_64-linux" ];
};
}
45 changes: 45 additions & 0 deletions pkgs/games/tiltfive/sdk.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
{ stdenv
, lib
, autoPatchelfHook
, fetchurl
, glfw
}:

let srcs = import ./srcs.nix { inherit fetchurl; }; in
stdenv.mkDerivation {
pname = "tiltfive-sdk";
version = srcs.version;

srcs = srcs.sdk;

buildInputs = [
glfw
];

nativeBuildInputs = [
autoPatchelfHook
];

installPhase = ''
runHook preInstall

mkdir -p $out

cp -rv * $out/

chmod 755 $out/Native/lib/linux/x86_64/libTiltFiveNative.so
chmod 755 $out/Utils/Linux/gameboard_transform

runHook postInstall
'';

meta = with lib; {
description = "Tilt Five™ Glasses SDK";
homepage = "https://docs.tiltfive.com/index.html";
# Non-redistributable. See: license_sdk_en.txt, 2.1.2.
license = with licenses; unfree;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
license = with licenses; unfree;
license = licenses.unfree;

sourceProvenance = with sourceTypes; [ binaryNativeCode ];
maintainers = with maintainers; [ q3k ];
platforms = [ "x86_64-linux" ];
};
}
14 changes: 14 additions & 0 deletions pkgs/games/tiltfive/srcs.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{ fetchurl }:
rec {
version = "1.3.2";

driver = fetchurl {
url = "https://files.tiltfive.com/tiltfive_driver_${version}_amd64.unpacked_debs.tar.xz";
sha256 = "sha256-MWxKqFEM6Q3gf8ik6OZk2aSFe6piTGljPOLpstJh0vA=";
};

sdk = fetchurl {
url = "https://files.tiltfive.com/tiltfive_sdk_${version}.tar.xz";
sha256 = "sha256-LC3DxOOHulgYO266LdHfAS6kiOzcnG7fEON4SJkMQlU=";
};
}
6 changes: 6 additions & 0 deletions pkgs/top-level/all-packages.nix
Original file line number Diff line number Diff line change
Expand Up @@ -37317,6 +37317,12 @@ with pkgs;

tibia = pkgsi686Linux.callPackage ../games/tibia { };

tiltfive-driver = callPackage ../games/tiltfive/driver.nix { };

tiltfive-control-panel = callPackage ../games/tiltfive/control-panel.nix { };

tiltfive-sdk = callPackage ../games/tiltfive/sdk.nix { };

tintin = callPackage ../games/tintin { };

tinyfugue = callPackage ../games/tinyfugue { };
Expand Down