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

session: implement dbus-broker-session #321

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

dvdhrm
Copy link
Member

@dvdhrm dvdhrm commented Jul 11, 2023

This is an implementation of dbus-broker-session, imitating dbus-run-session. This series integrates Rust code into dbus-broker. It uses meson-cargo to support proper integration of Cargo-based Rust code into the Meson build system, including meson-dist support.

This is only a preview. We likely want to add --scope=session to dbus-broker-launch to prevent systemd-activation from being performed for external sessions. And we likely want to set ´DBUS_SESSION_BUS_ADDRESS` in transient units, rather than relying on the user-session to set it (which would be the wrong address).

Session buses are still rather fragile since systemd decided to enforce one bus per user, rather than one per session. Anyway, we can still make it work for most development use-cases.

Cc: @teg

Add a rust library to the dbus-broker code-base. Pull in the
`meson-cargo` subproject to integrate `Cargo` based Rust code into the
Meson build system.

The integration syntax is a bit awkward, given that Meson does not allow
custom modules or custom functions. Hence, we have to use dict-expansion
to integrate `meson-cargo` in meson.build. While awkward to read, it
works fine.

So far, only a dummy libdbus_broker.a is built, which carries no
modules. Later patches will add actual Rust code.

Signed-off-by: David Rheinsberg <[email protected]>
Add a new binary called `dbus-broker-session`. This is modeled after
`dbus-run-session` and supports the same command-line arguments. On top,
it adds `--dbus-broker=...` as argument to allow specifying a
dbus-broker launcher to use rather than dbus-daemon. Furthermore, by
default, it will use dbus-broker rather than dbus-daemon.

Signed-off-by: David Rheinsberg <[email protected]>
@teg
Copy link
Contributor

teg commented Jul 11, 2023

Love the rust and love the long-requested feature!

Two high level questions: why do you support spawning dbus-daemon as well as bus-broker? I assume the main reason for wanting this as part of bus-broker is that dbus-daemon is not available (otherwise dbus-run-session is perfectly fine)? Seems like a lot of redundant code we could just drop.

If this is a drop in replacement (supports the same commandline options), might want to ship it as dbus-run-session. That way existing scripts etc would "just work" if the package is replaced.

@dvdhrm
Copy link
Member Author

dvdhrm commented Jul 11, 2023

Two high level questions: why do you support spawning dbus-daemon as well as bus-broker? I assume the main reason for wanting this as part of bus-broker is that dbus-daemon is not available (otherwise dbus-run-session is perfectly fine)? Seems like a lot of redundant code we could just drop.

dbus-run-session has a --dbus-daemon=/path/do/dbus-daemon argument. It overrides the default daemon to use. Since dbus-daemon is invoked differently than dbus-broker, I wouldn't know what to do if the argument is passed.

Maybe it is safe to just ignore it? I don't know. I just provided for now, but I would gladly drop it if possible.

If this is a drop in replacement (supports the same commandline options), might want to ship it as dbus-run-session. That way existing scripts etc would "just work" if the package is replaced.

Yeah, I haven't thought too much about packaging. So far you can install both implementations in parallel and I didn't want to break this. Distros can still symlink dbus-run-session. But maybe the best solution is to ship it in a separate package and completely strip it from dbus-daemon. But then, I think it needs to retain the ability to spawn dbus-daemon.

I am open to changing this, just haven't thought too much about the best way forward.

@teg
Copy link
Contributor

teg commented Jul 11, 2023

Two high level questions: why do you support spawning dbus-daemon as well as bus-broker? I assume the main reason for wanting this as part of bus-broker is that dbus-daemon is not available (otherwise dbus-run-session is perfectly fine)? Seems like a lot of redundant code we could just drop.

dbus-run-session has a --dbus-daemon=/path/do/dbus-daemon argument. It overrides the default daemon to use. Since dbus-daemon is invoked differently than dbus-broker, I wouldn't know what to do if the argument is passed.

Ah, fair!

Maybe it is safe to just ignore it? I don't know. I just provided for now, but I would gladly drop it if possible.

If this is a drop in replacement (supports the same commandline options), might want to ship it as dbus-run-session. That way existing scripts etc would "just work" if the package is replaced.

Yeah, I haven't thought too much about packaging. So far you can install both implementations in parallel and I didn't want to break this. Distros can still symlink dbus-run-session. But maybe the best solution is to ship it in a separate package and completely strip it from dbus-daemon. But then, I think it needs to retain the ability to spawn dbus-daemon.

I am open to changing this, just haven't thought too much about the best way forward.

Right, I guess we could always leave the door open for distros to decide how to ship it (if they want the things to be interchangeable or not).

My gut feeling is that dbus-broker-session should conflict with (and replace) dbus-run-session, whether it does that by conflicting with the dbus package, or a sub package containing dbus-run-session I guess could be up to distros.

The way you did it now leaves all options open, looks good to me.

pub struct Config {
pub config_file: Option<OsString>,
pub dbus_broker: Option<OsString>,
pub dbus_daemon: Option<OsString>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be clearer what this can be if we replace

pub dbus_broker: Option<OsString>,
pub dbus_daemon: Option<OsString>,

with

pub dbus: DBusImplementation,

which is defined something like:

pub enum DBusImplementation {
    Broker(Option<OsString>)
    Daemon(OsString)
}

Copy link
Contributor

@teg teg left a comment

Choose a reason for hiding this comment

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

I'm good with this regardless of my nitpick, would love to see it land!


#include <inttypes.h>

extern int32_t dbrk_session_main(int32_t argc, uint8_t **argv);
Copy link

Choose a reason for hiding this comment

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

This isn't correct. int32_t is not guaranteed to be an alias of int, nor is uint8_t necessarily an alias of unsigned char.

  • int is only required to be 16 bits wide, so the return value could be truncated on some weird MCU-like system. (To be fair, shell return values are truncated to unsigned 8 bits anyway, so it wouldn't make any difference.)
  • uint8_t is an optional type and might not be defined at all. (Not a big deal, as it would just fail to compile in the first place.)
  • char is only required to be at least 8 bits wide, not exactly. In theory you could have a system with an 11-bit char or something, in which case a char and uint8_t could have different pointer representations.

I realize these are mostly theoretical concerns that won't matter on any supported platorm. You're also not actually redefining main() itself. Nevertheless I just think it's simply best to do bindings by the book. The increasingly popular use of exact-width types combined with incorrect assumptions about primitive types can turn out to be messy.

Anyway, I'm not a Rust guy myself, but based on a quick google search, I think you should probably use libc::c_int and libc::c_char here.

Copy link
Member Author

Choose a reason for hiding this comment

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

This function refers to:

#[export_name = "dbrk_session_main"]
pub extern "C" fn main(
    argc: i32,
    argv: *const *const u8,
) -> i32 {

I think it matches the Rust function just fine. Please see the Rust-nomicon for details on FFI guarantees of Rust data-types, if unclear.

There is no int, or char, involved, nor any C datatypes that would ask for libc::* types. Am I missing something?

# Cargo Integration
#

cargo = custom_target(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be disabled/skipped at build time? If not, could you please add a meson_options.txt for it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Skip what? And why?

Copy link
Contributor

Choose a reason for hiding this comment

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

The whole dbus-broker-session, to avoid having to patch it out

Copy link
Member Author

Choose a reason for hiding this comment

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

It is part of the same configuration as the launcher. Why would you not want it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because it's Rust and uses Cargo, and that whole ecosystem is just not ready for distributions, I very much do not want to get tangled in maintaining it in Debian

Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing wrong with the PR itself, it's the Cargo ecosystem, which is a mess of unstable APIs, breaking ABIs, version pinning, static linking and vendoring. It's just not fit for purpose for a Linux distribution at this time, and all attempts to shoehorn it in ends up in a gigantic amount of work to create something incredibly fragile that requires constant, massive churn and engineering effort to maintain. It's just not worth it.

There is nothing you can do, really, unless you have a magic wand and can swish it to convince the Rust people that stable APIs/ABI and shared libraries were invented for a reason :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

There is nothing you can do, really, unless you have a magic wand and can swish it to convince the Rust people that stable APIs/ABI and shared libraries were invented for a reason :-)

I get paid full-time to improve Rust and to make it more applicable to application use-cases. I will gladly spend time on trying to improve things. So the more specific you are, the easier it is to figure out what we can do.

Nothing wrong with the PR itself, it's the Cargo ecosystem, which is a mess of unstable APIs, breaking ABIs, version pinning, static linking and vendoring. It's just not fit for purpose for a Linux distribution at this time, and all attempts to shoehorn it in ends up in a gigantic amount of work to create something incredibly fragile that requires constant, massive churn and engineering effort to maintain. It's just not worth it.

I think it is the responsibility of an application developer to audit their dependencies. And I agree, a lot of the ecosystem seems to spend little time rethinking the dependency model. However, I do not necessarily see a structural problem.

For many years, systemd linked all its utility libraries statically. At some point, systemd started shipping its own private libsystemd.so to avoid linking it into each systemd binary. To my knowledge, this is still how things work. How is that different to Cargo? You can certainly achieve the same with Cargo, by linking statically or building your own private shared library.

Distributions seem to be willing to package systemd. So can you explain how using Cargo is different here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Distributions seem to be willing to package systemd. So can you explain how using Cargo is different here?

Because that was not, is not, and will not be a public library used by external projects, so there is no dependency handling hell. It's exclusively private and internal, and split out just to save (a lot of) disk space. No reverse dependency is affected when it's updated, as there aren't any.

Things uploaded to Cargo are public libraries, used independently by many unrelated projects at the same time. So in a distribution you want to ship a single copy, so that there is only one file to update when there is a security vulnerability to fix in a stable release - but this cannot happen in Rust because there's no functional and usable dynamic linking, everything is vendored and statically compiled, so in practice you have N versions of the same library for N projects using it, and they all needs to be updated and rebuilt, and all their reverse dependencies need to be updated and rebuilt, and all their reverse dependencies need to be updated and rebuilt, and...

This is not really feasible at scale of course, and that's why we have shared libraries.

In order to be maintainable in a distribution, maturity and feature level need to at least match the C/C++ ecosystem:

  • Guaranteed stable API and ABI in the standard library, like glibc
  • Shared libraries and dynamic linking as first class citizen and default for all builds, including for the standard library
  • heavily encourage API and ABI stability in public libraries

As things stand, Rust is designed to be as convenient as possible for an individual application developer, at the expense of everybody else, and for large corporations that ship a handful of applications and can afford an army of engineers to handle the constant churn.

But if you have 60.000 applications to ship all together, things just don't scale, and are not intended to, because it's a use case they don't care about - which is fair enough, they have other goals and want to pursue them instead, and they are perfectly entitled to do so.
The problem arises when one tries to shoehorn this model in a Linux distribution, as Rust is just not made for that use case. They are trying in Debian, and the results are as disastrous as expected. Which brings us to dbus-broker - it is really not a end-user app that one installs in a Flatpak or so, and can live just fine as "leaf" in the dependency tree, it's a core system component that needs to be an integral part of the distribution. Hence why I would really appreciate a config knob to disable it - it's of course fine to have optional components.

Copy link
Member Author

Choose a reason for hiding this comment

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

Distributions seem to be willing to package systemd. So can you explain how using Cargo is different here?

Because that was not, is not, and will not be a public library used by external projects, so there is no dependency handling hell. It's exclusively private and internal, and split out just to save (a lot of) disk space. No reverse dependency is affected when it's updated, as there aren't any.

I think you are underestimating the influence of libsystemd. Yes, it is private, but it has found its way into lots of software by simple copy-paste. Many projects would love to make use of it, but they have to copy the pieces they want, since systemd is unwilling to share the code-base. I mean we even modeled some APIs in dbus-broker based on libsystemd.

The Rust ecosystem decided to instead provide infrastructure to share such helpers, even though admitting that stable API was not a requirement.

I very much prefer the Rust ecosystem, where the copy-paste is avoided, and instead a clear dependence is signaled. This at least allows tracking updates and security-fixes, while the C-world just happily leaves copy-pasted code around.

Maybe the copy-paste ecosystem of C is to be blamed, rather than blessing the static-linking of Cargo. But I personally do not believe that it makes much of a difference who we blame.

Things uploaded to Cargo are public libraries, used independently by many unrelated projects at the same time. So in a distribution you want to ship a single copy, so that there is only one file to update when there is a security vulnerability to fix in a stable release - but this cannot happen in Rust because there's no functional and usable dynamic linking, everything is vendored and statically compiled, so in practice you have N versions of the same library for N projects using it, and they all needs to be updated and rebuilt, and all their reverse dependencies need to be updated and rebuilt, and all their reverse dependencies need to be updated and rebuilt, and...

This sounds like something that can easily be automated. Cargo provides all the necessary metadata, but...

This is not really feasible at scale of course, and that's why we have shared libraries.

...I do agree that it can lead to excessive rebuilds and download sizes, as well as expensive dependency calculations. This is inherent to static linking, yes.

In order to be maintainable in a distribution, maturity and feature level need to at least match the C/C++ ecosystem:

* Guaranteed stable API and ABI in the standard library, like glibc

I strongly disagree. The C world never settled on a useful platform API. Pretending glibc provides a fully featured standard library on Linux platforms is something I cannot agree with. If that was true, libsystemd would not need to be the gigantic standard library it is now. The C world survived fine with everyone statically linking their own linked-lists, their own file-system helpers, their own event loops.

Yes, there are attempts to provide stable APIs (eg., glib, qt, ...), but I do not agree that this is a consensus in the C/C++ world.

I really think the Rust ecosystem just did away with the pretend and instead settled on a shared but static standard library.

* Shared libraries and dynamic linking as first class citizen and default for all builds, including for the standard library

* heavily encourage API and ABI stability in public libraries

I think it is the responsibility of a developer to use dependencies that try to retain backwards compatibility. I do not think the Rust ecosystem encourages unstable APIs, and I think the Rust standard library is a perfect example that stable API is certainly attainable in Rust.

Stable ABI does not matter for static linking (at least not for this discussion, I think), so I will ignore it for now. There is an ongoing effort in Rust to support a stable c-rust ABI, btw.

Which brings us to dbus-broker - it is really not a end-user app that one installs in a Flatpak or so, and can live just fine as "leaf" in the dependency tree, it's a core system component that needs to be an integral part of the distribution. Hence why I would really appreciate a config knob to disable it - it's of course fine to have optional components.

Lets get specific: Lets imagine dbus-broker shipping its own Rust crates and using Cargo to build them and publish it on crates.io. These crates will have no dependencies other than the Rust standard library (well, actually core and alloc, not even std). To me this is very similar to what we do with c-util right now, or what systemd does with libsystem.

I do not see how this changes anything from a distribution perspective. Why would the distribution decide to package the Cargo crates separately? It is not like you currently package the c-util dependencies we have separately, do you?

And yeah, if we start to pull in lots and lots of crates from all around the ecosystem, problems will likely pop up. But lets be honest, this would be on us to fix, not on the Rust ecosystem. I think linking all this stuff dynamically would not make it any better. I don't like this habit of pulling in things left and right, but don't blame it on the tools, blame it on the people that do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Distributions seem to be willing to package systemd. So can you explain how using Cargo is different here?

Because that was not, is not, and will not be a public library used by external projects, so there is no dependency handling hell. It's exclusively private and internal, and split out just to save (a lot of) disk space. No reverse dependency is affected when it's updated, as there aren't any.

I think you are underestimating the influence of libsystemd. Yes, it is private, but it has found its way into lots of software by simple copy-paste. Many projects would love to make use of it, but they have to copy the pieces they want, since systemd is unwilling to share the code-base. I mean we even modeled some APIs in dbus-broker based on libsystemd.

The Rust ecosystem decided to instead provide infrastructure to share such helpers, even though admitting that stable API was not a requirement.

I very much prefer the Rust ecosystem, where the copy-paste is avoided, and instead a clear dependence is signaled. This at least allows tracking updates and security-fixes, while the C-world just happily leaves copy-pasted code around.

Maybe the copy-paste ecosystem of C is to be blamed, rather than blessing the static-linking of Cargo. But I personally do not believe that it makes much of a difference who we blame.

This is exactly what I mean when I say that Rust is designed to be as convenient as possible for a single application developer, with no regards whatsoever for anybody else. If somebody wants to copy and paste private code they can do that, it's open source after all, but it's their problem and they get to keep the pieces, and it is a problem, not something to encourage. I very much doubt anybody else is really using internal systemd code outside of the public libsystemd library, I know for sure dbus-broker isn't - otherwise it couldn't be licensed as Apache2 and would be GPL2, for starters.

Things uploaded to Cargo are public libraries, used independently by many unrelated projects at the same time. So in a distribution you want to ship a single copy, so that there is only one file to update when there is a security vulnerability to fix in a stable release - but this cannot happen in Rust because there's no functional and usable dynamic linking, everything is vendored and statically compiled, so in practice you have N versions of the same library for N projects using it, and they all needs to be updated and rebuilt, and all their reverse dependencies need to be updated and rebuilt, and all their reverse dependencies need to be updated and rebuilt, and...

This sounds like something that can easily be automated. Cargo provides all the necessary metadata, but...

This is not really feasible at scale of course, and that's why we have shared libraries.

...I do agree that it can lead to excessive rebuilds and download sizes, as well as expensive dependency calculations. This is inherent to static linking, yes.

It is not automated, and "just automate it" is a fig leaf. It's hugely expensive, in terms of hardware and engineering resources, and all those resources are completely wasted trying to chase after a badly designed ecosystem that reintroduces long-solved problems for no good reasons, instead of being used for something actually useful. It's beyond obvious that the Rust owners do not care one bit about the Linux distributions model, so why should we waste our limited time and resources to accommodate them and fight an impossible, uphill battle? There are so many things to do, and so little time to do them already.

In order to be maintainable in a distribution, maturity and feature level need to at least match the C/C++ ecosystem:

* Guaranteed stable API and ABI in the standard library, like glibc

I strongly disagree. The C world never settled on a useful platform API. Pretending glibc provides a fully featured standard library on Linux platforms is something I cannot agree with. If that was true, libsystemd would not need to be the gigantic standard library it is now. The C world survived fine with everyone statically linking their own linked-lists, their own file-system helpers, their own event loops.

Yes, there are attempts to provide stable APIs (eg., glib, qt, ...), but I do not agree that this is a consensus in the C/C++ world.

I really think the Rust ecosystem just did away with the pretend and instead settled on a shared but static standard library.

One can take a program compiled against glibc, libsystemd or glib from 10 years ago, and run it without issues against today's version of those libraries. That's what having a stable ABI means. Whether they provide enough functionality or not it's completely orthogonal and unrelated - if anybody thinks glibc is missing some APIs, they can just go add them if they wish. They will be subject to the same ABI stability promises.
In Rust, one can't even use the same version twice, as the standard library is hashed in some way.

* Shared libraries and dynamic linking as first class citizen and default for all builds, including for the standard library

* heavily encourage API and ABI stability in public libraries

I think it is the responsibility of a developer to use dependencies that try to retain backwards compatibility. I do not think the Rust ecosystem encourages unstable APIs, and I think the Rust standard library is a perfect example that stable API is certainly attainable in Rust.

Stable ABI does not matter for static linking (at least not for this discussion, I think), so I will ignore it for now. There is an ongoing effort in Rust to support a stable c-rust ABI, btw.

Developers can't do anything if not even the standard library is stable. It would be pointless, so why bother? The ecosystem actively encourage to do the opposite - just vendor, pin and static link. So that's what developers do, it should not be a surprise at all. The responsibility is with those making these overall design decisions, as that's what pushes in a certain direction instead of another.

Which brings us to dbus-broker - it is really not a end-user app that one installs in a Flatpak or so, and can live just fine as "leaf" in the dependency tree, it's a core system component that needs to be an integral part of the distribution. Hence why I would really appreciate a config knob to disable it - it's of course fine to have optional components.

Lets get specific: Lets imagine dbus-broker shipping its own Rust crates and using Cargo to build them and publish it on crates.io. These crates will have no dependencies other than the Rust standard library (well, actually core and alloc, not even std). To me this is very similar to what we do with c-util right now, or what systemd does with libsystem.

I do not see how this changes anything from a distribution perspective. Why would the distribution decide to package the Cargo crates separately? It is not like you currently package the c-util dependencies we have separately, do you?

And yeah, if we start to pull in lots and lots of crates from all around the ecosystem, problems will likely pop up. But lets be honest, this would be on us to fix, not on the Rust ecosystem. I think linking all this stuff dynamically would not make it any better. I don't like this habit of pulling in things left and right, but don't blame it on the tools, blame it on the people that do it.

libsystemd-shared is part of the same source package, it's not separate, so it's the same codebase, the same repository, the same release tarball. If something is part of a different cargo project, then it's a separate project, and needs to be handled separately as such.

And in this PR you are already adding a dependency on an external lirbary, "clap", which itself depends on dozens of other libraries, so it's already out of the question.

If there were zero traces of cargo, and only and exclusively the compiler and its standard library were used, then it could maybe be feasible, even if painful. But as soon as cargo shows up, it's not workable at all, and I will need to either disable it if possible, or patch it out.

@dnicolodi
Copy link
Contributor

Recent Meson releases include support for building Rust targets, including Rust crates as subprojects. Would it be interesting to check whether that is enough to build dbus-broker-session? It it is, although I don't know that much about Rust, I can give it a try.

@dvdhrm
Copy link
Member Author

dvdhrm commented Jan 20, 2024

Recent Meson releases include support for building Rust targets, including Rust crates as subprojects. Would it be interesting to check whether that is enough to build dbus-broker-session? It it is, although I don't know that much about Rust, I can give it a try.

They do not support pulling dependencies from crates.io. You would have to recursively manage all deps as wrap-files. This does not sound feasible to me. But their plan is to eventually auto-generate the wrap-files from crates.io metadata.

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.

None yet

5 participants