scylladb icon indicating copy to clipboard operation
scylladb copied to clipboard

Improve compilation and linking of Rust files

Open wmitros opened this issue 3 years ago • 12 comments

This patch can be treated as a follow-up to #10341, or as a preparation before migrating from the lacking C++ wasmtime bindings to our custom implementation that will use the original Rust crate.

The main changes are the introduction of cargo profiles (equivalents of scylla build modes), fixes related to linking against multiple rust packages and corresponding adjustments in documentation.

Some details are included in the commit messages, but you can read more about using the Rust "CXX" bridge in a foreign build system here and the available cargo profile options here

wmitros avatar Jul 21 '22 12:07 wmitros

I will be away next week, but feel free to comment anyway

wmitros avatar Jul 21 '22 12:07 wmitros

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

scylladb-promoter avatar Jul 21 '22 12:07 scylladb-promoter

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

scylladb-promoter avatar Jul 21 '22 18:07 scylladb-promoter

I'm back from vacations so please review when you have time

wmitros avatar Aug 01 '22 15:08 wmitros

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

scylladb-promoter avatar Aug 01 '22 18:08 scylladb-promoter

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

scylladb-promoter avatar Aug 02 '22 18:08 scylladb-promoter

I don't understand any of this. We have a build running in install-dependencies.sh, why do we need to run it again?

avikivity avatar Aug 03 '22 08:08 avikivity

I don't understand any of this. We have a build running in install-dependencies.sh, why do we need to run it again?

Maybe the naming here is misleading. This patch affects the compilation of source files in the rust directory, while install-dependencies.sh is only used for downloading the rust toolchain needed for the compilation (cargo and a cxxbridge script for generating C++ headers from rust files)

wmitros avatar Aug 03 '22 09:08 wmitros

I noticed cxxbridge allows to remove some code duplication using cxx.h instead of cxx.hh so I added this in the rebase

wmitros avatar Aug 03 '22 12:08 wmitros

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

scylladb-promoter avatar Aug 03 '22 16:08 scylladb-promoter

I don't understand any of this. We have a build running in install-dependencies.sh, why do we need to run it again?

Maybe the naming here is misleading. This patch affects the compilation of source files in the rust directory, while install-dependencies.sh is only used for downloading the rust toolchain needed for the compilation (cargo and a cxxbridge script for generating C++ headers from rust files)

Please try to clarify the wording then so I can understand it.

avikivity avatar Aug 04 '22 09:08 avikivity

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

scylladb-promoter avatar Aug 09 '22 14:08 scylladb-promoter

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

scylladb-promoter avatar Aug 22 '22 11:08 scylladb-promoter

In the last rebases I modified the configure script: because we're creating one library for all rust sources together, I added a list of all rust files that are required for the lib. The targets that require rust sources should now include the related lib.rs file I've also experimented with auto-generating the Cargo.lock file, but once we track it and it's missing, we should probably fail and require that it's pulled instead of generating a new one

wmitros avatar Aug 22 '22 14:08 wmitros

This patch is now also a part of #11351 so if it makes more sense to comment with the context of further changes, it can be done there

wmitros avatar Aug 22 '22 14:08 wmitros

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

scylladb-promoter avatar Aug 22 '22 18:08 scylladb-promoter

Looks reasonable, will defer to @psarna decision.

avikivity avatar Aug 24 '22 08:08 avikivity

Since it's also part of https://github.com/scylladb/scylladb/pull/11351, I think we should close this issue and continue with https://github.com/scylladb/scylladb/pull/11351. The period in which wasm-based UDFs are disabled should be minimized/removed, and the most sure way of that is including these changes along with https://github.com/scylladb/scylladb/pull/11351

psarna avatar Aug 24 '22 09:08 psarna

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

scylladb-promoter avatar Aug 29 '22 18:08 scylladb-promoter

This PR was added as a part of #11351, which just got merged. We can close this now as well

wmitros avatar Jan 09 '23 19:01 wmitros