Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

arrpc: 3.4.0 -> 3.5.0; add systemd user service #351774

Merged
merged 2 commits into from
Dec 11, 2024

Conversation

UlyssesZh
Copy link
Member

Using tagged releases as OpenAsar/arrpc#48 is resolved now.

Added a systemd user service.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Comment on lines 23 to 24
substitute ${./arrpc.service} $out/share/systemd/user/arrpc.service \
--subst-var-by arrpc $out/bin/arrpc
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this is standard practice, would be the first time I see this in nixpkgs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is standard. Packages like thunar and syncthing all installs a service file there. Example:

# point to wrapped binary in all service files
for file in "lib/systemd/user/thunar.service" \
"share/dbus-1/services/org.xfce.FileManager.service" \
"share/dbus-1/services/org.xfce.Thunar.FileManager1.service" \
"share/dbus-1/services/org.xfce.Thunar.service"
do
rm -f "$out/$file"
substitute "${thunar}/$file" "$out/$file" \
--replace "${thunar}" "$out"
done

It will be picked up by system if config.systemd.packages includes the package. My concern is whether it should be a system unit or a user unit.

Copy link
Member

Choose a reason for hiding this comment

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

The example you've provided is for vendoring a service provided by upstream - not for shipping our own, though I can't exactly see an issue with it...

I am in favor of having it as an user service, since that's already what the home-manager module does

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright it seems that I am wrong. I need to define a nixos module for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

https://discourse.nixos.org/t/how-to-enable-upstream-systemd-user-services-declaratively/7649

Honestly I still don't quite understand how systemd user services provided by packages work in NixOS. There are some discussions.

Copy link
Member

@NotAShelf NotAShelf left a comment

Choose a reason for hiding this comment

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

Please separate formatting and package change commits.

@UlyssesZh UlyssesZh marked this pull request as draft October 28, 2024 16:05
@UlyssesZh
Copy link
Member Author

UlyssesZh commented Oct 28, 2024

@NotAShelf @Anomalocaridid It seems that running arrpc for a while (like some minutes) makes it un-killable even with kill -9. This makes the computer stuck at shutting down, which is pretty bad. Could you see whether you can reproduce the bug? I cannot find an issue for that in the upstream, but I am suspecting that this is a bug of newer Node versions.

@NotAShelf
Copy link
Member

I've been running arrpc for a while now, with relative success (if you don't count the CPU spike after a few hours) Is this bug exclusive to 3.5.0? Or does it exist on older versions as well?

@UlyssesZh
Copy link
Member Author

It exists on 3.4.0, too. It only happened after I upgraded my NixOS 24.05 channel on Oct 24. Could you please try the master branch of nixpkgs?

@UlyssesZh
Copy link
Member Author

On my laptop, cloning the arrpc repo main branch and run npm i && node src (with Node 20.15 from NixOS 24.05) reproduces this bug.

@NotAShelf
Copy link
Member

Okay, built from master and started it in the background. I'll leave it running for a while.

@NotAShelf
Copy link
Member

result/bin/arrpc 
[arRPC] arRPC v3.4.0
[arRPC > bridge] listening on 1337
[arRPC > ipc] listening at /run/user/1000/discord-ipc-0
[arRPC > websocket] 6463 in use!
[arRPC > websocket] listening on 6464
[arRPC > process] started
zsh: killed     result/bin/arrpc

after 10 minutes of runtime, killed with kill -9 <pid>

@UlyssesZh
Copy link
Member Author

That's strange. Could you try my branch in the PR?

@UlyssesZh
Copy link
Member Author

This is getting increasingly strange. If I use an older generation built before I updated on Oct 24, the bug doesn't appear, even if I run the exact same result/bin/arrpc.

@NotAShelf
Copy link
Member

Also works from your branch.

@ofborg ofborg bot requested a review from NotAShelf October 28, 2024 19:24
@UlyssesZh UlyssesZh force-pushed the arrpc-3.5.0 branch 2 times, most recently from 5f381ea to 2add37d Compare October 29, 2024 00:08
@UlyssesZh
Copy link
Member Author

Could you share your nix-info -m? I can steadily reproduce this bug. Here is mine:

  • system: "x86_64-linux"
  • host os: Linux 6.6.58, NixOS, 24.05 (Uakari), 24.05.5993.cd3e8833d706
  • multi-user?: yes
  • sandbox: yes
  • version: nix-env (Nix) 2.18.8
  • channels(root): "nixos-24.05, nixos-hardware, nixos-unstable"
  • nixpkgs: /nix/var/nix/profiles/per-user/root/channels/nixos

This Linux kernel shouldn't matter because I tried both 6.1 and 6.6 (I cannot try newer kernels because nvidia-x11 fails to build).

If you are also using the nixos-24.05 channel, could you try upgrading it? Sorry for bothering.

@NotAShelf
Copy link
Member

Mine is similar to yours, though I'm running

  1. Lix
  2. nixos-unstable (24.11)

I believe we have already established that your branch does build and run fine, as well as current master. This is likely an issue on your end.

@UlyssesZh
Copy link
Member Author

After bisecting, I found that f142a76 (linux_6_6: 6.6.56 -> 6.6.57) (if I use kernel 6.6) or 0dffe83 (linux_6_1: 6.1.112 -> 6.1.113) (if I use kernel 6.1) is the problematic commit. @NotAShelf Could you please tell me your kernel version?

@NotAShelf
Copy link
Member

NotAShelf commented Oct 30, 2024

Currently 6.12-rc5, I was running 6.11.5 at the time of my previous tests.

@UlyssesZh
Copy link
Member Author

Sadly I cannot use kernel 6.11 because nvidia-x11 fails to build.

Opened OpenAsar/arrpc#120.

@FliegendeWurst FliegendeWurst merged commit 44f8ed8 into NixOS:master Dec 11, 2024
39 of 40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants