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

FTBFS on AArch64 Darwin due to unknown linker options with Apple ld #832

Open
jchadwick-buf opened this issue Jul 23, 2024 · 0 comments
Open

Comments

@jchadwick-buf
Copy link

Since 16ca259, cel-cpp fails to build correctly on AArch64 Darwin, due to using -Wl,--export-dynamic-symbol:

ld: unknown options: --export-dynamic-symbol=cel_common_internal_LegacyListValue_GetType --export-dynamic-symbol=cel_common_internal_LegacyListValue_DebugString --export-dynamic-symbol=cel_common_internal_LegacyListValue_GetSerializedSize --export-dynamic-symbol=cel_common_internal_LegacyListValue_SerializeTo --export-dynamic-symbol=cel_common_internal_LegacyListValue_ConvertToJsonArray --export-dynamic-symbol=cel_common_internal_LegacyListValue_IsEmpty --export-dynamic-symbol=cel_common_internal_LegacyListValue_Size --export-dynamic-symbol=cel_common_internal_LegacyListValue_Get --export-dynamic-symbol=cel_common_internal_LegacyListValue_ForEach --export-dynamic-symbol=cel_common_internal_LegacyListValue_NewIterator --export-dynamic-symbol=cel_common_internal_LegacyListValue_Contains --export-dynamic-symbol=cel_common_internal_LegacyMapValue_GetType --export-dynamic-symbol=cel_common_internal_LegacyMapValue_DebugString --export-dynamic-symbol=cel_common_internal_LegacyMapValue_GetSerializedSize --export-dynamic-symbol=cel_common_internal_LegacyMapValue_SerializeTo --export-dynamic-symbol=cel_common_internal_LegacyMapValue_ConvertToJsonObject --export-dynamic-symbol=cel_common_internal_LegacyMapValue_IsEmpty --export-dynamic-symbol=cel_common_internal_LegacyMapValue_Size --export-dynamic-symbol=cel_common_internal_LegacyMapValue_Find --export-dynamic-symbol=cel_common_internal_LegacyMapValue_Get --export-dynamic-symbol=cel_common_internal_LegacyMapValue_Has --export-dynamic-symbol=cel_common_internal_LegacyMapValue_ListKeys --export-dynamic-symbol=cel_common_internal_LegacyMapValue_ForEach --export-dynamic-symbol=cel_common_internal_LegacyMapValue_NewIterator --export-dynamic-symbol=cel_common_internal_LegacyStructValue_DebugString --export-dynamic-symbol=cel_common_internal_LegacyStructValue_GetSerializedSize --export-dynamic-symbol=cel_common_internal_LegacyStructValue_SerializeTo --export-dynamic-symbol=cel_common_internal_LegacyStructValue_GetType --export-dynamic-symbol=cel_common_internal_LegacyStructValue_GetTypeName --export-dynamic-symbol=cel_common_internal_LegacyStructValue_ConvertToJsonObject --export-dynamic-symbol=cel_common_internal_LegacyStructValue_GetFieldByName --export-dynamic-symbol=cel_common_internal_LegacyStructValue_GetFieldByNumber --export-dynamic-symbol=cel_common_internal_LegacyStructValue_HasFieldByName --export-dynamic-symbol=cel_common_internal_LegacyStructValue_HasFieldByNumber --export-dynamic-symbol=cel_common_internal_LegacyStructValue_Equal --export-dynamic-symbol=cel_common_internal_LegacyStructValue_ForEachField --export-dynamic-symbol=cel_common_internal_LegacyStructValue_Qualify --export-dynamic-symbol=cel_common_internal_LegacyTypeReflector_NewListValueBuilder --export-dynamic-symbol=cel_common_internal_LegacyTypeReflector_NewMapValueBuilder

Apple's system linker, ld, does not support this option, though I believe the option -exported_symbol may be equivalent.

Since Apple ld is OS-specific, it might be OK to just use a typical select statement here. Like this:

diff --git a/common/BUILD b/common/BUILD
index 8933cf5e..aad7cad9 100644
--- a/common/BUILD
+++ b/common/BUILD
@@ -553,7 +553,10 @@ cc_library(
     # use the dynamic linker to find the symbols at runtime. This only works if they are exported as
     # dynamic symbols. When static linking, our symbols are not exported as dynamic by default. We
     # workaround this by explicitly passing our symbols to the linker for exporting.
-    linkopts = ["-Wl,--export-dynamic-symbol=%s" % dynamic_symbol for dynamic_symbol in _DYNAMIC_SYMBOLS],
+    linkopts = select({
+        "@bazel_tools//src/conditions:darwin": ["-Wl,-exported_symbol -Wl,%s" % dynamic_symbol for dynamic_symbol in _DYNAMIC_SYMBOLS],
+        "//conditions:default": ["-Wl,--export-dynamic-symbol=%s" % dynamic_symbol for dynamic_symbol in _DYNAMIC_SYMBOLS],
+    }),
     deps = [
         ":casting",
         ":json",

Testing locally, this does seem to make the build go further.

P.S.: It seems like all of LLVM, GNU Binutils and Apple's linker have equivalent options where you can specify a file that contains a list of symbols to export; -exported_symbols_list on Apple LD and --export-dynamic-symbol-list on GNU Binutils and LLVM's linker. That said, the format is different between the two: the LLVM and GNU Binutils version seems to match the symbol versions file and look like { a; b; } whereas the Apple linker version appears to be one per-line, like:

a
b

Maybe still worth it to make the linker args more reasonable. (Eventually it's bound to breach the argv limit on some platform, I'm sure.)

P.P.S: The equivalent windows_msvc option might be a little trickier to nail down. Realistically the closest Win32 analogue I can think of is linker export definitions, which is yet another format that looks something like:

LIBRARY lib
EXPORTS
    a
    b

It's a little awkward that every platform has these little quirks with linking that can't be hidden away by Bazel somehow.

rodaine referenced this issue in bufbuild/protovalidate-cc Aug 7, 2024
Updates Protobuf to v27 and protovalidate to v0.7.1, and fixes all of
the resulting compilation and conformance failures.

As one would expect, there was a tremendous amount of troubleshooting
involved in this thankfully-relatively-small PR. Here's my log of what
happened. I'll try to be succinct, but I want to capture all of the
details so my reasoning can be understood in the future.

- First, I tried to update protobuf. This led to pulling a newer version
of absl. The version of cel-cpp we use did not compile with this version
of absl.

- Next, I tried to update cel-cpp. However, the latest version of
cel-cpp is broken on macOS for two separate reasons
<sup>[1](google/cel-cpp#831),
[2](https://github.com/google/cel-cpp/issues/832)</sup>.

- After taking a break to work on other protovalidate implementations I
returned and tried another approach. This time, instead of updating
cel-cpp, I just patched it to work with newer absl. Thankfully, this
proved surprisingly viable. The `cel_cpp.patch` file now contains this
fix too.

- Unfortunately, compilation was broken in CI on a non-sense compiler
error:
    ```
error: could not convert template argument 'ptr' from 'const
google::protobuf::Struct& (* const)()' to 'const
google::protobuf::Struct& (* const)()'
    ```
    It seemed likely to be a compiler issue, thus I was stalled again.

- For some reason it finally occurred to me that I probably should just
simply update the compiler. In a stroke of accidental rubber-ducking
luck, I noticed that GitHub's `ubuntu-latest` had yet to actually move
to `ubuntu-24.04`, which has a vastly more up-to-date C++ toolchain than
the older `ubuntu-22.04`. This immediately fixed the problem.

- E-mail validation is hard. In other languages we fall back on standard
library functionality, but C++ puts us at a hard impasse; the C++
standard library hardly concerns itself with application-level
functionality like SMTP standards. Anyway, I channeled my frustration at
the lack of a consistent validation scheme for e-mail, which culminated
into bufbuild/protovalidate#236.

For the new failing test cases, we needed to improve the validation of
localpart in C++. Lacking any specific reference point, I decided it
would be acceptable if the C++ version started adopting ideas from
WHATWG HTML email validation. It doesn't move the `localpart` validation
to _entirely_ work like WHATWG HTML email validation, as our version
still has our specific checks, but now we are a strict subset in
protovalidate-cc, so we can remove our additional checks later if we can
greenlight adopting the WHATWG HTML standard.

- The remaining test failures are all related to ignoring validation
rules and presence. The following changes were made:
- The algorithm for ignoring empty fields is improved to match the
specified behavior closer.
- The `ignore` option is now taken into account in addition to the
legacy `skipped` and `ignore_empty` options.
      - Support is added for `IGNORE_IF_DEFAULT_VALUE`
- An edge case is added to ignore field presence on synthetic `Map`
types. I haven't traced down why, but `has_presence` seems to always be
true for fields of synthetic `Map` types in the C++ implementation.
(Except in proto3?)

And with that I think we will have working Editions support.
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

No branches or pull requests

1 participant