lorri: init at version unstable-2019-10-30 #72889

curiousleo · 2019-11-06T12:23:31Z

Motivation for this change

lorri is still unstable, but already makes it easier to work with project-specific Nix shells.

Things done

Notify maintainers

cc @Profpatsch @grahamc @ocharles

nixos/modules/services/development/lorri.nix

curiousleo · 2019-11-06T12:39:45Z

pkgs/tools/misc/lorri/default.nix

+    description = "Your project's nix-env";
+    homepage = "https://github.com/target/lorri";
+    license = licenses.asl20;
+    maintainers = [ maintainers.Profpatsch ];


@Profpatsch :D - let me know if you're okay with this

Sure. We should add Graham and yourself :)

Profpatsch

Great work!

nixos/modules/services/development/lorri.nix

Profpatsch · 2019-11-06T15:28:22Z

pkgs/tools/misc/lorri/default.nix

+    description = "Your project's nix-env";
+    homepage = "https://github.com/target/lorri";
+    license = licenses.asl20;
+    maintainers = [ maintainers.Profpatsch ];


Sure. We should add Graham and yourself :)

pkgs/tools/misc/lorri/runtime-closure.nix.template

Profpatsch · 2019-11-06T15:59:55Z

pkgs/tools/misc/lorri/default.nix

+
+rustPlatform.buildRustPackage rec {
+  pname = "lorri";
+  version = "rolling-release-2019-10-30";


I think we wanted to push this as 0.5 (should also put that into the Cargo.toml in upstream).

Cool - I'm happy to change this to reflect a new release of lorri. For now, I think that rolling-release-2019-10-30 is an accurate name for the revision this is pulling from.

Thanks for the porting effort.
I think unstable-YYYY-MM-DD is the preferred naming schema. But 0.5 is released soon. So no further change needed.

Profpatsch · 2019-11-06T16:02:39Z

pkgs/tools/misc/lorri/default.nix

+  cargoSha256 = "1daff4plh7hwclfp21hkx4fiflh9r80y2c7k2sd3zm4lmpy0jpfz";
+
+  # Note that https://travis-ci.org/target/lorri/builds/605036891 passed.
+  doCheck = false;


Are the tests failing? We might want to enable them, because we pin a different nixpkgs version in the lorri repo (which we don’t update that often usually).

Yes, they were failing due to path issues. I'll look into making them pass.

Okay, here's what I've tried:

If I just set doCheck = true, two tests fail. One of them because it fails to write something to /nix/var. I believe that this error does not occur upstream because of the bogus-nixpkgs NIX_PATH: https://github.com/target/lorri/blob/95c3e3da1be4f0d8ddb62dce21afa02d8cecfdf7/default.nix#L18. I don't really understand the effect of setting this.

The bogus-nixpkgs thing in the upstream default.nix requires that directory to exist, and possibly also to be populated with packages that are used for testing: https://github.com/target/lorri/tree/95c3e3da1be4f0d8ddb62dce21afa02d8cecfdf7/nix/bogus-nixpkgs. I think there is a way to make this work, which is essentially "copy every .nix file from the lorri repo into nixpkgs in this PR".

As a last alternative, I kept doCheck = false and tried to create an integration test in nixpkgs/nixos/tests. I didn't get very far unfortunately: making the Python test helpers do what I want is non-trivial.

I don't really understand the effect of setting this.

Setting it in the checkPhase should be fine and fix that issue.

which is essentially "copy every .nix file from the lorri repo into nixpkgs in this PR".

Just the stuff under nix/bogus-nixpkgs should be necessary.

I couldn't get it to work, and I'm not convinced it is worth cluttering the nixpkgs repo with lorri's nix files and slowing down the nixpkgs build given that lorri has CI. You can see my attempts in the commit history of this branch.

@curiousleo you can just point the NIX_PATH to $src. No need to have these nix files locally.

Yep, you can do something like

preCheck = '' export NIX_PATH=nixpkgs=${src}/nix/bogus-nixpkgs ''

you can just point the NIX_PATH to $src. No need to have these nix files locally.

I must have misunderstood @Profpatsch - I thought you said that there were checks preventing the use of this technique in order to ensure all nix files are actually checked into this repo.

That makes things much neater, of course! :)

Yep, you can do something like

preCheck = '' export NIX_PATH=nixpkgs=${src}/nix/bogus-nixpkgs ''

The test errors with "can't write to /nix/store" persist. Do you think running the unit tests is necessary if there is an integration test (https://github.com/NixOS/nixpkgs/blob/115d1ec64f851df9db365ad7b6bf7b3e629f32d0/nixos/tests/lorri/default.nix)? If yes, I'm going to need help with getting doCheck = true to work.

Let’s just set it to false for now, to get it in. We are running the tests upstream anyway.

nixos/modules/services/development/lorri.nix

Profpatsch · 2019-11-06T16:15:30Z

nixos/modules/services/development/lorri.nix

+        PrivateTmp = true;
+        ProtectSystem = "strict";
+        ProtectHome = "read-only";
+        Restart = "on-failure";


Not sure, it’s certainly more user-friendly, but would we be notified of issues that way?

It should still appear in the syslog, and if it keeps crashing, systemd won't restart it indefinitely, so should be fine IMHO.

nixos/modules/services/development/lorri.nix

aszlig · 2019-11-06T19:47:12Z

nixos/modules/services/development/lorri.nix

+      serviceConfig = {
+        ExecStart = "${pkgs.lorri}/bin/lorri daemon";
+        PrivateTmp = true;
+        ProtectSystem = "strict";


Remark: Even more strict would be if we'd have systemd-confinement for user units.

I'd keep that as a follow-up.

nixos/modules/services/development/lorri.nix

curiousleo

Thanks @aszlig and @Profpatsch for the reviews! I will now do the following:

Remove the systemd service from this PR as suggested by @Profpatsch.
Address your comments on the package declaration to get it into a mergeable state.
Create a second PR with just the systemd service and address your comments related to the service declaration there. I'll reply to your comments here and point to the changes I'll have made so you'll see that I will have addressed them.

nixos/modules/services/development/lorri.nix

curiousleo · 2019-11-07T08:42:07Z

pkgs/tools/misc/lorri/default.nix

+
+rustPlatform.buildRustPackage rec {
+  pname = "lorri";
+  version = "rolling-release-2019-10-30";


Cool - I'm happy to change this to reflect a new release of lorri. For now, I think that rolling-release-2019-10-30 is an accurate name for the revision this is pulling from.

curiousleo · 2019-11-07T08:42:38Z

pkgs/tools/misc/lorri/default.nix

+  cargoSha256 = "1daff4plh7hwclfp21hkx4fiflh9r80y2c7k2sd3zm4lmpy0jpfz";
+
+  # Note that https://travis-ci.org/target/lorri/builds/605036891 passed.
+  doCheck = false;


Yes, they were failing due to path issues. I'll look into making them pass.

curiousleo · 2019-11-08T12:43:13Z

Comments on the package declaration addressed, PTAL. (See #72889 (review) for what I'm planning to do next.)

curiousleo · 2019-11-08T18:10:30Z

I have not yet addressed all review comments - no need to review again at this point, thanks!

nixos/modules/services/development/lorri.nix

flokli · 2019-11-12T10:03:28Z

nixos/modules/services/development/lorri.nix

+      serviceConfig = {
+        ExecStart = "${pkgs.lorri}/bin/lorri daemon";
+        PrivateTmp = true;
+        ProtectSystem = "strict";


I'd keep that as a follow-up.

flokli · 2019-11-12T10:04:09Z

nixos/modules/services/development/lorri.nix

+        PrivateTmp = true;
+        ProtectSystem = "strict";
+        ProtectHome = "read-only";
+        Restart = "on-failure";


It should still appear in the syslog, and if it keeps crashing, systemd won't restart it indefinitely, so should be fine IMHO.

nixos/tests/lorri/default.nix

pkgs/tools/misc/lorri/default.nix

nixos/tests/lorri/default.nix

curiousleo · 2019-11-12T15:44:26Z

Comments addressed, PTAL.

curiousleo · 2019-11-13T09:26:41Z

@grahamc @Profpatsch mind taking another look?

flokli · 2019-11-13T15:07:29Z

@GrahamcOfBorg test lorri

flokli · 2019-11-13T15:20:00Z

nixos/tests/lorri/default.nix

+    )
+
+    # Start the daemon and wait until it is ready
+    machine.execute("lorri daemon > lorri.stdout 2> lorri.stderr &")


At least according to previous discussions, we don't really want to run lorri as the root user, do we?

I'd suggest to import ./common/user-account.nix, and run it as alice or bob.

We also don't test the socket activation at all. I propose starting and forking the daemon manually should be done for alice, and bob should rely on systemd socket activation. WDYT?

My thinking:

no test < current test < what you're proposing

Unfortunately I can't get your proposal to work! So I'm inclined to merge now with the test as-is, and improve on the test in a follow-up.

Captured this task here: target/lorri#218

Profpatsch

I think we can merge
Addressed the description part, PTAL. Would love a meta.longDescription as well, but that can be fixed later.

We can pull the tests into a different PR if you want, the module and package are good!

Includes user service (nixos/modules/services/development/lorri) that starts on demand.

Profpatsch · 2019-11-14T14:35:49Z

@GrahamcOfBorg test lorri

Profpatsch · 2019-11-14T15:06:20Z

LGTM!

curiousleo mentioned this pull request Nov 6, 2019

lorri (including user service): init at version rolling-release-2019-10-30 curiousleo/nixpkgs#1

Closed

ofborg bot added 6.topic: nixos 8.has: module (update) 8.has: package (new) labels Nov 6, 2019

ofborg bot requested a review from Profpatsch November 6, 2019 12:37

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Nov 6, 2019

curiousleo commented Nov 6, 2019

View reviewed changes

nixos/modules/services/development/lorri.nix Outdated Show resolved Hide resolved

curiousleo commented Nov 6, 2019

View reviewed changes

Profpatsch reviewed Nov 6, 2019

View reviewed changes

ofborg bot requested a review from Profpatsch November 6, 2019 17:19

aszlig reviewed Nov 6, 2019

View reviewed changes

nixos/modules/services/development/lorri.nix Outdated Show resolved Hide resolved

aszlig reviewed Nov 6, 2019

View reviewed changes

nixos/modules/services/development/lorri.nix Outdated Show resolved Hide resolved

aszlig reviewed Nov 6, 2019

View reviewed changes

nixos/modules/services/development/lorri.nix Outdated Show resolved Hide resolved

curiousleo commented Nov 7, 2019

View reviewed changes

ofborg bot removed 6.topic: nixos 8.has: module (update) labels Nov 8, 2019

ofborg bot requested a review from grahamc November 8, 2019 12:51

curiousleo force-pushed the add-lorri branch from 4a0fb42 to dad85f1 Compare November 8, 2019 13:02

curiousleo marked this pull request as ready for review November 8, 2019 13:07

curiousleo changed the title ~~lorri (including user service): init at version rolling-release-2019-10-30~~ lorri: init at version rolling-release-2019-10-30 Nov 8, 2019

curiousleo force-pushed the add-lorri branch from dad85f1 to 115d1ec Compare November 8, 2019 18:04

curiousleo requested a review from infinisil as a code owner November 8, 2019 18:04

curiousleo force-pushed the add-lorri branch from dad85f1 to 115d1ec Compare November 8, 2019 18:04

ofborg bot added 6.topic: nixos 8.has: module (update) labels Nov 8, 2019

curiousleo mentioned this pull request Nov 12, 2019

Run lorri daemon as a service on Linux and macOS target/lorri#191

Merged

flokli reviewed Nov 12, 2019

View reviewed changes

pkgs/tools/misc/lorri/default.nix Outdated Show resolved Hide resolved

curiousleo force-pushed the add-lorri branch from 115d1ec to 98dd3bb Compare November 12, 2019 10:45

curiousleo requested a review from flokli November 12, 2019 10:46

flokli reviewed Nov 12, 2019

View reviewed changes

nixos/tests/lorri/default.nix Outdated Show resolved Hide resolved

curiousleo force-pushed the add-lorri branch 2 times, most recently from c990d27 to 061169f Compare November 12, 2019 14:22

curiousleo changed the title ~~lorri: init at version rolling-release-2019-10-30~~ lorri: init at version unstable-2019-10-30 Nov 13, 2019

curiousleo force-pushed the add-lorri branch from 061169f to 0e54a83 Compare November 13, 2019 09:23

curiousleo requested a review from flokli November 13, 2019 14:20

flokli reviewed Nov 13, 2019

View reviewed changes

Profpatsch approved these changes Nov 14, 2019

View reviewed changes

lorri: init at version unstable-2019-10-30

d8fffbc

Includes user service (nixos/modules/services/development/lorri) that starts on demand.

curiousleo force-pushed the add-lorri branch from 0e54a83 to 5ca6763 Compare November 14, 2019 12:26

ofborg bot requested a review from Profpatsch November 14, 2019 12:44

lorri service module: init

0cec061

curiousleo force-pushed the add-lorri branch from 5ca6763 to 0cec061 Compare November 14, 2019 12:49

Profpatsch merged commit e5db97d into NixOS:master Nov 14, 2019

curiousleo deleted the add-lorri branch November 14, 2019 15:11

This was referenced Nov 14, 2019

nixpkgs: add test for lorri.socket and lorri.service target/lorri#218

Open

Include version in error reports for easier triage target/lorri#219

Closed

Profpatsch mentioned this pull request Nov 15, 2019

[19.09] lorri: init at unstable-2019-10-30 (backport) #73451

Merged

flokli mentioned this pull request Nov 15, 2019

lorri: add service nix-community/home-manager#799

Closed

Janik-Haag added the 12. first-time contribution label Jun 12, 2023

lorri: init at version unstable-2019-10-30 #72889

lorri: init at version unstable-2019-10-30 #72889

Conversation

curiousleo commented Nov 6, 2019

Motivation for this change

Things done

Notify maintainers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Profpatsch 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

curiousleo Nov 12, 2019 • edited

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

curiousleo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curiousleo commented Nov 8, 2019

curiousleo commented Nov 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curiousleo commented Nov 12, 2019

curiousleo commented Nov 13, 2019

flokli commented Nov 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Profpatsch left a comment

Choose a reason for hiding this comment

Profpatsch commented Nov 14, 2019

Profpatsch commented Nov 14, 2019

curiousleo Nov 12, 2019 •

edited