dependencies: add rust #10341

wmitros · 2022-04-07T06:08:08Z

The main reason for adding rust dependency to scylla is the
wasmtime library, which is written in rust. Although there
exist c++ bindings, they don't expose all of its features,
so we want to do that ourselves using rust's cxx.

The patch also includes an example rust source to be used in
c++, and its example use in tests/boost/rust_test.

The usage of wasmtime has been slightly modified to avoid
duplicate symbol errors, but as a result of adding a Rust
dependency, it is going to be removed from configure.py
completely anyway

Signed-off-by: Wojciech Mitros wojciech.mitros@scylladb.com

wmitros · 2022-04-07T06:12:37Z

This patch also requires updating the docker image, but I need a maintainer to do that for me

avikivity · 2022-04-07T09:22:43Z

rust/inc/src/lib.rs

@@ -0,0 +1,9 @@
+#[cxx::bridge(namespace = "rust")]


Missing copyright/license.

I'll pick off just the install-dependencies part since it requires extra work.

It's queued now (9740807), please rebase once it promotes.

avelanarius · 2022-04-07T11:59:17Z

docs/design-notes/rust.md

+to `new_pkg/Cargo.toml`
+3. Add `"new_pkg",` to the workspace members list in `Cargo.toml`
+4. Write your Rust code in `new_pkg/src/lib.rs`
+5. To export a function `fn foo(x: i32) -> i32, add its declaration as follows to the same file


Suggested change

5. To export a function `fn foo(x: i32) -> i32, add its declaration as follows to the same file

5. To export a function `fn foo(x: i32) -> i32`, add its declaration as follows to the same file

denesb · 2022-04-07T12:05:47Z

configure.py

@@ -453,6 +453,7 @@ def find_headers(repodir, excluded_dirs):
    'test/boost/restrictions_test',
    'test/boost/role_manager_test',
    'test/boost/row_cache_test',
+    'test/boost/rust_test',


I don't see this source file in this commit.

Oof, I missed it when adding to the commit, I'm adding it in the rebase

nyh · 2022-04-07T12:31:37Z

configure.py

@@ -1879,6 +1892,8 @@ def configure_abseil(build_dir, mode, mode_config):
                    for src in srcs
                    if src.endswith('.cc')]
            objs.append('$builddir/../utils/arch/powerpc/crc32-vpmsum/crc32.S')
+            if has_wasmtime:
+                objs.append('/usr/lib64/libwasmtime.a')


Why be so specific where libwastime needs to reside? Can't we use "-lwasmtime" to use it?

I'm not exactly sure why, but when using -lwasmtime I was getting duplicate symbol erros when linking. I didn't look that much into it because this wasmtime dependency is going to be removed anyway

Actually, this was when -lwasmtime was added to the libs list, perhaps here in objs this could still work - I'll check

so, did you check it? Any conclusions?

I did check it, but unfortunately it still doesn't work (if I put it in objs ninja wants a rule to make -lwasmtime)

nyh

I don't understand the motivation of this series at all.

The cover letter talked about adding Rust as a "dependency", for "wasmtime" - but I see in the actual commits something very different than just "adding a dependency".
It appears like you're planning to put Rust at an equal footing to C++ in Scylla's source code - i.e. part of Scylla's code will be written in Rust, and as a part of that - making a lot of changes to configure.py (some of which like that wasmtime thing, I didn't understand). None of this is explained in the cover letter, or motivated - why are we doing this? What will we gain from this that will offset the tons of complexity that this will add for future developers?
Moreover, I don't understand if you already have a feature that you plan to do with Rust. Is this wasmtime? Do we really need Rust source code in Scylla to use wasmtime? (I was under the impression we didn't)?

psarna · 2022-04-08T07:06:07Z

I don't understand the motivation of this series at all.

