search

haasn / libplacebo

Official mirror of libplacebo
http://libplacebo.org/
GNU Lesser General Public License v2.1
547 stars 69 forks source link

Build Fail on macOS 10.14 Mojave and earlier #199

Closed deus0ww closed 1 year ago

deus0ww commented 1 year ago

The recent changes to src/convert.cc is causing build to fail on macOS 10.13 and 10.14 (building fine on macOS 13.5.1) with the errors below. 

clang++ -Isrc/libplacebo.318.dylib.p -Isrc -I../src -Isrc/include -I../src/include -I../3rdparty/Vulkan-Headers/include -I../3rdparty/fast_float/include -Isrc/opengl/include -I../src/opengl/include -Isrc/shaders -Isrc/vulkan -Isrc/opengl/include/glad -I/usr/local/opt/vulkan-headers/include -I/usr/local/Cellar/little-cms2/2.15/include -fvisibility=hidden -flto=thin -fcolor-diagnostics -Wall -Winvalid-pch -Wextra -std=c++17 -O3 -Wundef -Wshadow -Wparentheses -Wpointer-arith -DPL_HAVE_PTHREAD '' -MD -MQ src/libplacebo.318.dylib.p/convert.cc.o -MF src/libplacebo.318.dylib.p/convert.cc.o.d -o src/libplacebo.318.dylib.p/convert.cc.o -c ../src/convert.cc
FAILED: src/libplacebo.318.dylib.p/convert.cc.o 
clang++ -Isrc/libplacebo.318.dylib.p -Isrc -I../src -Isrc/include -I../src/include -I../3rdparty/Vulkan-Headers/include -I../3rdparty/fast_float/include -Isrc/opengl/include -I../src/opengl/include -Isrc/shaders -Isrc/vulkan -Isrc/opengl/include/glad -I/usr/local/opt/vulkan-headers/include -I/usr/local/Cellar/little-cms2/2.15/include -fvisibility=hidden -flto=thin -fcolor-diagnostics -Wall -Winvalid-pch -Wextra -std=c++17 -O3 -Wundef -Wshadow -Wparentheses -Wpointer-arith -DPL_HAVE_PTHREAD '' -MD -MQ src/libplacebo.318.dylib.p/convert.cc.o -MF src/libplacebo.318.dylib.p/convert.cc.o.d -o src/libplacebo.318.dylib.p/convert.cc.o -c ../src/convert.cc
../src/convert.cc:32:1: error: unknown type name 'concept'
concept has_std_to_chars = requires(char *begin, char *end, T &n)
^
../src/convert.cc:32:42: error: expected '(' for function-style cast or type construction
concept has_std_to_chars = requires(char *begin, char *end, T &n)
                                    ~~~~ ^
../src/convert.cc:32:43: error: use of undeclared identifier 'begin'; did you mean 'std::begin'?
concept has_std_to_chars = requires(char *begin, char *end, T &n)
                                          ^~~~~
                                          std::begin
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/initializer_list:100:1: note: 'std::begin' declared here
begin(initializer_list<_Ep> __il) _NOEXCEPT
^
../src/convert.cc:32:55: error: expected '(' for function-style cast or type construction
concept has_std_to_chars = requires(char *begin, char *end, T &n)
                                                 ~~~~ ^
../src/convert.cc:32:56: error: use of undeclared identifier 'end'; did you mean 'std::end'?
concept has_std_to_chars = requires(char *begin, char *end, T &n)
                                                       ^~~
                                                       std::end
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/initializer_list:109:1: note: 'std::end' declared here
end(initializer_list<_Ep> __il) _NOEXCEPT
^
../src/convert.cc:32:61: error: 'T' does not refer to a value
concept has_std_to_chars = requires(char *begin, char *end, T &n)
                                                            ^
../src/convert.cc:31:20: note: declared here
template <typename T>
                   ^
../src/convert.cc:32:64: error: use of undeclared identifier 'n'
concept has_std_to_chars = requires(char *begin, char *end, T &n)
                                                               ^
../src/convert.cc:54:1: error: unknown type name 'concept'
concept has_std_from_chars = requires(const char *begin, const char *end, T &n)
^
../src/convert.cc:54:39: error: expected expression
concept has_std_from_chars = requires(const char *begin, const char *end, T &n)
                                      ^
../src/convert.cc:54:58: error: expected expression
concept has_std_from_chars = requires(const char *begin, const char *end, T &n)
                                                         ^
../src/convert.cc:54:75: error: 'T' does not refer to a value
concept has_std_from_chars = requires(const char *begin, const char *end, T &n)
                                                                          ^
../src/convert.cc:53:20: note: declared here
template <typename T>
                   ^
../src/convert.cc:54:78: error: use of undeclared identifier 'n'
concept has_std_from_chars = requires(const char *begin, const char *end, T &n)
                                                                             ^
../src/convert.cc:91:1: error: expected ')'
CHAR_CONVERT(hex, unsigned short, 16)
^
../src/convert.cc:84:37: note: expanded from macro 'CHAR_CONVERT'
        return to_chars(buf, len, n __VA_OPT__(,) __VA_ARGS__); \
                                    ^
../src/convert.cc:91:1: note: to match this '('
../src/convert.cc:84:24: note: expanded from macro 'CHAR_CONVERT'
        return to_chars(buf, len, n __VA_OPT__(,) __VA_ARGS__); \
                       ^
../src/convert.cc:91:1: error: expected ')'
CHAR_CONVERT(hex, unsigned short, 16)
^
../src/convert.cc:88:35: note: expanded from macro 'CHAR_CONVERT'
        return from_chars(str, *n __VA_OPT__(,) __VA_ARGS__);   \
                                  ^
../src/convert.cc:91:1: note: to match this '('
../src/convert.cc:88:26: note: expanded from macro 'CHAR_CONVERT'
        return from_chars(str, *n __VA_OPT__(,) __VA_ARGS__);   \
                         ^
../src/convert.cc:92:1: error: expected ')'
CHAR_CONVERT(int, int)
^
../src/convert.cc:84:37: note: expanded from macro 'CHAR_CONVERT'
        return to_chars(buf, len, n __VA_OPT__(,) __VA_ARGS__); \
                                    ^
../src/convert.cc:92:1: note: to match this '('
../src/convert.cc:84:24: note: expanded from macro 'CHAR_CONVERT'
        return to_chars(buf, len, n __VA_OPT__(,) __VA_ARGS__); \
                       ^
../src/convert.cc:92:1: error: expected ')'
CHAR_CONVERT(int, int)
^
../src/convert.cc:88:35: note: expanded from macro 'CHAR_CONVERT'
        return from_chars(str, *n __VA_OPT__(,) __VA_ARGS__);   \
                                  ^
../src/convert.cc:92:1: note: to match this '('
../src/convert.cc:88:26: note: expanded from macro 'CHAR_CONVERT'
        return from_chars(str, *n __VA_OPT__(,) __VA_ARGS__);   \
                         ^
../src/convert.cc:93:1: error: expected ')'
CHAR_CONVERT(uint, unsigned int)
^
../src/convert.cc:84:37: note: expanded from macro 'CHAR_CONVERT'
        return to_chars(buf, len, n __VA_OPT__(,) __VA_ARGS__); \
                                    ^
../src/convert.cc:93:1: note: to match this '('
../src/convert.cc:84:24: note: expanded from macro 'CHAR_CONVERT'
        return to_chars(buf, len, n __VA_OPT__(,) __VA_ARGS__); \
                       ^
../src/convert.cc:93:1: error: expected ')'
CHAR_CONVERT(uint, unsigned int)
^
../src/convert.cc:88:35: note: expanded from macro 'CHAR_CONVERT'
        return from_chars(str, *n __VA_OPT__(,) __VA_ARGS__);   \
                                  ^
../src/convert.cc:93:1: note: to match this '('
../src/convert.cc:88:26: note: expanded from macro 'CHAR_CONVERT'
        return from_chars(str, *n __VA_OPT__(,) __VA_ARGS__);   \
                         ^
../src/convert.cc:94:1: error: expected ')'
CHAR_CONVERT(int64, int64_t)
^
../src/convert.cc:84:37: note: expanded from macro 'CHAR_CONVERT'
        return to_chars(buf, len, n __VA_OPT__(,) __VA_ARGS__); \
                                    ^
../src/convert.cc:94:1: note: to match this '('
../src/convert.cc:84:24: note: expanded from macro 'CHAR_CONVERT'
        return to_chars(buf, len, n __VA_OPT__(,) __VA_ARGS__); \
                       ^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 errors generated.
haasn commented 1 year ago

cc @kasper93

kasper93 commented 1 year ago

I will add code path without using concepts to resolve this issue. But we need to define the line somwhere, because if we start going back to Ubuntu 18.04 era compilers, it may not be worth my effort to fix that target...

deus0ww commented 1 year ago

I totally understand ending support for old systems/compilers and I would really appreciate it if you could document new requirements somewhere. This way users will know if a change is intentional or an oversight.

kasper93 commented 1 year ago

It is be GCC 10 and Clang 10 currently. GCC actually supports concepts since 6, but as TS, so need to use different namespace.

I don't know what versions it is in Apple Clang world.

In general I think we do not define hard requirement, because our support is opportunistic. If a user comes with an issue and it is not a pain to fix/add support we can do that.

deus0ww commented 1 year ago

macOS 10.14:

$ clang --version
Apple LLVM version 10.0.0 (clang-1000.11.45.5)
Target: x86_64-apple-darwin17.7.0

macOS 10.13:

$ clang --version
Apple clang version 11.0.0 (clang-1100.0.33.17)
Target: x86_64-apple-darwin18.7.0

Unfortunately for me, this is equivalent to Clang 9... source

haasn commented 1 year ago

We should, at the very least, add a check to meson.build and warn the user with a more readable error message (where the minimum requirements are also printed).

Though, if we're adding a meson check, why not just directly check to see if from_chars compiles, and not require the concept?

kasper93 commented 1 year ago

Compilation errors are compilation errors, not generation ones.

We should, at the very least, add a check to meson.build and warn the user with a more readable error message (where the minimum requirements are also printed).

It's pretty clear message, "concepts failed to compile" and we use concepts. It is completely unrelated to conversion code in fact.

How do you imagine checking all that in meson and inferring "better" message? Do you plan to maintain supported compiler list in meson and somehow keep it updated? Or compile convert.cc also during generation and parse compiler output? Or unit test all 14 conversion function individually in meson?

haasn commented 1 year ago

It's pretty clear message, "concepts failed to compile" and we use concepts.

Pretty clear if you ignore the clutter/flood of irrelevant messages surrounding it.

How do you imagine checking all that in meson and inferring "better" message?

has_concepts = cc.compiles('template<typename T> concept Test = true;')

Or unit test all 14 conversion function individually in meson?

Afaict, it's only two cases we care about: parsing float, and printing float. So only two checks needed, no?

kasper93 commented 1 year ago

Pretty clear if you ignore the clutter/flood of irrelevant messages surrounding it.

Don't @ me. It is compiler issue. Incidentally newer compilers produce better diagnostics, but we are talking about older ones here.

has_concepts = cc.compiles('template concept Test = true;')

Just don't use concepts. No need to really check it.

kasper93 commented 1 year ago

Probably fixed by https://code.videolan.org/videolan/libplacebo/-/merge_requests/584

@deus0ww: Could you give it a try?

deus0ww commented 1 year ago

On macOS 10.14, it's failing during linking:

Undefined symbols for architecture x86_64:
  "std::__1::__itoa::__u32toa(unsigned int, char*)", referenced from:
      _pl_str_print_int in convert.cc.o
      _pl_str_print_uint in convert.cc.o
  "std::__1::__itoa::__u64toa(unsigned long long, char*)", referenced from:
      _pl_str_print_int64 in convert.cc.o
      _pl_str_print_uint64 in convert.cc.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.
kasper93 commented 1 year ago

So this was likely fixed by https://reviews.llvm.org/D74626

libc++ incorrectly make them available, while to looks like appropriate symbols are available starting with 10.15.

I don't know if you can build on newer macos and link statically libc++?

@haasn: What do we do about it? This would essentially mean to revert all convert.cc to support <= 10.14 as it looks like even basic conversion don't work.

haasn commented 1 year ago

@kasper93 macOS 10.14 is from 2018, right? Then actually, why do we care? Arguably even Ubuntu 20.04 is stretching it, because 22.04 is current LTS.

Maybe we should just revert this patch.

kasper93 commented 1 year ago

macOS 10.14 is from 2018, right? Then actually, why do we care?

I don't know about macOS. I don't use their ecosystem, don't even know if 10.14 is relevant still or not.

Arguably even Ubuntu 20.04 is stretching it, because 22.04 is current LTS.

We can wait for next LTS, I think 20.04 is still common.

Maybe we should just revert this patch.

Nah, let's keep it, it is not that terrible. I'm just surprised it fails during linking now. Apparently Apple is not that good w.r.t. C++ or even POSIX support...

deus0ww commented 1 year ago

Thank you, both, for looking into this. I'll stick with v6.292.1 on older Macs.

eko5624 commented 1 year ago

Thank you, both, for looking into this. I'll stick with v6.292.1 on older Macs.

v6.292.1 doesn't work on mac by the way. Check this https://github.com/haasn/libplacebo/issues/195 You need this patch https://code.videolan.org/videolan/libplacebo/-/merge_requests/564

deus0ww commented 1 year ago

Thank, @eko5624. That pr was annoyingly merged after the problematic changes. I'll just build mpv/ffmpeg without libplacebo on older macs as they don't need it anyway.