Don't build a shared library for embedded data - #4993
marketchedai wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
5d0310c to
1884b34
Compare
|
Rebased onto When I opened this, I cited The rule itself is still broken on macOS, though — the failing target is now the rule's own test, |
|
The Google cla must be agreed to before we can review or merge this pr. |
|
@marketchedai The Google cla must be agreed to before we can review or merge this pr. |
1884b34 to
ad5a30e
Compare
xls_cc_embed_data produces both a static and a shared library. Linking the shared variant fails on macOS because the toolchain passes the ELF-only -soname flag to the Mach-O linker: ld64.lld: error: unknown argument '-soname=libxls_Sbuild_Urules_Stests_Slibarbitrary_Udata_Ufile.dylib' This blocks any test target depending on embedded data, since tests link their dependencies dynamically by default. On current main that includes the rule's own test: //xls/build_rules/tests:embed_data_test cannot be built on macOS at all. Embedded data is only ever linked statically into its consumers, so the shared library is an unused artifact -- no target in the tree depends on it. Passing disallow_dynamic_library skips producing it, which avoids the broken link on macOS and drops a dead artifact on every platform. Verified on macOS arm64 against 924b9fd: //xls/build_rules/tests/... passes 31/31 with this change and embed_data_test fails to build without it; //xls/common/... passes 37/37; the four main tools still build; and //xls/common:subprocess_helper_embedded still builds under --dynamic_mode=fully. Co-authored with Claude Code (https://claude.com/claude-code).
ad5a30e to
341fd64
Compare
|
Again you must agree to the CLA for PRs to be considered: https://github.com/google/xls/pull/4993/checks?check_run_id=107370647803 |
Fixes #4992.
xls_cc_embed_dataproduces both a static and a shared library. The sharedvariant fails to link on macOS, because the toolchain passes the ELF-only
-sonameflag to the Mach-O linker:This blocks any test target that depends on embedded data, because
cc_testlinks its dependencies dynamically by default. On current
mainthat includesthe rule's own test —
//xls/build_rules/tests:embed_data_testcannot be builton macOS at all.
Embedded data is only ever linked statically into its consumers, so the shared
library is an unused artifact: nothing in the tree depends on it. Passing
disallow_dynamic_library = Trueskips producing it, which avoids the brokenlink on macOS and drops a dead artifact on every platform.
If you would prefer this fixed in the toolchain configuration so the shared
variant links correctly on Mach-O, I am happy to redo it that way; I took the
minimal route.
Testing
Tested on macOS (arm64, macOS 26.6.2) only, with Bazel 8.7.0, rebased onto
924b9fd. I have not built this on Linux — relying on CI for that. The
change is platform-independent Starlark; its effect on Linux is that the
.sois no longer produced, and consumers link the static library as they already
did.
//xls/build_rules/tests/...passes 31/31 with this change, andembed_data_testfails to build without it on macOS.//xls/common/...passes 37/37.interpreter_main,ir_converter_main,opt_mainandcodegen_mainbuild,and a DSLX -> IR -> opt -> Verilog run produces correct output.
//xls/common:subprocess_helper_embeddedstill builds under--dynamic_mode=fully.Only two targets use this rule (
//xls/common:subprocess_helper_embeddedandthe test's
arbitrary_data_file), andrdepsshows no consumer of the sharedvariant.
🤖 Generation assisted with Claude Code