tiltfive: init at 1.3.2 by q3k · Pull Request #236695 · NixOS/nixpkgs

q3k · 2023-06-08T16:36:55Z

Tilt Five™ Glasses are an AR headset.

This packages its driver (control panel, service) and SDK. It also implements a NixOS module which installs the driver.

Description of changes

Things done

khunt-tiltfive

Hope you don't mind a couple of comments inline. I'm not familiar with Nix, so I've not considered that side of things.

(Disclaimer - I work for Tilt Five, but my comments here don't represent an official position or any kind of endorsement).

nixos/modules/hardware/tiltfive.nix

nixos/doc/manual/release-notes/rl-2311.section.md

nixos/modules/hardware/tiltfive.nix

khunt-tiltfive · 2023-06-08T17:17:16Z

My comments have been resolved, thank you. Cursory inspection looks ok, but I'm not qualified to comment on nix things.
(Disclaimer - I work for Tilt Five, but my comments here don't represent an official position or any kind of endorsement).

q3k · 2023-06-08T17:29:08Z

@ofborg build tiltfive-control-panel tiltfive-driver tiltfive-sdk
@ofborg test tiltfive

q3k · 2023-06-08T17:33:46Z

Ah, it seems like OfBorg refuses to build/test unfree packages?

q3k · 2023-06-08T17:40:13Z

@ofborg eval

nixos/doc/manual/release-notes/rl-2311.section.md

AndersonTorres · 2023-06-12T01:23:15Z

nixos/modules/hardware/tiltfive.nix

Suggested change

with lib;

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?

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?

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.

with works in unexpected ways when used more than once.

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.)

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.

nixos/modules/hardware/tiltfive.nix

nixos/tests/tiltfive.nix

pkgs/games/tiltfive/control-panel.nix

AndersonTorres · 2023-06-12T01:29:56Z

pkgs/games/tiltfive/control-panel.nix

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

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

nope. but using two white lines makes no sense

AndersonTorres · 2023-06-12T01:30:24Z

pkgs/games/tiltfive/control-panel.nix

Do not parameterize pname

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?

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.

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 :).

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.

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?

it creates a useless dependency link, making static debugging a little more difficult.

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?

being effectively a constant, using pname makes no real difference to understand the code. It is just harder to read and conveys nothing useful.

pkgs/games/tiltfive/sdk.nix

nixos/modules/hardware/tiltfive.nix

SuperSandro2000 · 2023-06-12T09:36:21Z

pkgs/games/tiltfive/control-panel.nix

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.

SuperSandro2000 · 2023-06-14T13:10:30Z

pkgs/games/tiltfive/sdk.nix

+    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;


Suggested change

license = with licenses; unfree;

license = licenses.unfree;

pedrovelho · 2025-04-14T07:22:54Z

nixos/modules/hardware/tiltfive.nix

+in
+{
+  options.hardware.tiltfive = {
+    enable = mkEnableOption (lib.mdDoc "tiltfive driver and service");


q3k force-pushed the q3k/tiltfive branch from 13370d6 to ec60b0d Compare June 8, 2023 16:43

khunt-tiltfive reviewed Jun 8, 2023

View reviewed changes

q3k force-pushed the q3k/tiltfive branch from ec60b0d to f9523b0 Compare June 8, 2023 17:09

q3k force-pushed the q3k/tiltfive branch from f9523b0 to e7623ae Compare June 8, 2023 17:25

q3k force-pushed the q3k/tiltfive branch from e7623ae to 47e560e Compare June 8, 2023 17:33

rhendric mentioned this pull request Jun 10, 2023

nix: build unfree packages NixOS/ofborg#644

Closed

AndersonTorres suggested changes Jun 12, 2023

View reviewed changes

SuperSandro2000 reviewed Jun 12, 2023

View reviewed changes

q3k added 2 commits June 12, 2023 11:55

tiltfive-driver,tiltfive-control-panel,tiltfive-sdk: init at 1.3.2

302d679

nixos/tiltfive: init at 1.3.2

af9c4bb

q3k force-pushed the q3k/tiltfive branch from 47e560e to af9c4bb Compare June 12, 2023 09:55

SuperSandro2000 reviewed Jun 14, 2023

View reviewed changes

h7x4 added 8.has: module (new) This PR adds a module in `nixos/` 8.has: tests This PR has tests labels Oct 19, 2023

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 bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024

wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024

pedrovelho reviewed Apr 14, 2025

View reviewed changes

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 14, 2025

nixpkgs-ci bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 12, 2025

-    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

Uh oh!

Conversation

q3k commented Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Things done

Uh oh!

khunt-tiltfive left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

khunt-tiltfive commented Jun 8, 2023

Uh oh!

q3k commented Jun 8, 2023

Uh oh!

q3k commented Jun 8, 2023

Uh oh!

q3k commented Jun 8, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndersonTorres Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndersonTorres Jun 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

q3k commented Jun 8, 2023 •

edited

Loading

AndersonTorres Jun 12, 2023 •

edited

Loading

AndersonTorres Jun 14, 2023 •

edited

Loading