Skip to content

abduco: add bug/feature patches, add mirror#286124

Merged
Lassulus merged 4 commits into
NixOS:masterfrom
toastal:abduco-xdg
Feb 15, 2024
Merged

abduco: add bug/feature patches, add mirror#286124
Lassulus merged 4 commits into
NixOS:masterfrom
toastal:abduco-xdg

Conversation

@toastal

@toastal toastal commented Feb 3, 2024

Copy link
Copy Markdown
Contributor

This eliminates clutter in the user’s $HOME directory

martanne/abduco#22

Exit codes not reported on dead sessions

martanne/abduco#45

Send down pixels for window sizing so terminal emulators like kitty can use it display images

martanne/abduco#62

Project owner explicitly states code is available on both GitHub & SourceHut, so a mirror was added--this also adds resilience.

Description of changes

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.05 Release Notes (or backporting 23.05 and 23.11 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.

@toastal
toastal requested a review from pSub February 3, 2024 18:49
@ofborg ofborg Bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Feb 3, 2024
@toastal toastal changed the title adbuco: patch to use XDG directory scheme by default adbuco: add bug/feature patches, add mirror Feb 3, 2024
Comment thread pkgs/tools/misc/abduco/default.nix Outdated
Comment on lines 14 to 15

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.

Please do not substitute all parts of the URL and just write them literal.

Also fetching from GitHub and SourceHut should use the corrosponding fetcher which also does not need to be substituted.

@toastal toastal Feb 10, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This doesn’t make sense to me. Those fetchers do not support mirrors, but the project is mirrored. Both fetchers are just wrappers for the common-denominator of fetchzip anyhow which does support mirrors. I have been in situations where I was wanting to hack on a Nix package, but one of the mirrors was down, but multiple mirrors were not specified.

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.

I don't think we support that currently but fetching from GitHub should be done via fetchFromGitHub to support possible future, better URLs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It’s the same URL & doesn’t support mirroring elsewhere so it’s a bad solution for many applications. The more mirrors we see off Microsoft GitHub, the more resilient the packaging can be from a single source of network failure.

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.

Well, no. Those are not straight up mirrors and the exact hash can depend on the git version used.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What do you think happens more… mirrors going down or a Git and NAR hash conflict that would cause wrong code to be shipped? I’ll spoil it & tell you that it will be servers going down. These projects explicitly state that they are using mirrors & shipping the same code to the multiple repos for accessibility & resilience. Microsoft GitHub goes down or times out several times a month in my region of the world (& it’s very annoying). I would not just honoring the maker’s mirror remark but also encouraging others to make sure their code isn’t centralized in one spot for when the server is down, the net is down, or a new sanction is raised.

Comment thread pkgs/tools/misc/abduco/default.nix Outdated
Comment thread pkgs/tools/misc/abduco/default.nix Outdated
Comment thread pkgs/tools/misc/abduco/default.nix Outdated
Comment thread pkgs/tools/misc/abduco/default.nix Outdated
This eliminates clutter in the user’s $HOME directory
> You can always fetch the current code base from the git repository
> located at GitHub or SourceHut.

The owner says the source code will be offered in two places so the code
fetching should reflect the mirror & it adds resiliance.
@tilpner

tilpner commented Jun 2, 2024

Copy link
Copy Markdown
Member

These patches were quite a surprise to me: XDG_RUNTIME_DIR is not available by default in systemd services, but is in a normal ssh session. If you now create an abduco session within a systemd service, it will by default bind to a socket in a different directory than abduco (in a normal ssh session) will use when attempting to connect to that session.

@toastal

toastal commented Jun 3, 2024

Copy link
Copy Markdown
Contributor Author

@tilpner https://patch-diff.githubusercontent.com/raw/martanne/abduco/pull/22.patch

Code looks like it just adds priority fallbacks to XDG spec. Would you have access to the XDG vars if started as a user process instead of a system process? Where did you expect it to be, $HOME wouldn’t make sense not as a user… fall back to /tmp?

@tilpner

tilpner commented Jun 3, 2024

Copy link
Copy Markdown
Member

@toastal

Code looks like it just adds priority fallbacks to XDG spec. Would you have access to the XDG vars if started as a user process instead of a system process?

I don't know if user services have access to XDG_RUNTIME_DIR by default, but I don't think they're a great alternative. Systemd user services start and stop when the user logs in/out, unless you enable lingering, which would then apply to all user services (and I don't think the module even allows you to specify user-specific user services). I think the sandboxing options are also more limited for user services, and of course you can't integrate with system-level targets.

I want a system-level service to start a session that I can then later connect to from an interactive ssh session. For that, abduco needs to pick the same socket directory during both the service session and the ssh session.

Specifying the default XDG_RUNTIME_DIR=/run/user/$UID within the service (which is ugly, because it contains the current UID and can't be specified in systemd.services.foo.environment) doesn't help, because that directory is only created when a user actually logs in, long after the service started and bound itself to a socket. It's not the service's responsibility to create /run/user/$UID either, and changing the location would require also changing it for the ssh session (and other session types).

Where did you expect it to be, $HOME wouldn’t make sense not as a user… fall back to /tmp?

My workaround is actually setting ABDUCO_SOCKET_DIR=$HOME manually for the service and in the ForceCommand/.profile before attaching, but that actually doesn't allow me to specify the exact path, because abduco actually uses $ABDUCO_SOCKET_DIR/abduco/ for socket locations. Ironically, I can now choose between $HOME/.abduco/abduco (double abduco) or $HOME/abduco (not hidden), when the patch intent was to clean up my home directory.

Agreeing on the other items in the fallback order is more difficult, since it would require either changing the environment on the ssh session to unusual values, or unsetting them entirely to reach a later fallback.

Even though I can attach to my session again, I'm not convinced nixpkgs should be carrying this patch, as it might require other users with similar service/ssh splits to rediscover this patch addition upon upgrading a system to 24.05. At least, it should probably come with an edit to the abduco(1) manpage, which currently contains an incomplete socket directory preference list. And/or be mentioned in the release notes, perhaps.

@toastal

toastal commented Jun 3, 2024

Copy link
Copy Markdown
Contributor Author

I’m still not quite getting the ‘why’, just your ‘how’ @tilpner… Whose home directory are you attaching to if you set it to $HOME? Aren’t a lot of systemd services ran by users without home directories such as a user for just the service? I would have assumed it would have been best to set $ABDUCO_SOCKET_DIR to something in /run if it was a system-level service rather than using a $HOME from a not real user account.

I would like a better understanding of this since I wanted to look into switching a NixOS module to abduco from tmux to save a tiny bit on resources… especially since the server spawn into the session isn’t using any of the multiplexing features. In which case I was planning to set $ABDUCO_SOCKET_DIR=/run/$SERVICE/$UID rather than piling up in /run/abduco/$MANY_UIDs.

I still feel like XDG is correct behavior as most applications adhere to it with users also expecting it & this seems more like an edge case. But as I also want this in the near-term future for a system service, I would like to understand as my naïve perspective sees this usage of $HOME as a hack & I want to understand why my perception is wrong.

@tilpner

tilpner commented Jun 4, 2024

Copy link
Copy Markdown
Member

I’m still not quite getting the ‘why’, just your ‘how’ @tilpner

First: I'm not saying $HOME is the philosophically correct place for the socket to be, or that it needs to be universal. I'm open to suggestions, this is just my workaround because the patch broke attachment for me.

Whose home directory are you attaching to if you set it to $HOME? Aren’t a lot of systemd services ran by users without home directories such as a user for just the service?

My service is running as a separate user with a home directory in /var/lib/ for storing its state. Yes, this does not work if you have no $HOME.

But since I'm using ssh (mosh) to attach to the abduco session, I need to have a separate user for key management anyway (not required, but easier). And the service has state, so if I automatically create a home directory for it, then I don't need to separately set up its permissions.

I would have assumed it would have been best to set $ABDUCO_SOCKET_DIR to something in /run if it was a system-level service rather than using a $HOME from a not real user account.

I agree that the socket directory shouldn't be persistent (impermanence helps here, but obviously you can't rely on it in nixpkgs). $HOME is used as a convenient place to put it, that has already been created and had its permissions configured.
I would like to put it in a user-specific place in /run instead, if that place is set up outside the binding service.

I would like a better understanding of this since I wanted to look into switching a NixOS module to abduco from tmux to save a tiny bit on resources… especially since the server spawn into the session isn’t using any of the multiplexing features. In which case I was planning to set $ABDUCO_SOCKET_DIR=/run/$SERVICE/$UID rather than piling up in /run/abduco/$MANY_UIDs.

