Add `addr` module #430

elmarco · 2023-08-05T19:58:27Z

Address the following shortcomings of zbus <=3 Address implementation:

the Address enum didn't match the spec well, and didn't allow easily more parameter combination. Use a DBusAddr and a Transport structs with field getters instead. Make optional fields and combinations just work without breaking API.
didn't handle ;-separated addresses at all
did not follow the spec quite rigourously, in particular with percent encoded values
implementation was becoming a mess, mixing parsing, connection handling, os-specifics..

The new implementation doesn't use hashmap (only hashset for optional validation), and try to avoid unnecessary copies.

Fixes #476.

It brings `let else`: https://github.com/rust-lang/rust/blob/master/RELEASES.md#user-content-version-1650-2022-11-03

They will be used next in other modules.

Signed-off-by: Marc-André Lureau <[email protected]>

There is not much you can do with it anymore.

elmarco · 2023-08-11T10:40:25Z

CI is still broken The pkg-config command could not be found., no idea why...

Anyhow, removing the draft status of this PR.

zeenix · 2023-08-11T14:47:15Z

CI is still broken The pkg-config command could not be found., no idea why...

That's fallout from your changes. I pushed revert of your commits and that seem to help. Can you try rebasing on latest main?

zeenix · 2023-08-11T14:48:28Z

no idea why...

The action thinks that everything in cached but in reality, it's not and hence the missing pkg-config error.

.github/workflows/rust.yml

zbus/src/win32.rs

zeenix · 2023-08-11T14:59:14Z

zbus/src/addr/address.rs

+
+/// A bus address.
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub struct DBusAddr<'a> {


Why add DBus prefix? It's part of zbus so it's clear what address we mean.

Commit log is very lacking in details. What's relationship with existing Address etc.

No relationship, it's a rewrite from scratch.

I use DBus prefix, and Addr, for consistency with rust standard library. Everything address-related uses addr consistently, and for types, has a prefix. IpAddr, SocketAddr etc. Address (or Addr) would be too common, and would likely conflict with other modules/crates and require a rename.

for consistency with rust standard library

std lib's context is super generic. That is not the case for zbus.

Address (or Addr) would be too common, and would likely conflict with other modules/crates and require a rename.

As discussed before, the mod hierarchy in Rust makes this a complete non-issue. Users can either import only the parent (or its parent mod) or alias during import. They also can choose not to use imports at all and use the full path.

sigh naming... I have a clear preference for DBusAddr, ToDBusAddr.. over Address, ToAddress. But if you insist, I'll just rename stuff.

zeenix · 2023-08-11T15:02:03Z

zbus/src/connection/raw/macos.rs

@@ -0,0 +1,21 @@
+#![cfg(target_os = "macos")]
+
+use crate::{addr::DBusAddr, process::run, Error, Result};


zb: add/rewrite raw stream connection

Again, a huge bunch of changes with no details in the commit log of what's going on.

zeenix · 2023-08-11T15:05:34Z

zbus/src/win32.rs

@@ -26,7 +26,6 @@ use winapi::{
    },
 };

-use crate::Address;


The commit log says you're deprecating it but you're in fact dropping it.

I see potential for further splitting the changes.

Not dropping, you can still use the legacy Address to parse or manipulate Address. But you can't use it to connect anymore. I am fine with removing it now if you prefer.

I am fine with removing it now if you prefer.

Yes that's also (what I thought) we decided. I do not believe this new DBusAddress is a new API at all. We should just change Address to fit our needs. This is not an API typical users use, especially directly.

I am not sure what you mean by "changing Address to fit our needs".. As I see it, DBusAddr is very different.. there are no simple way to "change" the exisiting code, honestly.

Also addr could be a new zbus_addr crate. It's fairly standalone (only zbus::Error/Result is reused)

Look, we already have a type to represent a dbus address: Address. This is good and we should continue with that. Now that type has a few issues and you already have solution for solving that and we can totally break API here. i-e merge DBusAddress into Address.

Also addr could be a new zbus_addr crate.

Sure but we'll then not want the Address API. The user only needs 1 (main) type here and that's what they should have.

zeenix · 2024-01-26T23:27:24Z

Closing this in favor of #556. One big difference between this PR and #556 is that the latter doesn't make it possible to use Address as a reference type. I was half-way making that happen through a bit of a different approach when I realized that in practice that won't give us anything and typical use will have to allocate anyway. See the commit log of my incomplete attempt.

elmarco · 2024-01-27T10:54:45Z

Closing this in favor of #556. One big difference between this PR and #556 is that the latter doesn't make it possible to use Address as a reference type. I was half-way making that happen through a bit of a different approach when I realized that in practice that won't give us anything and typical use will have to allocate anyway. See the commit log of my incomplete attempt.

You'd need to give me a few days if you want me to compare our approaches. From a quick glance, it seems you broke the API but didn't solve some of the pending issues, such as address list handling.

I find it sad that your main argument to block my proposal was that we shouldn't break API (I really think it was necessary and weak, because addr API isn't really used outside), but then you just did that in your own way and find it more acceptable now, you should have worked with me from there...

And the main argument to close my work is:
"We've to decode the string in multiple properties involved here and the
input string is readonly so we've to allocate new strings anyway. We
could just keep the original string as is and decode on-demand but given
that most uses of Address will see these properties being read later,
that will only delay alloction. Moreover, it will be worse cause then
we'll have to allocate each time the properties in question are ready."

How is that related to this branch?

Also the split between address parsing handling and raw stream connection ("add/rewrite raw stream connection") is cleaner here. All in all, did you seriously look at this and compare it with the existing work and your work?

zeenix · 2024-01-27T13:37:56Z

You'd need to give me a few days if you want me to compare our approaches.

I gave you several months to address the concerns with this PR and you told me that you won't have the time to address the concerns. So I had to do everything myself cause there was so much in your branch that I disagreed with and I don't like fixing others code. Now that I'm done after
many days of work, you want to be involved all of a sudden. 😢

it seems you broke the API

As I told you before, we want to break the API. We're breaking API in multiple places already. As long as the typical user has an easy way to port their code, I'm happy. Address isn't an API typically used directly by users.

didn't solve some of the pending issues

It does make it a nice hierarchy, adds guid and makes it possible to add new properties in the future w/o breaking the API. That addresses #476 pretty well.

such as address list handling.

Pretty sure that can be added on top w/o breaking API. If not, we do a 5.0 at some point.

And the main argument to close my work is:

The main argument is that you didn't finish your work here, despite several months passing by and 4.0 is super late, as it is. So if I address #476 in a separate PR in my own way, this work becomes obsolete, at least for now.

How is that related to this branch?

Since it's very difficult to avoid allocations for strings that need % decoding, your branch is still doing that so the issue I describe in the commit log, is relevant.

Having said that, looking again at your PR, I got some ideas on how I can avoid most allocations at least by decoding to buffers on the stack for some properties at least. However, I'll attempt that after merging my PR. I want to release tomorrow at latest cause Mon onward, I've to focus almost 100% on work-work. If I manage to finish before then, good. Otherwise, it'll have to wait for next API break.

zeenix · 2024-01-27T13:52:57Z

All in all, did you seriously look at this and compare it with the existing work and your work?

Not really. As I told you in my review here, I didn't agree with the overall approach of introducing a secondary address type/hierarchy (which I also said in IM before this PR) so I did at first look into addressing my concerns in this branch but quickly gave up, because I realized I need to start from scratch anyway.

Anyway, if you felt strongly enough about your work getting merged, you had more than sufficient time and reminders to get it done in time. Now it's super late. I'm sorry but that's what will always happen if you don't finish the job.

elmarco force-pushed the addr branch 4 times, most recently from bac8fe0 to f6508dc Compare August 11, 2023 10:13

elmarco added 7 commits August 11, 2023 14:23

⬆️ Bump MSRV to 1.65

81d15bc

It brings `let else`: https://github.com/rust-lang/rust/blob/master/RELEASES.md#user-content-version-1650-2022-11-03

🚨 zb: use or_default(), as suggested by clippy

78a3f97

♻️ zb: export some win32 types

9c593b6

They will be used next in other modules.

🎉 zb: add addr module

241e884

Signed-off-by: Marc-André Lureau <[email protected]>

♻️ zb: add/rewrite raw stream connection

af500ad

♻️ zb: use addr

63a1c9e

🗑️ zb: deprecate Address

1f79b4f

There is not much you can do with it anymore.

elmarco force-pushed the addr branch from f6508dc to 1f79b4f Compare August 11, 2023 10:25

elmarco changed the title ~~Draft: Add addr module~~ Add addr module Aug 11, 2023

zeenix requested changes Aug 11, 2023

View reviewed changes

zeenix closed this Jan 26, 2024

elmarco mentioned this pull request Feb 1, 2024

Use dbus-addr crate #562

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `addr` module #430

Add `addr` module #430

elmarco commented Aug 5, 2023 •

edited by zeenix

Loading

elmarco commented Aug 11, 2023

zeenix commented Aug 11, 2023

zeenix commented Aug 11, 2023

zeenix Aug 11, 2023

elmarco Aug 12, 2023 •

edited

Loading

zeenix Aug 14, 2023

elmarco Aug 14, 2023

zeenix Aug 11, 2023

elmarco Aug 12, 2023

zeenix Aug 11, 2023

elmarco Aug 12, 2023

zeenix Aug 14, 2023

elmarco Aug 14, 2023

elmarco Aug 14, 2023

zeenix Aug 15, 2023

zeenix commented Jan 26, 2024

elmarco commented Jan 27, 2024 •

edited

Loading

zeenix commented Jan 27, 2024

zeenix commented Jan 27, 2024

		@@ -0,0 +1,21 @@
		#![cfg(target_os = "macos")]

		use crate::{addr::DBusAddr, process::run, Error, Result};

Add addr module #430

Add addr module #430

Conversation

elmarco commented Aug 5, 2023 • edited by zeenix Loading

elmarco commented Aug 11, 2023

zeenix commented Aug 11, 2023

zeenix commented Aug 11, 2023

Choose a reason for hiding this comment

elmarco Aug 12, 2023 • edited Loading

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

zeenix commented Jan 26, 2024

elmarco commented Jan 27, 2024 • edited Loading

zeenix commented Jan 27, 2024

zeenix commented Jan 27, 2024

Add `addr` module #430

Add `addr` module #430

elmarco commented Aug 5, 2023 •

edited by zeenix

Loading

elmarco Aug 12, 2023 •

edited

Loading

elmarco commented Jan 27, 2024 •

edited

Loading