Rust integration: Adding variables & clients. #102

SirVer · 2023-10-10T14:10:25Z

Co-authored-by: Dima Dorezyuk ddo@qwello.eu
Signed-off-by: Holger Rapp hra@qwello.eu

SirVer · 2023-10-10T14:13:10Z

@hikinggrass Kai, could you run clang-format for me again, please? I still do not have access to a Linux dev machine.

Update: Figured it out.

SirVer · 2023-10-10T14:18:48Z

We updated also EVerest/everest-core#344 to work with this new code.

SirVer · 2023-10-16T08:22:32Z

@hikinggrass @a-w50 Friendly ping? This has been sitting for a week now.

hikinggrass · 2023-10-16T11:46:19Z

@hikinggrass @a-w50 Friendly ping? This has been sitting for a week now.

I'm still working on the cmake integration, but I hope to have something to push sometime today

hikinggrass · 2023-10-16T21:56:24Z

@hikinggrass @a-w50 Friendly ping? This has been sitting for a week now.

I'm still working on the cmake integration, but I hope to have something to push sometime today

This (in combination with the changes I just pushed on everest-core) should allow rust modules to be built with cmake without having to manually specify the location of everest-framework or to rely on heuristics. There have been some changes to the config handling in everest-framework recently which I've already rebased against.
The cmake support isn't completely ready yet and still needs some adjusting before merging.

One thing I've noticed with the rust modules in general: it looks like they do not yet publish true to everest/<module_id>/ready once their initialization code has run and triggering this manually doesn't seem to call the on_ready handler yet. That's probably something we should fix before merging this as well.

a-w50 · 2023-10-17T07:28:51Z

everestrs/everestrs/src/lib.rs

+    pub fn set_subscriber(&self, sub_impl: Weak<dyn Subscriber>) {
+        *self.sub_impl.write().unwrap() = Some(sub_impl);


from an api perspective, set_subscriber can only be called once, right? If so, this should belong into the constructor.

The better place would be the ctor but for our user facing api we have following dependencies

The publishers need a ref to the runtime

the subscribers need a ref to the publishers

The runtime needs a ref to the subscribers.

to brake this vicious cycle we pass the subscribers after the construction to the runtime

have a look at the core or for how it works together

a-w50 · 2023-10-17T07:31:43Z

everestrs/everestrs/src/lib.rs

+                self.cpp_module
+                    .as_ref()
+                    .unwrap()
+                    .provide_command(self, implementation_id.clone(), name);


Question: The runtime (self) is passed here via provide_command. Are rust references direct pointers, i.e. do they point to a specific memory location? If so, this makes the Runtime class non-moveable, correct?

This is a good point. But we have to pin not the subscriber reference but the runtime. The cxx bridge and set_subscribers should probably take Pin since this is the point from which we aren’t allowed to move. By not implementing Unpin we make sure that the runtime remains pinned once the set_subscriber is called

a-w50 · 2023-10-17T07:33:43Z

everestrs/everestrs_sys/everestrs_sys.cpp

        return json::parse(blob.data.begin(), blob.data.end());
    });
 }

+void Module::subscribe_variable(const Runtime& rt, rust::String implementation_id, rust::String name) const {
+    // TODO(hrapp): I am not sure how to model the multiple slots that could theoretically be here.


@SirVer : what do you mean by multiple slots?

@a-w50 I mean that num_connections != 1 for this implementation_id. I did not understand the C++ code around this, hence I did not model it in Rust at all currently. My understanding is that the c++ code initially assumed that there would only be one connection, but then this was extendended to allow for more than one connection. To me this is a topic that needs addressing, but can be done at a later point. I also need a clinic where somebody explains this to me (or @dorezyuk, whoever implements this for rust).

a-w50 · 2023-10-17T07:39:40Z

everestrs/everestrs/build.rs

@@ -7,6 +7,10 @@ struct Libraries {
 }

 fn find_everest_workspace_root() -> PathBuf {
+    if let Ok(everest_framework_dir) = env::var("EVEREST_RS_FRAMEWORK_SOURCE_LOCATION") {


The naming is a bit off here, is it everest framework dir OR everestrs framework source location?

My intention with naming here was that this environment variable is meant for everestrs and contains the location to the source of everest-framework

a-w50 · 2023-10-17T07:42:05Z

everestrs/everestrs/build.rs

+        if let Ok(everest_framework_dir) = env::var("EVEREST_RS_FRAMEWORK_BINARY_LOCATION") {
+            let everest_framework_path = PathBuf::from(everest_framework_dir);
+            (
+                everest_framework_path.join("everestrs/libeverestrs_sys.a"),
+                everest_framework_path.join("lib/libframework.so"),
+            )
+        } else {
+            (
+                root.join("everest-framework/build/everestrs/libeverestrs_sys.a"),
+                root.join("everest-framework/build/lib/libframework.so"),
+            )
+        };


Not sure about rust best practices, but for me this if let Ok(..) = ... { doesn't naturally read. Also the implicit returns are hard to spot (but could just be me ...)

nit: I'd prefer a match statement here too. The implicit returns are rustic - i.e. idiomatic. They are like meaningful whitespace in Python - one first finds them appalling, once used to them they become rather nice.

a-w50 · 2023-10-17T07:43:36Z

everestrs/everestrs/build.rs

-    let everestrs_sys = root.join("everest-framework/build/everestrs/libeverestrs_sys.a");
-    let framework = root.join("everest-framework/build/lib/libframework.so");
+    let (everestrs_sys, framework) =
+        if let Ok(everest_framework_dir) = env::var("EVEREST_RS_FRAMEWORK_BINARY_LOCATION") {


Compare the naming:
if let Ok(everest_framework_dir) = env::var("EVEREST_RS_FRAMEWORK_SOURCE_LOCATION") {
and
if let Ok(everest_framework_dir) = env::var("EVEREST_RS_FRAMEWORK_BINARY_LOCATION") {

we do need the binary location here, not the source location, which are usually not identical

a-w50 · 2023-10-17T07:47:39Z

everestrs/everestrs/build.rs

-    let everestrs_sys = root.join("everest-framework/build/everestrs/libeverestrs_sys.a");
-    let framework = root.join("everest-framework/build/lib/libframework.so");
+    let (everestrs_sys, framework) =
+        if let Ok(everest_framework_dir) = env::var("EVEREST_RS_FRAMEWORK_BINARY_LOCATION") {


If you pass locations by environment variables, why don't you pass the the complete path to libeverestrs_sys.a and libframework.so directly, this would be more stable. Note that at least inside cmake you can get the full path to library-targets (using generator expressions: FRAMEWORK_LIB=$<TARGET_FILE:everest::framework>)

Absolutely, would add yet another environment variable but I guess the cost of that is quite low

a-w50 · 2023-10-17T07:50:54Z

src/manager.cpp

@@ -199,6 +200,26 @@ static SubprocessHandle exec_cpp_module(const ModuleStartInfo& module_info, std:
    return handle;
 }

+static SubprocessHandle exec_rust_module(const ModuleStartInfo& module_info, std::shared_ptr<RuntimeSettings> rs) {


There is really no point here in passing the RuntimeSettings as shared pointer, just pass it as a (ideally const) reference.

At the moment this is just to keep it in line with the other exec_* functions, I would suggest we refactor this in a new PR

Add a TODO then for now so we do not forget? Reference this MR for posterity.

src/manager.cpp

SirVer · 2023-10-18T17:53:18Z

I did a flyby of the code, I have little time this week, so might not be able to dive deeper - thanks for the work and review so far!

SirVer · 2023-11-03T16:50:23Z

@hikinggrass What is this still waiting on?

Co-authored-by: Dima Dorezyuk <ddo@qwello.eu> Signed-off-by: Holger Rapp <hra@qwello.eu> Signed-off-by: Holger Rapp <HolgerRapp@gmx.net>

Signed-off-by: Holger Rapp <HolgerRapp@gmx.net>

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>

Signed-off-by: aw <aw@pionix.de>

- clean target still needs to be fixed Signed-off-by: aw <aw@pionix.de>

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>

.

hikinggrass · 2023-11-03T17:14:05Z

@hikinggrass What is this still waiting on?

just a rebase, I'm squashing right now :)

SirVer · 2023-11-06T09:11:53Z

❤️ ❤️ 🎆 H O O R A Y 🚀 ❤️ ❤️

SirVer force-pushed the hrapp/fleshed_out_rust branch from 3a76180 to 9cf60a7 Compare October 10, 2023 14:12

SirVer force-pushed the hrapp/fleshed_out_rust branch from 847a985 to 157efa7 Compare October 10, 2023 14:27

hikinggrass force-pushed the hrapp/fleshed_out_rust branch from 17935bc to be7eab6 Compare October 16, 2023 21:40

a-w50 previously requested changes Oct 17, 2023

View reviewed changes

hikinggrass force-pushed the hrapp/fleshed_out_rust branch from af1c7fb to 31c92f8 Compare November 2, 2023 11:47

hikinggrass approved these changes Nov 2, 2023

View reviewed changes

hikinggrass requested a review from a-w50 November 3, 2023 16:51

SirVer and others added 12 commits November 3, 2023 17:52

Rust integration: Adding variables & clients.

a0826bb

Co-authored-by: Dima Dorezyuk <ddo@qwello.eu> Signed-off-by: Holger Rapp <hra@qwello.eu> Signed-off-by: Holger Rapp <HolgerRapp@gmx.net>

Ran clang-format.

c4b9056

Signed-off-by: Holger Rapp <HolgerRapp@gmx.net>

Fix README formatting.

bbbfda6

Signed-off-by: Holger Rapp <HolgerRapp@gmx.net>

Fix RuntimeSettings usage in everestrs after recent changes

b74694d

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>

Use environment variables to find libeverestrs_sys.a and libframework.so

b01e92b

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>

Make manager aware of rust binaries

7b6d8e2

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>

use Arc::pin

88e1c44

Remove special handling for rust binaries, re-use normal binary loading

167c3b2

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>

Rename cpp_binary_path to binary_path

8afcad0

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>

Necessary changes for everest core

52723a0

Signed-off-by: aw <aw@pionix.de>

Minor tweaks

0197d68

- clean target still needs to be fixed Signed-off-by: aw <aw@pionix.de>

Relect recent cmake changes in the README

14b5a1b

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>

hikinggrass force-pushed the hrapp/fleshed_out_rust branch from c7ff891 to 14b5a1b Compare November 3, 2023 17:12

hikinggrass merged commit 49f5f30 into EVerest:main Nov 3, 2023
1 of 3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust integration: Adding variables & clients. #102

Rust integration: Adding variables & clients. #102

SirVer commented Oct 10, 2023 •

edited

Loading

SirVer commented Oct 10, 2023 •

edited

Loading

SirVer commented Oct 10, 2023

SirVer commented Oct 16, 2023

hikinggrass commented Oct 16, 2023

hikinggrass commented Oct 16, 2023

a-w50 Oct 17, 2023

dorezyuk Oct 18, 2023

a-w50 Oct 17, 2023

dorezyuk Oct 18, 2023

a-w50 Oct 17, 2023

SirVer Oct 18, 2023

a-w50 Oct 17, 2023

hikinggrass Oct 17, 2023

a-w50 Oct 17, 2023

SirVer Oct 18, 2023

a-w50 Oct 17, 2023

hikinggrass Oct 17, 2023

a-w50 Oct 17, 2023

hikinggrass Oct 17, 2023

a-w50 Oct 17, 2023

hikinggrass Oct 17, 2023

SirVer Oct 18, 2023

SirVer commented Oct 18, 2023

SirVer commented Nov 3, 2023

hikinggrass commented Nov 3, 2023

SirVer commented Nov 6, 2023

		pub fn set_subscriber(&self, sub_impl: Weak<dyn Subscriber>) {
		*self.sub_impl.write().unwrap() = Some(sub_impl);

Rust integration: Adding variables & clients. #102

Rust integration: Adding variables & clients. #102

Conversation

SirVer commented Oct 10, 2023 • edited Loading

SirVer commented Oct 10, 2023 • edited Loading

SirVer commented Oct 10, 2023

SirVer commented Oct 16, 2023

hikinggrass commented Oct 16, 2023

hikinggrass commented Oct 16, 2023

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

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

SirVer commented Oct 18, 2023

SirVer commented Nov 3, 2023

hikinggrass commented Nov 3, 2023

SirVer commented Nov 6, 2023

SirVer commented Oct 10, 2023 •

edited

Loading

SirVer commented Oct 10, 2023 •

edited

Loading