The cover letter talked about adding Rust as a "dependency", for "wasmtime" - but I see in the actual commits something very different than just "adding a dependency". It appears like you're planning to put Rust at an equal footing to C++ in Scylla's source code - i.e. part of Scylla's code will be written in Rust, and as a part of that - making a lot of changes to configure.py (some of which like that wasmtime thing, I didn't understand). None of this is explained in the cover letter, or motivated - why are we doing this? What will we gain from this that will offset the tons of complexity that this will add for future developers? Moreover, I don't understand if you already have a feature that you plan to do with Rust. Is this wasmtime? Do we really need Rust source code in Scylla to use wasmtime? (I was under the impression we didn't)?

We need it if we want the implementation to be production-ready, and if I understand it correctly, we eventually want wasm to become our go-to language for user-defined functions and aggregates. C++ bindings are missing features that are crucial for us, like the ability to run webassembly asynchronously, by forcing the execution to yield every X instructions. We could maintain our own fork of the bindings, but it would be even more cumbersome, and by integrating directly with Rust source code we eliminate two abstraction layers (C++ bindings done on top of pre-compiled C bindings), so the overall implementation will be much less fragile.

avikivity · 2022-04-10T13:16:58Z

I'd also like to see some of the administrative code moved to rust.

wmitros · 2022-04-11T13:24:25Z

Sorry for not explaining the motivation for this patch clearly enough, it's true that it introduces a lot of complexity, but we couldn't come up with a better solution for achieving our goals for the new UDFs.

I've rebased the patch on the commit adding dbuild image and fixed the initial review comments (adding -lwasmtime to objs didn't work after all, so I left it as is)

wmitros · 2022-04-11T14:03:39Z

The last rebases should fix errors in the jenkins build (edit: they didn't)

wmitros · 2022-04-11T16:03:24Z

Turns out that for the automatic tests we're using ./tools/toolchain/dbuild -e PATH=/usr/lib64/ccache:/usr/bin:/usr/sbin to use ccache. In this series I added cxxbridge to /usr/local/bin so it's not in PATH when building the test, which causes it to fail.

@avikivity can we make another new docker image, with cxxbridge in /usr/bin? (we'd have adjust the line cargo install cxxbridge-cmd --root /usr/local in install-dependencies)

This can also be fixed by modifying the commands used in testing (is there any reason we don't include /usr/local/bin:/usr/local/sbin:/bin:/sbin in the PATH?)

avikivity · 2022-04-12T08:32:44Z

Turns out that for the automatic tests we're using ./tools/toolchain/dbuild -e PATH=/usr/lib64/ccache:/usr/bin:/usr/sbin to use ccache. In this series I added cxxbridge to /usr/local/bin so it's not in PATH when building the test, which causes it to fail.

@avikivity can we make another new docker image, with cxxbridge in /usr/bin? (we'd have adjust the line cargo install cxxbridge-cmd --root /usr/local in install-dependencies)

This can also be fixed by modifying the commands used in testing (is there any reason we don't include /usr/local/bin:/usr/local/sbin:/bin:/sbin in the PATH?)

Better to adjust the path. Accepted practice is to allow the package manager to manage /usr/bin.

And it should be adjusted for everyone, not just tests, so that tests don't fail if they are run outside the harness.

benipeled · 2022-04-14T07:43:47Z

@wmitros I ran a byo-job [0] with your patch + fix to the PATH https://github.com/scylladb/scylla-pkg/pull/2855 - it's still running but it looks good.

[0] https://jenkins.scylladb.com/view/master/job/scylla-master/job/byo/job/byo_build_tests_dtest/1384/

scylladb-promoter · 2022-04-14T13:21:02Z

CI state FAILURE - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/95/

piodul · 2022-04-16T20:04:14Z

configure.py

+            command = cxxbridge $in > $out
+            description = RUST_HEADER $out
+        rule rust_lib
+            command = CARGO_HOME=build/rust/.cargo cargo build --release --manifest-path=rust/Cargo.toml --target-dir=build/rust -p ${{pkg}}


This creates the .a library in the build/rust/release/ directory, but other rules expect it to be in the build/{mode}/rust/release/ directory and the build fails for me because of that.

You are building the rust libraries in release profile regardless of the actual scylla build mode. It's not great because most of the time during development (in dev mode) we don't want a fully optimized build and would prefer the compilation time to be short instead. Preferably, we would like to customize the compiler options for each build mode.

I suggest looking into cargo profiles. The dev and release profiles are already defined by default (but might need some tweaking), so you need to define custom debug, sanitize and coverage profiles. The rust_lib could select the appropriate profile with --profile {mode}.

As you are using a cargo workspace, note that the profiles need to be defined in workspace's Cargo.toml, not the crates' Cargo.toml.

This creates the .a library in the build/rust/release/ directory, but other rules expect it to be in the build/{mode}/rust/release/ directory and the build fails for me because of that.

I missed this when fixing another issue, will update soon

I suggest looking into cargo profiles. The dev and release profiles are already defined by default (but might need some tweaking), so you need to define custom debug, sanitize and coverage profiles. The rust_lib could select the appropriate profile with --profile {mode}.

The issue I had with cargo profiles was that I couldn't add the debug profile, and the dev profile was building files in the debug directory. I postponed fixing this problem because there was practically zero rust files to compile for now so it doesn't make a noticeable difference yet, but I was planning to add the profiles alongside the first rust contribution - using rust wasmtime instead of the c++ bindings

piodul · 2022-04-16T20:33:35Z

docs/design-notes/rust.md

+
+## Using Rust in Scylla
+
+To create a Rust module `new_pkg` and use it in a C++ source file:


s/module/package/

"Module" and "package" mean different things in the context of Rust. The correct word here is "package".

wmitros · 2022-04-20T11:19:49Z

I've added some fixes suggested by @piodul in configure.py and the docs, but the patch still doesn't use cargo profiles.

scylladb-promoter · 2022-04-20T11:28:39Z

CI state FAILURE - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/125/

scylladb-promoter · 2022-04-27T08:30:26Z

CI state FAILURE - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/179/

scylladb-promoter · 2022-04-28T01:19:02Z

CI state SUCCESS - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/188/

psarna

looks good, pinged one old question

psarna · 2022-05-05T13:30:08Z

configure.py

@@ -1879,6 +1892,8 @@ def configure_abseil(build_dir, mode, mode_config):
                    for src in srcs
                    if src.endswith('.cc')]
            objs.append('$builddir/../utils/arch/powerpc/crc32-vpmsum/crc32.S')
+            if has_wasmtime:
+                objs.append('/usr/lib64/libwasmtime.a')


so, did you check it? Any conclusions?

The patch includes an example rust source to be used in c++, and its example use in tests/boost/rust_test. Signed-off-by: Wojciech Mitros <wojciech.mitros@scylladb.com>

Using Rust in Scylla is not intuitive, the doc explains the entire process of adding new Rust source files to Scylla. What happens during compilation is also explained. Signed-off-by: Wojciech Mitros <wojciech.mitros@scylladb.com>

wmitros · 2022-05-11T14:50:45Z

The last rebase only fixes a comment

scylladb-promoter · 2022-05-11T17:54:41Z

CI state SUCCESS - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/309/

wmitros force-pushed the rust-dependency branch from a2d4653 to 0bff01d Compare April 7, 2022 06:08

avikivity reviewed Apr 7, 2022

View reviewed changes

avelanarius reviewed Apr 7, 2022

View reviewed changes

denesb reviewed Apr 7, 2022

View reviewed changes

nyh reviewed Apr 7, 2022

View reviewed changes

nyh requested changes Apr 7, 2022

View reviewed changes

wmitros force-pushed the rust-dependency branch from 0bff01d to 36a98d4 Compare April 11, 2022 13:10

wmitros force-pushed the rust-dependency branch 2 times, most recently from 8486094 to 5467aa4 Compare April 11, 2022 14:02

wmitros force-pushed the rust-dependency branch from 5467aa4 to b286873 Compare April 11, 2022 15:53

piodul reviewed Apr 16, 2022

View reviewed changes

wmitros force-pushed the rust-dependency branch from 3aa80f7 to e0c1c96 Compare April 20, 2022 11:15

wmitros force-pushed the rust-dependency branch from e0c1c96 to 4c06b52 Compare April 27, 2022 21:43

psarna approved these changes May 5, 2022

View reviewed changes

wmitros added 2 commits May 11, 2022 16:49

tests: add rust example

4ad012c

The patch includes an example rust source to be used in c++, and its example use in tests/boost/rust_test. Signed-off-by: Wojciech Mitros <wojciech.mitros@scylladb.com>

docs: document rust

cb5d054

Using Rust in Scylla is not intuitive, the doc explains the entire process of adding new Rust source files to Scylla. What happens during compilation is also explained. Signed-off-by: Wojciech Mitros <wojciech.mitros@scylladb.com>

wmitros force-pushed the rust-dependency branch from 4c06b52 to cb5d054 Compare May 11, 2022 14:49

scylladb-promoter closed this in eb6f4cc May 12, 2022

scylladb-promoter merged commit eb6f4cc into scylladb:master May 12, 2022

wmitros mentioned this pull request Jul 21, 2022

Improve compilation and linking of Rust files #11102

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dependencies: add rust #10341

dependencies: add rust #10341

wmitros commented Apr 7, 2022

wmitros commented Apr 7, 2022

avikivity Apr 7, 2022

wmitros Apr 7, 2022

avikivity Apr 7, 2022

avelanarius Apr 7, 2022

denesb Apr 7, 2022

wmitros Apr 7, 2022

nyh Apr 7, 2022

wmitros Apr 7, 2022

wmitros Apr 7, 2022

psarna May 5, 2022

wmitros May 11, 2022

nyh left a comment

psarna commented Apr 8, 2022

avikivity commented Apr 10, 2022

wmitros commented Apr 11, 2022

wmitros commented Apr 11, 2022 •

edited

wmitros commented Apr 11, 2022

avikivity commented Apr 12, 2022

benipeled commented Apr 14, 2022

scylladb-promoter commented Apr 14, 2022

piodul Apr 16, 2022

piodul Apr 16, 2022 •

edited

wmitros Apr 20, 2022

piodul Apr 16, 2022

wmitros Apr 20, 2022

wmitros commented Apr 20, 2022

scylladb-promoter commented Apr 20, 2022

scylladb-promoter commented Apr 27, 2022

scylladb-promoter commented Apr 28, 2022

psarna left a comment

psarna May 5, 2022

wmitros commented May 11, 2022

scylladb-promoter commented May 11, 2022

	5. To export a function `fn foo(x: i32) -> i32, add its declaration as follows to the same file
	5. To export a function `fn foo(x: i32) -> i32`, add its declaration as follows to the same file


		## Using Rust in Scylla

		To create a Rust module `new_pkg` and use it in a C++ source file:

dependencies: add rust #10341

dependencies: add rust #10341

Conversation

wmitros commented Apr 7, 2022

wmitros commented Apr 7, 2022

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

nyh left a comment

Choose a reason for hiding this comment

psarna commented Apr 8, 2022

avikivity commented Apr 10, 2022

wmitros commented Apr 11, 2022

wmitros commented Apr 11, 2022 • edited

wmitros commented Apr 11, 2022

avikivity commented Apr 12, 2022

benipeled commented Apr 14, 2022

scylladb-promoter commented Apr 14, 2022

Choose a reason for hiding this comment

piodul Apr 16, 2022 • edited

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

wmitros commented Apr 20, 2022

scylladb-promoter commented Apr 20, 2022

scylladb-promoter commented Apr 27, 2022

scylladb-promoter commented Apr 28, 2022

psarna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmitros commented May 11, 2022

scylladb-promoter commented May 11, 2022

wmitros commented Apr 11, 2022 •

edited

piodul Apr 16, 2022 •

edited