That works, if you create the /run/$SERVICE directory with the proper permissions prior to starting the service (probably with systemd-tmpfiles). The problem is, that to attach to this socket, the user will have to set their socket directory to a service-specific value instead of a user-specific value (this also rules out systemd's $RUNTIME_DIRECTORY).
In my opinion, running abduco should show all the sessions the current user can attach to, and not require multiple invocations with different socket directories.

Which brings us back to /run/user/$UID/abduco/$socket, which unfortunately does not get created at service start (and I don't want to manually create it, and potentially interfere with whatever does end up creating it).

@toastal

toastal commented Jun 5, 2024

Copy link
Copy Markdown
Contributor Author

The issue I see with this is that it seems you want to list sessions from another users which seems a bit weird. In the past, I have written my own new bin scripts for a module that gets added to my ‘real user’ or system environment named like service-resume or service-attach or service-admin that just runs as a wrapper around sudo -u serviceuser adbuco -a service-name (which could also hold the env var, maybe be nice & echo out how to detach too). Personally I find this UX a bit nicer as abduco then becomes an implementation detail rather than a command one needs to know/remember--a power user of abduco might find it more annoying but I would venture many users wouldn’t care. But even a power user might still prefer the XDG expectation are met for general, non-systemd-service use cases.

Even still, if XDG vars aren’t set, why would abduco not be falling thru to $HOME anyhow if you said the XDG variables don’t exist for systemd?

(Just to reiterate, I’m interested in knowing how this is best done like you for a similar use case I bet, but I have yet to get around to it)

@tilpner

tilpner commented Jun 6, 2024

Copy link
Copy Markdown
Member

The issue I see with this is that it seems you want to list sessions from another users which seems a bit weird.

That's not what I meant. I think abduco should (by default) list all sessions of the current user.
With the ABDUCO_SOCKET_DIR=/run/$SERVICE/$UID suggestion, abduco will only list the sessions related to $SERVICE, even though there might be another service that provides an attachable session to the same user. To get all possible sessions, you'd have to call abduco once per service with different values of $SERVICE in ABDUCO_SOCKET_DIR.

In the past, I have written my own new bin scripts for a module that gets added to my ‘real user’ or system environment named like service-resume or service-attach or service-admin that just runs as a wrapper around sudo -u serviceuser adbuco -a service-name (which could also hold the env var, maybe be nice & echo out how to detach too).

Yes, that would work well, except for discoverability of sessions.

Personally I find this UX a bit nicer as abduco then becomes an implementation detail rather than a command one needs to know/remember--a power user of abduco might find it more annoying but I would venture many users wouldn’t care. But even a power user might still prefer the XDG expectation are met for general, non-systemd-service use cases.

If we decide that uncluttering the home directory is worth having a non-standard socket location, then we should document that decision in the manpage (and possibly release notes).

Even still, if XDG vars aren’t set, why would abduco not be falling thru to $HOME anyhow if you said the XDG variables don’t exist for systemd?

That's the problem: it does, but only for the service. Let's walk through the fallback in both cases:

With the patch, the fallback order is (for some, a user-specific directory will be created inside):

  • $ABDUCO_SOCKET_DIR
  • $XDG_RUNTIME_DIR
  • $XDG_CACHE_HOME
  • $HOME
  • $TMPDIR
  • /tmp/abduco

After booting the server, the service starts and launches an abduco session. None of ABDUCO_SOCKET_DIR, XDG_RUNTIME_DIR, or XDG_CACHE_HOME are set, so it falls back to HOME and
creates a socket in $HOME/.abduco. The usual value /run/user/$UID would be invalid, as it doesn't exist prior to the ssh login.

When I attach from an ssh session, ABDUCO_SOCKET_DIR is not set, but XDG_RUNTIME_DIR is set to /run/user/$UID. abduco then creates/uses a socket directory in /run/user/$UID.

The problem here is not the fallback order in isolation, but that the session creation and attachment happen with different environment variables present.

@toastal

toastal commented Jun 7, 2024

Copy link
Copy Markdown
Contributor Author

Do you think it should always start in /run/user/$UID instead of $HOME in the context of NixOS? This sounds like possibly a better UX altogether than any of the above, no? We could drop the patch & instead wrap binary with a default $ABDUCO_SOCKET_DIR=… to get that behavior.

@tilpner

tilpner commented Jun 8, 2024

Copy link
Copy Markdown
Member

That would reduce surprise by being more consistent, but what about when there is no /run/user/$UID? Just fall through to $HOME//tmp?

The wrapper script must only set ABDUCO_SOCKET_DIR if it was not already provided in the environment, otherwise there is no way to set a custom socket location.

@philiptaron philiptaron changed the title adbuco: add bug/feature patches, add mirror abduco: add bug/feature patches, add mirror Sep 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants