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

chore: Switch to libmariadb-dev #1635

Closed
wants to merge 4 commits into from
Closed

Conversation

ghagl
Copy link

@ghagl ghagl commented Nov 27, 2024

Description

The MySQL repository causes issues when trying to build for other architectures (non-existent MySQL binaries), therefore the MariaDB packages should instead be used. This has been successfully tested on arm64.

Testing

Build on all relevant architectures (x86, arm64, ...).

@ghagl ghagl force-pushed the master branch 2 times, most recently from ff8a17c to 4205542 Compare November 29, 2024 13:42
Copy link
Member

@jrconlin jrconlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. I'm fine switching to mariadb for a lot of reasons, it just hasn't come up as a priority for the team.

@jrconlin jrconlin requested a review from pjenvey December 4, 2024 18:22
@jrconlin jrconlin requested a review from taddes December 9, 2024 17:32
apt-get -q update && \
apt-get -q install -y --no-install-recommends libmysqlclient-dev cmake
apt-get -q install -y --no-install-recommends libmariadb-dev-compat libmariadb-dev cmake
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: libmariadb-dev is superfluous w/ libmariadb-dev-compat (which depends on it)

@@ -11,12 +11,10 @@ ARG DATABASE_BACKEND=spanner

# cmake is required to build grpcio-sys for Spanner builds
RUN \
# Fetch and load the MySQL public key. We need to install libmysqlclient-dev to build syncstorage-rs
# We need to install libmariadb-dev to build syncstorage-rs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can kill this comment entirely

@@ -65,16 +56,9 @@ RUN \
RUN \
groupadd --gid 10001 app && \
useradd --uid 10001 --gid 10001 --home /app --create-home app && \
# first, an apt-get update is required for gnupg, which is required for apt-key adv
# first, an apt-get update is required for the packages
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please kill this comment entirely as well (it was explaining the previous need for calling apt-get update twice here)

@pjenvey
Copy link
Member

pjenvey commented Dec 11, 2024

(For reference: where we originally moved away from maria's client: #1435 )

@tommie
Copy link

tommie commented Dec 27, 2024

Oh, I just realized I'm doing similar work. I have a branch that upgrades to Diesel 2.x, which uses mysqlclient-sys 0.4.0, which supports finding libmariadb-dev without -compat.

I retained the libmysqlclient code as a build arg option.

@ghagl
Copy link
Author

ghagl commented Feb 12, 2025

Is this PR now redundant by other changes that have already been made upstream?

pjenvey added a commit that referenced this pull request Mar 21, 2025
as it builds on more architectures (such as arm64) and we no longer require its
TLS support

derived from ghagl's and tommie's PRs (thanks!)
#1635
#1644

Closes SYNC-4690
pjenvey added a commit that referenced this pull request Mar 21, 2025
as it builds on more architectures (such as arm64) and we no longer require its
TLS support

derived from ghagl's and tommie's PRs (thanks!)
#1635
#1644

Closes SYNC-4690
pjenvey added a commit that referenced this pull request Mar 21, 2025
as it builds on more architectures (such as arm64) and we no longer require its
TLS support

derived from ghagl's and tommie's PRs (thanks!)
#1635
#1644

Closes SYNC-4690
@pjenvey
Copy link
Member

pjenvey commented Mar 24, 2025

Is this PR now redundant by other changes that have already been made upstream?

It is as of the recently merged #1665 -- thank you for the PR @ghagl

@pjenvey pjenvey closed this Mar 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants