Skip to content

Conversation

@Ray-Eldath
Copy link
Contributor

@Ray-Eldath Ray-Eldath commented Sep 11, 2023

Change logs

This PR solves portability issues causing CBDB failed to compile on macOS using gcc-13:

  • matrix.c: gcc-12 and newer enhanced -Waddress to warn more always-false bool expressions, which makes one line in matrix.c to be warned and GPDB is refused to compile using gcc-12 and newer toolchain.
  • log.c: on MacOS, timeval.tv_usec is defined differently which needs to be casted in order to be printed by %ld.
  • sm4.h: type definations conflict with the system header on various different platforms.

See: https://gcc.gnu.org/gcc-12/changes.html

How was this patch tested?

Because we currently don't have a gcc-13 nor a macOS pipeline, automated testing is not possible. I manually tested this PR on Intel mac and Apple M2 mac, they all successfully build the binaries though special configure arguments are required. I've talked to releng team to write a script to make these steps easier.

Testing procedure:

  1. brew install gcc and make sure your gcc --version is a real gcc not something aliased to clang.
  2. run readmes/README.macOS.bash to install all dependencies
  3. BREWPREFIX=$(brew --prefix); CXXFLAGS="-I $BREWPREFIX/Cellar/xerces-c/3.2.4_1/include" CFLAGS="-ggdb -Og -g3 -fno-omit-frame-pointer -I $BREWPREFIX/include -I $BREWPREFIX/Cellar/xerces-c/3.2.4_1/include" LDFLAGS="-L $BREWPREFIX/Cellar/libevent/2.1.12_1/lib -L $BREWPREFIX/Cellar/xerces-c/3.2.4_1/lib -L $BREWPREFIX/opt/zstd/lib" ./configure --enable-debug --prefix=/home/gpadmin/install/cbdb
  4. make -j12 install

Maybe we should modify the readmes/README.macOS.bash to install the dependencies and ln all Cellar headers and libs to $(brew --predix)/include and $(brew --predix)/lib so that we don't need to specify any special paths in -I and -L.

Caveat

Warning

w/o a CI pipeline to guard against any changes that may break portability, CBDB could soon turns unportable again. Such thing has already happened to GPDB, their repo is entirely incompatible with gcc-13. I suggest we should at least add a small build (not testing, just build) pipeline on gcc-13 on macOS, or gcc-13 on Linux at least.

@Ray-Eldath
Copy link
Contributor Author

I've consulted @kongfanshen-0801 whether using macro to disable those type definitions in sm4.h is fine, he seems to be OK with it. since on other platforms the definitions are consistent with the system headers, I think we should remove all those name-conflicting definitions altogether.

@tuhaihe
Copy link
Member

tuhaihe commented Sep 12, 2023

Hi @Ray-Eldath , thanks for your work!

This PR solves portability issues causing CBDB failed to compile on
macOS using gcc-13:
- matrix.c: gcc-12 and newer enhanced -Waddress to warn more always-false
    bool expressions, which makes one line in matrix.c to be warned and
    GPDB is refused to compile using gcc-12 and newer toolchain.
- log.c: on MacOS, timeval.tv_usec is defined differently which needs
    to be casted in order to be printed by %ld.
- sm4.h: type definations conflict with the system header on various
    different platforms.

See: https://gcc.gnu.org/gcc-12/changes.html
@kongfanshen-0801
Copy link
Contributor

LGTM

Copy link
Contributor

@my-ship-it my-ship-it left a comment

Choose a reason for hiding this comment

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

LGTM

@my-ship-it my-ship-it merged commit af6a799 into apache:main Sep 20, 2023
@Ray-Eldath Ray-Eldath deleted the fix-portability-issues branch September 20, 2023 07:40
baotingfang pushed a commit that referenced this pull request Dec 1, 2023
This PR solves portability issues causing CBDB failed to compile on
macOS using gcc-13:
- matrix.c: gcc-12 and newer enhanced -Waddress to warn more always-false
    bool expressions, which makes one line in matrix.c to be warned and
    GPDB is refused to compile using gcc-12 and newer toolchain.
- log.c: on MacOS, timeval.tv_usec is defined differently which needs
    to be casted in order to be printed by %ld.
- sm4.h: type definations conflict with the system header on various
    different platforms.

See: https://gcc.gnu.org/gcc-12/changes.html
@my-ship-it
Copy link
Contributor

Fix #29

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