Rewrite ::address (import from dbus_addr) by elmarco · Pull Request #989 · z-galaxy/zbus

elmarco · 2024-09-15T18:59:38Z

Based on the discussion from #982, import dbus_addr as zbus_address and rebase #562 to make use of it.

zeenix · 2024-09-15T19:34:58Z

Thanks! I'll have a look soon. In the meantime, could you please rebase this on top of #976? That one is just waiting for a test case to be added.

zeenix

You'll need to change some fundamental design here to make it acceptable to me, sorry.

zbus/src/error.rs

zbus_address/README.md

zbus_address/src/lib.rs

zbus_address/src/address.rs

zbus_address/src/address_list.rs

zbus_address/src/address.rs

zeenix · 2024-09-17T11:45:54Z

I mainly see only 2 blockers here now:

The split. As discussed before, I'm not at all convinced a split is worth it in practical terms. We should instead make use of cargo features to make zbus modular instead.
The naming.

elmarco · 2024-09-17T11:48:01Z

I mainly see only 2 blockers here now:

* The split. As discussed before, I'm not at all convinced a split is worth it in practical terms. We should instead make use of cargo features to make zbus modular instead.

* The naming.

I disagree with you on those two points, but if it is what it takes to make progress...

zeenix · 2024-09-17T12:12:04Z

I disagree with you on those two points

That's well established. :)

but if it is what it takes to make progress...

I'm sorry but yeah. You've to keep in mind that I did try my best to explain my reasoning in detail. I regret that I failed to convince you in the end.

zeenix · 2024-09-18T22:19:27Z

@elmarco is this ready for review?

elmarco · 2024-09-19T12:00:13Z

@elmarco is this ready for review?

yes please

zbus/src/address/address_list.rs

zbus/src/address/percent.rs

zbus/src/address/address.rs

zeenix

Found a few more things, mostly minor nits.

zbus/src/address/transport/mod.rs

zbus/src/address/transport/systemd.rs

zbus/src/address/transport/unix.rs

zbus/src/address/transport/unixexec.rs

zbus/src/address/transport.rs

zbus/src/address/transport/nonce_tcp.rs

zeenix

Let's get it in but please fix the remaining minor things in a follow-up commit. 🙏

zbus/src/address/transport/mod.rs

zeenix · 2024-09-29T12:02:23Z

Seems it needs another rebase, so you might as well fix the minor issues while you rebase.

Parsing can be done with legacy Address and DBusAddr

Connecting to an address can recursively resolve to a different address to connect to. Because it's an async function, box it.

As requested by Zeeshan.

Some addresses should only be used on specific target OS. This makes the address handling specific to the target, which will make its usage impractical for some use cases (web, configuration etc) As requested by Zeeshan.

As requested by Zeenshan.

zeenix · 2024-10-02T12:09:17Z

zbus/src/address/transport/unixexec.rs

+#[derive(Debug, PartialEq, Eq)]
+pub struct Unixexec<'a> {
+    path: Cow<'a, OsStr>,
+    argv: Vec<(usize, Cow<'a, str>)>,


why keep the index in a Vec? Vec and slices are indexed already.

First, because you may or may not have argv0...

You may also have multiple argvN (since we don't enforce this anymore)

And also the spec says " If a specific argvX is not specified no further argvY for Y > X are taken into account." which may be interepreted as the address may be accepted with argvX missing but further argvY.. in which case you should ignore them for exec, but you may still want to retain them?..

First, because you may or may not have argv0...

That just means that argv0 needs special treatment (sperate field/getter).

But nm. I didn't remember that args are specified as key=value, with key as argN..

Unresolving this thread for this part:

First, because you may or may not have argv0...

That just means that argv0 needs special treatment (sperate field/getter).

This is also closer to the spec since arg0 is separate key.

zeenix · 2024-10-02T12:10:29Z

zbus/src/connection/connect/win32.rs

+};
+
+pub(crate) async fn connect(
+    l: &crate::address::transport::Autolaunch<'_>,


I'm sure you can come up with a better name than l.

autolaunch isn't super long. 😆

zbus/src/address/transport/vsock.rs

zbus/src/error.rs

zbus/src/connection/connect/mod.rs

This reverts commit fc03642, reversing changes made to deffbb8. It turns out there was a major design flaw in the API: the parsed address string was not kept in a parsed state, resulting in reparsing of the string form, for each getter. Because of this, I decided to revert the changes for 5.0.

⏪️ Revert "Merge pull request #989 from elmarco/address"

zeenix requested changes Sep 16, 2024

View reviewed changes

elmarco force-pushed the address branch 6 times, most recently from a0f3be2 to 8b9f688 Compare September 18, 2024 09:34

elmarco changed the title ~~Introduce zbus_address & use it~~ Rewrite ::address (import from dbus_addr) Sep 18, 2024

elmarco force-pushed the address branch from 8b9f688 to 8de41b3 Compare September 18, 2024 10:07

elmarco mentioned this pull request Sep 18, 2024

✨ zb: Add support for unixexec transport #976

Merged

elmarco force-pushed the address branch from 8de41b3 to 34a64e8 Compare September 18, 2024 11:43

elmarco force-pushed the address branch from 34a64e8 to cf24d60 Compare September 19, 2024 12:01

zeenix reviewed Sep 19, 2024

View reviewed changes

zbus/src/address/address_list.rs Show resolved Hide resolved

zeenix requested changes Sep 20, 2024

View reviewed changes

zbus/src/address/percent.rs Show resolved Hide resolved

zbus/src/address/address.rs Outdated Show resolved Hide resolved

elmarco force-pushed the address branch from cf24d60 to 1e710dc Compare September 20, 2024 12:21

zeenix requested changes Sep 22, 2024

View reviewed changes

zeenix requested changes Sep 23, 2024

View reviewed changes

zbus/src/address/transport.rs Show resolved Hide resolved

zbus/src/address/transport.rs Show resolved Hide resolved

elmarco force-pushed the address branch 4 times, most recently from 11f4e9d to f3e5d25 Compare September 24, 2024 11:04

zeenix reviewed Sep 29, 2024

View reviewed changes

zbus/src/address/transport/nonce_tcp.rs Show resolved Hide resolved

zeenix approved these changes Sep 29, 2024

View reviewed changes

zbus/src/address/transport/mod.rs Outdated Show resolved Hide resolved

elmarco added 12 commits October 1, 2024 14:48

♻️ zb: handle Vsock transport connection (without legacy_address)

1a93abd

♻️ zb: handle Launchd transport connection (without legacy_address)

36020eb

♻️ zb: move Launchd connection test

c62289d

♻️ zb: make windows_autolaunch_bus_address() return String

bcaad6e

Parsing can be done with legacy Address and DBusAddr

✨ zb: make connect() recursively callable

4d2cdfd

Connecting to an address can recursively resolve to a different address to connect to. Because it's an async function, box it.

♻️ zb: handle win32 Autolaunch connection (without legacy_address)

a9dbca6

♻️ zb: move win32 Autolaunch connection test

00b2c35

🔥 zb: drop legacy_address module

c2c6d42

➖ zb: drop async-recursion dependency

dec224c

🚚 zb: rename DBusAddr to Address

979019c

As requested by Zeeshan.

🩹 zb: make address parsing target-specific

2dc80c4

Some addresses should only be used on specific target OS. This makes the address handling specific to the target, which will make its usage impractical for some use cases (web, configuration etc) As requested by Zeeshan.

🚚 zb: src/connection/connect.rs ⟶ src/connection/connect/mod.rs

0b4362b

As requested by Zeenshan.

elmarco force-pushed the address branch from dd97091 to 0b4362b Compare October 1, 2024 10:48

zeenix enabled auto-merge October 1, 2024 11:50

zeenix disabled auto-merge October 1, 2024 11:51

zeenix merged commit fc03642 into z-galaxy:main Oct 1, 2024

zeenix mentioned this pull request Oct 2, 2024

Remove ::address #982

Closed

zeenix reviewed Oct 2, 2024

View reviewed changes

zbus/src/address/transport/vsock.rs Show resolved Hide resolved

zeenix reviewed Oct 2, 2024

View reviewed changes

zbus/src/error.rs Show resolved Hide resolved

zeenix mentioned this pull request Oct 2, 2024

Fix connection breakage #1036

Merged

zeenix reviewed Oct 2, 2024

View reviewed changes

zbus/src/connection/connect/mod.rs Show resolved Hide resolved

This was referenced Oct 7, 2024

Refactoring as requested #1050

Closed

Remaining issues with new address API #1064

Closed

zeenix added a commit that referenced this pull request Oct 14, 2024

Merge pull request #1068 from zeenix/revert-address-changes

d85ef96

⏪️ Revert "Merge pull request #989 from elmarco/address"

Uh oh!

Comments

Conversation

elmarco commented Sep 15, 2024

Uh oh!

zeenix commented Sep 15, 2024

Uh oh!

zeenix left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zeenix commented Sep 17, 2024

Uh oh!

elmarco commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeenix commented Sep 17, 2024

Uh oh!

zeenix commented Sep 18, 2024

Uh oh!

elmarco commented Sep 19, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zeenix left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zeenix left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zeenix commented Sep 29, 2024

Uh oh!

zeenix Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

elmarco Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

zeenix Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

zeenix Oct 3, 2024

Choose a reason for hiding this comment

Uh oh!

zeenix Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

elmarco Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

zeenix Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

elmarco commented Sep 17, 2024 •

edited

Loading