Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ffsync: Update syncstorage-rs to v0.17.15 #6228

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mreid-tt
Copy link
Contributor

@mreid-tt mreid-tt commented Sep 12, 2024

Description

This PR includes the following:

  1. Update syncstorage-rs to v0.17.15

Fixes #

Checklist

  • Build rule all-supported completed successfully
  • New installation of package completed successfully
  • Package upgrade completed successfully (Manually install the package again)
  • Package functionality was tested
  • Any needed documentation is updated/created

Type of change

  • Package update

@mreid-tt mreid-tt self-assigned this Sep 12, 2024
@mreid-tt mreid-tt added the build/rust Requires rust build system label Sep 12, 2024
@mreid-tt
Copy link
Contributor Author

hey @hgy59, I was having another go at this further to the challenges of upgrading diesel in #6187. Looking deeper at the errors originally we had this:

error: failed to run custom build command for `mysqlclient-sys v0.4.1`

Caused by:
  process didn't exit successfully: `/github/workspace/spk/ffsync/work-aarch64-7.1/diesel-2.2.3/target/release/build/mysqlclient-sys-257caf6ec025a9da/build-script-build` (exit status: 101)
  --- stderr
  thread 'main' panicked at /github/workspace/distrib/cargo/registry/src/index.crates.io-6f17d22bba15001f/mysqlclient-sys-0.4.1/build.rs:91:5:
  Did not find a compatible version of libmysqlclient.
  Ensure that you installed one and teached mysqlclient-sys how to find it
  You have the following options for that:

  * Use `pkg_config` to automatically detect the right location
  * Use vcpkg to automatically detect the right location. 
  You also need to set `MYSQLCLIENT_VERSION` to specify which
  version of libmysqlclient you are using
  * Set the `MYSQLCLIENT_LIB_DIR` and `MYSQLCLIENT_VERSION` environment 
  variables to point the compiler to the right directory and specify 
  which version is used
  * Make the `mysql_config` binary avaible in the environment that invokes
  the compiler
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
warning: build failed, waiting for other jobs to finish...
error: failed to compile `diesel_cli v2.2.3 (/github/workspace/spk/ffsync/work-aarch64-7.1/diesel-2.2.3/diesel_cli)`, intermediate artifacts can be found at `/github/workspace/spk/ffsync/work-aarch64-7.1/diesel-2.2.3/target`.

If we focus on the stderr it provides some guidance on this. First there is the declaration of MYSQLCLIENT_LIB_DIR and MYSQLCLIENT_VERSION environment variables. Checking with our code for diesel we already have one of them set:

ENV += MYSQLCLIENT_LIB_DIR=$(STAGING_INSTALL_PREFIX)/lib

So the next thing I did was set the other one in f52e0ea and this revealed a new error:

  --- stderr
  thread 'main' panicked at /github/workspace/distrib/cargo/registry/src/index.crates.io-6f17d22bba15001f/mysqlclient-sys-0.4.1/build.rs:230:13:
  mysqlclient-sys does not provide bundled bindings for libmysqlclient `6.1.11` for the target `aarch64-unknown-linux-gnu`.
                   Consider using the `buildtime_bindgen` feature or contribute bindings to the crate
  Debug information: (version: None, target_arch: aarch64, ptr_size: 64)
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
warning: build failed, waiting for other jobs to finish...
error: failed to compile `diesel_cli v2.2.4 (/github/workspace/spk/ffsync/work-aarch64-7.1/diesel-2.2.4/diesel_cli)`, intermediate artifacts can be found at `/github/workspace/spk/ffsync/work-aarch64-7.1/diesel-2.2.4/target`.

This concurs with what I see in the mysqlclient-sys build code. So the next strategy was to use the buildtime_bindgen feature which I implemented in 7b0b200. This resulted in a very different error as follows:

  --- stderr
  thread 'main' panicked at /github/workspace/distrib/cargo/registry/src/index.crates.io-6f17d22bba15001f/bindgen-0.69.4/lib.rs:622:31:
  Unable to find libclang: "couldn't find any valid shared libraries matching: ['libclang.so', 'libclang-*.so', 'libclang.so.*', 'libclang-*.so.*'], set the `LIBCLANG_PATH` environment variable to a path where one of these files can be found (invalid: [])"
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
warning: build failed, waiting for other jobs to finish...
error: failed to compile `diesel_cli v2.2.4 (/github/workspace/spk/ffsync/work-aarch64-7.1/diesel-2.2.4/diesel_cli)`, intermediate artifacts can be found at `/github/workspace/spk/ffsync/work-aarch64-7.1/diesel-2.2.4/target`.

So based on this it seems to be trying to dynamically create the bindings for libmysqlclient 6.1.11 but is missing libclang. I don't know if this is something you can possibly assist with?

Alternately, if libclang is only used to generate the binding we can perhaps upgrade to libmysqlclient 8.4 which already has a supported binding.

This is all a bit of guesswork on my part but let me know if I may be onto something.

@mreid-tt
Copy link
Contributor Author

mreid-tt commented Sep 13, 2024

@hgy59, further to the research I did above, I was looking more into the buildtime_bindgen feature and in the readme for mysqlclient-sys we see the following:

The buildtime_bindgen feature allows you to generate bindings at build time matching your locally installed libmysqlclient version. It uses pkg-config, vcpkg, mysql_config or the MYSQLCLIENT_INCLUDE_DIR variable to determine the location of your mysql.h header. Additional bindgen configurations can be provided by setting the BINDGEN_EXTRA_CLANG_ARGS variable.

Looking into that variable mentioned at the end I find it listed in the readme for the rust-bindgen repo. This has the note:

BINDGEN_EXTRA_CLANG_ARGS: extra arguments to pass to clang.

Now in that repo there is an issue rust-lang/rust-bindgen#1949 which advises that:

Currently, bindgen uses libclang to understand C/C++ code.

As such, it doesn't support gcc which is why we need to provide clang for it to work. I've been looking at our wiki and it's not clear to me how this would be done. Searching the codebase I only come across this mention of clang in the spk/synocli-devel/Makefile. It seems to be including lines like these:

OPTIONAL_DEPENDS = cross/llvm
OPTIONAL_DESC += ", LLVM 16.0.6"
SPK_COMMANDS += bin/clang-16

I don't have a firm grasp on this so I would need some guidance in getting this up and running.

EDIT: After exploring the option of upgrading libmysqlclient, I now understand why it may not be straightforward. The Makefile points to MySQL Connector/C, with the latest version being 6.1.11. Upgrading beyond this would require switching to a different source, MySQL Connector/C++, which is currently at version 8.4.0. Since it uses a different programming language, this would likely involve creating a new package rather than performing a simple upgrade.

@th0ma7
Copy link
Contributor

th0ma7 commented Sep 13, 2024

Hoping not to muddy the water but is that libclang from an host or target perspective? i.e. using clang to build? or to link against its library at cross-compile time?

From the log snipets above that isn't clear to me.

@mreid-tt
Copy link
Contributor Author

Hoping not to muddy the water but is that libclang from an host or target perspective? i.e. using clang to build? or to link against its library at cross-compile time?

I really don't know how to answer that as this is a bit new for me. In the same readme for buildgen i do see this line:

BINDGEN_EXTRA_CLANG_ARGS_: similar to BINDGEN_EXTRA_CLANG_ARGS, but used to set per-target arguments to pass to clang. Useful to set system include directories in a target-specific way in cross-compilation environments with multiple targets. Has precedence over BINDGEN_EXTRA_CLANG_ARGS.

Other than that, is there a specific command or something in the codebase which would suggest which use case?

@hgy59
Copy link
Contributor

hgy59 commented Sep 13, 2024

Just saw a comment in https://github.com/mozilla-services/syncstorage-rs/blob/master/syncstorage-mysql/Cargo.toml

# There appears to be a compilation error with diesel

@hgy59
Copy link
Contributor

hgy59 commented Sep 13, 2024

If we focus on the stderr it provides some guidance on this. First there is the declaration of MYSQLCLIENT_LIB_DIR and MYSQLCLIENT_VERSION environment variables. Checking with our code for diesel we already have one of them set:

ENV += MYSQLCLIENT_LIB_DIR=$(STAGING_INSTALL_PREFIX)/lib

This is now obsolete with the bugfix for mysql_config in #6218 see cross/mysql-connector-c/Makefile

Instead of defining MYSQLCLIENT_LIB_DIR and MYSQLCLIENT_VERSION you have to add the path to mysql_config and it will evaluate the values for you:

# let it find mysql_config to find version and library path of libmysqlclient
ENV += PATH=$$PATH:$(STAGING_INSTALL_PREFIX)/bin 

@mreid-tt
Copy link
Contributor Author

Just saw a comment in https://github.com/mozilla-services/syncstorage-rs/blob/master/syncstorage-mysql/Cargo.toml

# There appears to be a compilation error with diesel

@hgy59, thanks for this. I looked a bit more into the codebase and saw in mozilla-services/syncstorage-rs@eb02e83 that they are not actually requiring anything more than diesel 1.4:

From the commit comment:

We're currenlty locked to diesel 1.4 due to some significant changes in the diesel_logger crate and how it expects Connections to be defined.

From the Cargo.toml:

Updating to 2.* requires changes to the Connection code for logging. (Adding an instrumentation() and set_instrumentation() method.) More investigation required.

Based on this I am curious how come the diesel 2.1.6 that we use works at all?

@mreid-tt mreid-tt changed the title ffsync: Update syncstorage-rs to v0.17.4 ffsync: Update syncstorage-rs to v0.17.14 Nov 20, 2024
@mreid-tt
Copy link
Contributor Author

@hgy59 I believe this update is ready. Reviewing the source repository for requirements.txt and tools/tokenserver/requirements.txt, I didn’t notice any updates since the last version we merged. It seems no changes are needed to the existing requirements, but I’d appreciate it if you could double-check and confirm if everything is good to go.

@mreid-tt mreid-tt changed the title ffsync: Update syncstorage-rs to v0.17.14 ffsync: Update syncstorage-rs to v0.17.15 Nov 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build/rust Requires rust build system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants