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

Fix 'tests' target build on macOS 15 #5364

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

ihnorton
Copy link
Member

@ihnorton ihnorton commented Oct 30, 2024

On macOS 15, make tests is broken on current dev with errors like this:

pydev ❯ nj tests
[184/468] Building CXX object tiledb/stdx/test/CMakeFiles/unit_stdx.dir/compat.cc.o
FAILED: tiledb/stdx/test/CMakeFiles/unit_stdx.dir/compat.cc.o
/usr/bin/g++ -DNDEBUG -D_FILE_OFFSET_BITS=64 -I/Users/inorton/work/git/TileDB -isystem /Users/inorton/work/bld/TileDB-rel/vcpkg_installed/arm64-osx/include -O3 -DNDEBUG -std=c++20 -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.1.sdk -mmacosx-version-min=15.0 -fPIE -fvisibility=hidden -Wall -Wextra -Werror -O3 -Wno-unqualified-std-cast-call -MD -MT tiledb/stdx/test/CMakeFiles/unit_stdx.dir/compat.cc.o -MF tiledb/stdx/test/CMakeFiles/unit_stdx.dir/compat.cc.o.d -o tiledb/stdx/test/CMakeFiles/unit_stdx.dir/compat.cc.o -c /Users/inorton/work/git/TileDB/tiledb/stdx/test/compat.cc
In file included from /Users/inorton/work/git/TileDB/tiledb/stdx/test/compat.cc:34:
In file included from /Users/inorton/work/git/TileDB/tiledb/stdx/stop_token:41:
/Users/inorton/work/git/TileDB/tiledb/stdx/bits/stop_token.h:150:7: error: reference to '__stop_callback_base' is ambiguous
  150 |       __stop_callback_base* __cb,
      |       ^

gist: https://gist.github.com/ihnorton/2f616091f92be4b493f7fec6fdd40dee

It looks like AppleClang has added incomplete support for stop_token and std::jthread, but __cpp_lib_jthread guard doesn't exist, which breaks our existing feature guards in tiledb/stdx/{stop_token,thread}.

Fix is to add -fexperimental-library to AppleClang>16 cxx flags.


TYPE: NO_HISTORY
DESC: Fix 'tests' target build on AppleClang 16 (stop_token,jthread)

`make tests` is broken on current dev with errors like this:

```
pydev ❯ nj tests
[184/468] Building CXX object tiledb/stdx/test/CMakeFiles/unit_stdx.dir/compat.cc.o
FAILED: tiledb/stdx/test/CMakeFiles/unit_stdx.dir/compat.cc.o
/usr/bin/g++ -DNDEBUG -D_FILE_OFFSET_BITS=64 -I/Users/inorton/work/git/TileDB -isystem /Users/inorton/work/bld/TileDB-rel/vcpkg_installed/arm64-osx/include -O3 -DNDEBUG -std=c++20 -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.1.sdk -mmacosx-version-min=15.0 -fPIE -fvisibility=hidden -Wall -Wextra -Werror -O3 -Wno-unqualified-std-cast-call -MD -MT tiledb/stdx/test/CMakeFiles/unit_stdx.dir/compat.cc.o -MF tiledb/stdx/test/CMakeFiles/unit_stdx.dir/compat.cc.o.d -o tiledb/stdx/test/CMakeFiles/unit_stdx.dir/compat.cc.o -c /Users/inorton/work/git/TileDB/tiledb/stdx/test/compat.cc
In file included from /Users/inorton/work/git/TileDB/tiledb/stdx/test/compat.cc:34:
In file included from /Users/inorton/work/git/TileDB/tiledb/stdx/stop_token:41:
/Users/inorton/work/git/TileDB/tiledb/stdx/bits/stop_token.h:150:7: error: reference to '__stop_callback_base' is ambiguous
  150 |       __stop_callback_base* __cb,
      |       ^
```

gist: https://gist.github.com/ihnorton/2f616091f92be4b493f7fec6fdd40dee

It looks like AppleClang has added incomplete support for `stop_token` and `std::jthread`, but `__cpp_lib_jthread` guard doesn't exist, which breaks our existing feature guards in `tiledb/stdx/{stop_token,thread}`.

Fix is to add `-fexperimental-library` to AppleClang>16 cxx flags.

---
TYPE: NO_HISTORY
DESC: Fix 'tests' target build on AppleClang 16 (stop_token,jthread)
Copy link
Member

@teo-tsirpanis teo-tsirpanis left a comment

Choose a reason for hiding this comment

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

I think the root cause of the failures is that we are defining stuff in the std namespace, which is undefined behavior IIRC. We should use stdx or something else, and forward to std if the APIs are available on the platform.

@ihnorton
Copy link
Member Author

We should use stdx or something else, and forward to std if the APIs are available on the platform.

The platform tests are broken on AppleClang 16 without this change because __cpp_lib_thread is not defined, so our conditional include for the alternative implementation is used.

Copy link
Contributor

@bekadavis9 bekadavis9 left a comment

Choose a reason for hiding this comment

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

LGTM

@teo-tsirpanis teo-tsirpanis self-requested a review October 30, 2024 19:07
@teo-tsirpanis
Copy link
Member

(removing the requested changes, please create a story to change the std namespace for the polyfills)

@bekadavis9
Copy link
Contributor

make tests still fails for me on macOS 15.1 after this fix.

A few errors include:

error: no member named 'copy' in namespace 'std::ranges'; did you mean 'bcopy'?

error: no member named 'permutable' in namespace 'std'

error: unexpected type name 'ZI': expected expression

error: no member named 'sortable' in namespace 'std'

@ihnorton ihnorton merged commit 08acabc into dev Nov 6, 2024
65 checks passed
@ihnorton ihnorton deleted the ihn/sc-58550/fix-tests-target-macos15 branch November 6, 2024 15:22
ihnorton added a commit that referenced this pull request Jan 2, 2025
Fix compilation errors on newer Clang (upstream) versions which do not fully
support jthread and stop_token. Already used for AppleClang variant.

x-ref
  - #5364
  - #5411
ihnorton added a commit that referenced this pull request Jan 3, 2025
)

Fix compilation errors on newer Clang (upstream) versions which do not
fully support jthread and stop_token. Already used for AppleClang
variant.

x-ref
  - #5364
  - closes #5411

---
TYPE: NO_HISTORY
DESC: Add -fexperimental-library flag on Clang 16 to fix compile errors
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.

3 participants