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.

Sign up for GitHub

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

Jump to bottom

MDEV-30554 RocksDB risc64 libatomic linking on rocksdb tools missing: was: Update build_rocksdb.cmake #2472

Closed
wants to merge 2 commits into from
Closed

MDEV-30554 RocksDB risc64 libatomic linking on rocksdb tools missing: was: Update build_rocksdb.cmake #2472

wants to merge 2 commits into from

Conversation

LocutusOfBorg
Copy link

Do not erase ${SYSTEM_LIBS} variable when setting it, this makes the latomic flag set for riscv64 lost

this is the current code

if(CMAKE_SYSTEM_PROCESSOR STREQUAL "riscv64")
 set(SYSTEM_LIBS ${SYSTEM_LIBS} -latomic)
endif()
find_package(Threads REQUIRED)
if(WIN32)
  set(SYSTEM_LIBS ${SYSTEM_LIBS} Shlwapi.lib Rpcrt4.lib)
else()
  set(SYSTEM_LIBS ${CMAKE_THREAD_LIBS_INIT} ${LIBRT} ${CMAKE_DL_LIBS})
endif()

So, in case of not WIN32, the SYSTEM_LIBS with latomic value is just lost. We should keep it to fix riscv64 build failure
https://launchpadlibrarian.net/648316103/buildlog_ubuntu-lunar-riscv64.mariadb-10.6_1%3A10.6.11-2_BUILDING.txt.gz

@LocutusOfBorg
Update build_rocksdb.cmake
04110b6 
Do not erase ${SYSTEM_LIBS} variable when setting it, this makes the latomic flag set for riscv64 lost
@CLAassistant
Copy link

CLAassistant commented Feb 1, 2023 •
edited

CLA assistant check
All committers have signed the CLA.

@LocutusOfBorg
Copy link
Author

@ottok I'm uploading the change in Ubuntu, feel free to upload in Debian if you agree please :)

@LocutusOfBorg
Add CheckAtomic package (from cmake-modules-extra) helper to fix buil…
7c42515 
…d on riscv64 architecture and all the archs needing latomic link
grooverdan added a commit to grooverdan/mariadb-server that referenced this pull request Feb 3, 2023
@grooverdan
MDEV-30554 RockDB libatomic linking on riscv64
51f09a7 
The existing storage/rocksdb/CMakeCache.txt defined
ATOMIC_EXTRA_LIBS when atomics where required. This was
determined by the toplevel configure.cmake test
(HAVE_GCC_C11_ATOMICS_WITH_LIBATOMIC).

As build_rocksdb.cmake is included after ATOMIC_EXTRA_LIBS
was set, we just need to use it. As such no riscv64
specific macro is needed in build_rocksdb.cmake.

As highlighted by Gianfranco Costamagna (@LocutusOfBorg)
in MariaDB#2472 overwriting SYSTEM_LIBS was problematic.
This is corrected in case in future SYSTEM_LIBS is changed
elsewhere.

Closes MariaDB#2472.
grooverdan added a commit to grooverdan/mariadb-server that referenced this pull request Feb 3, 2023
@grooverdan
MDEV-30554 RockDB libatomic linking on riscv64
394c486 
The existing storage/rocksdb/CMakeCache.txt defined
ATOMIC_EXTRA_LIBS when atomics where required. This was
determined by the toplevel configure.cmake test
(HAVE_GCC_C11_ATOMICS_WITH_LIBATOMIC).

As build_rocksdb.cmake is included after ATOMIC_EXTRA_LIBS
was set, we just need to use it. As such no riscv64
specific macro is needed in build_rocksdb.cmake.

As highlighted by Gianfranco Costamagna (@LocutusOfBorg)
in MariaDB#2472 overwriting SYSTEM_LIBS was problematic.
This is corrected in case in future SYSTEM_LIBS is changed
elsewhere.

Closes MariaDB#2472.
grooverdan added a commit to grooverdan/mariadb-server that referenced this pull request Feb 3, 2023
@grooverdan
MDEV-30554 RockDB libatomic linking on riscv64
845e00f 
The existing storage/rocksdb/CMakeCache.txt defined
ATOMIC_EXTRA_LIBS when atomics where required. This was
determined by the toplevel configure.cmake test
(HAVE_GCC_C11_ATOMICS_WITH_LIBATOMIC).

As build_rocksdb.cmake is included after ATOMIC_EXTRA_LIBS
was set, we just need to use it. As such no riscv64
specific macro is needed in build_rocksdb.cmake.

As highlighted by Gianfranco Costamagna (@LocutusOfBorg)
in MariaDB#2472 overwriting SYSTEM_LIBS was problematic.
This is corrected in case in future SYSTEM_LIBS is changed
elsewhere.

Closes MariaDB#2472.
@grooverdan grooverdan mentioned this pull request Feb 3, 2023
Merged
@grooverdan
Copy link
Member

@LocutusOfBorg thanks very much for highlighting the obvious flaw and digging up the University of Illinois check.

It seems from the build above that the amd64 Centos 7 tries to include libatomic and it doesn't exist.

As it turns out the top level configure.cmake already does a similar check.

Do you mind checking #2477.

@grooverdan grooverdan changed the title Update build_rocksdb.cmake MDEV-30554 RocksDB risc64 libatomic linking on rocksdb tools missing: was: Update build_rocksdb.cmake Feb 3, 2023
@ottok
Copy link
Contributor

ottok commented Feb 3, 2023

@ottok I'm uploading the change in Ubuntu, feel free to upload in Debian if you agree please :)

What do you mean? Are you going to fork the version in Ubuntu? That tends to cause me more work and not really help.

@LocutusOfBorg
Copy link
Author

@ottok we can sync as soon as you upload again in Debian
Actually I uploaded the patch to mariadb-10.6 to later discover that it can't build anymore because mariadb has higher version. So my upload is just lost.

@ottok
Copy link
Contributor

ottok commented Feb 5, 2023

Thanks @LocutusOfBorg for your help. Could you please also review #2477 or any of the open MRs at https://salsa.debian.org/mariadb-team/mariadb-server/-/merge_requests to help the build issues and Debian packaging improve faster?

There is also a s390x test error visible on Debian builds at https://buildd.debian.org/status/package.php?p=mariadb which did not reproduce on Launchpad builds at https://launchpad.net/~mysql-ubuntu/+archive/ubuntu/mariadb-10.11/+builds?build_text=&build_state=all which I could use some help on. Merge Requests directly on the Debian (and Ubuntu) packaging welcome so I can do the next upload as soon as possible.

@LocutusOfBorg
Copy link
Author

Started 7 hours ago
Finished 7 minutes ago (took 7 hours, 26 minutes, 31.1 seconds)

@LocutusOfBorg
Copy link
Author

@ottok I did open a merge request with the PR to fix riscv64, as uploaded in Ubuntu, so we can sync again on next update.
The s390x failure looks scary, its not a endianess issue to me, some sort of toolchain trouble?

grooverdan added a commit to grooverdan/mariadb-server that referenced this pull request Feb 7, 2023
@grooverdan
MDEV-30554 RockDB libatomic linking on riscv64
195c0e8 
The existing storage/rocksdb/CMakeCache.txt defined
ATOMIC_EXTRA_LIBS when atomics where required. This was
determined by the toplevel configure.cmake test
(HAVE_GCC_C11_ATOMICS_WITH_LIBATOMIC).

As build_rocksdb.cmake is included after ATOMIC_EXTRA_LIBS
was set, we just need to use it. As such no riscv64
specific macro is needed in build_rocksdb.cmake.

As highlighted by Gianfranco Costamagna (@LocutusOfBorg)
in MariaDB#2472 overwriting SYSTEM_LIBS was problematic.
This is corrected in case in future SYSTEM_LIBS is changed
elsewhere.

Closes MariaDB#2472.
grooverdan added a commit that referenced this pull request Feb 7, 2023
@grooverdan
MDEV-30554 RockDB libatomic linking on riscv64
17423c6 
The existing storage/rocksdb/CMakeCache.txt defined
ATOMIC_EXTRA_LIBS when atomics where required. This was
determined by the toplevel configure.cmake test
(HAVE_GCC_C11_ATOMICS_WITH_LIBATOMIC).

As build_rocksdb.cmake is included after ATOMIC_EXTRA_LIBS
was set, we just need to use it. As such no riscv64
specific macro is needed in build_rocksdb.cmake.

As highlighted by Gianfranco Costamagna (@LocutusOfBorg)
in #2472 overwriting SYSTEM_LIBS was problematic.
This is corrected in case in future SYSTEM_LIBS is changed
elsewhere.

Closes #2472.
@grooverdan
Copy link
Member

merged #2477 instead. Thanks for testing.

@grooverdan grooverdan closed this Feb 7, 2023
@LocutusOfBorg LocutusOfBorg deleted the patch-1 branch February 7, 2023 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
4 participants
@LocutusOfBorg @CLAassistant @grooverdan @ottok