Repository navigation
Conversation
The stdlib header is produced by building to_c_array and running it on std.jsonnet. When cross-compiling, to_c_array is built for the target and cannot run on the build machine, so the build fails. In that case, write the same comma-separated, NUL-terminated byte array from CMake at configure time instead. Native builds are unchanged. Assisted-by: Claude Code:claude-opus-5-5
|
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. |
There was a problem hiding this comment.
Code Review
This pull request introduces conditional logic in stdlib/CMakeLists.txt to handle cross-compilation by packing std.jsonnet at configure time using CMake commands instead of running the to_c_array executable. The reviewer suggests a cleaner, unified approach: using a helper CMake script executed via cmake -P at build time. This works for both native and cross-compiling builds, avoids configure-time overhead, and allows for the complete removal of the to_c_array executable and its source file.
| add_executable(to_c_array EXCLUDE_FROM_ALL to_c_array.cpp) | ||
| configure_target_cc_props(to_c_array) | ||
|
|
||
| add_custom_command( | ||
| COMMENT "Packing std.jsonnet into std.jsonnet.h" | ||
| COMMAND | ||
| to_c_array | ||
| "${CMAKE_CURRENT_SOURCE_DIR}/std.jsonnet" | ||
| "${CMAKE_CURRENT_BINARY_DIR}/std.jsonnet.h" | ||
| OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/std.jsonnet.h" | ||
| MAIN_DEPENDENCY std.jsonnet | ||
| DEPENDS to_c_array) | ||
| if(CMAKE_CROSSCOMPILING) | ||
| # to_c_array is built for the target and cannot run on the build | ||
| # machine, so pack std.jsonnet at configure time instead. Same layout | ||
| # as to_c_array: comma-separated byte values, NUL-terminated. | ||
| set_property(DIRECTORY APPEND PROPERTY CMAKE_CONFIGURE_DEPENDS std.jsonnet) | ||
| file(READ "${CMAKE_CURRENT_SOURCE_DIR}/std.jsonnet" std_hex HEX) | ||
| string(REGEX REPLACE "([0-9a-f][0-9a-f])" "0x\\1," std_bytes "${std_hex}") | ||
| file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/std.jsonnet.h" "${std_bytes}0") | ||
| add_library(stdlib_h INTERFACE) | ||
| else() | ||
| add_custom_command( | ||
| COMMENT "Packing std.jsonnet into std.jsonnet.h" | ||
| COMMAND | ||
| to_c_array | ||
| "${CMAKE_CURRENT_SOURCE_DIR}/std.jsonnet" | ||
| "${CMAKE_CURRENT_BINARY_DIR}/std.jsonnet.h" | ||
| OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/std.jsonnet.h" | ||
| MAIN_DEPENDENCY std.jsonnet | ||
| DEPENDS to_c_array) | ||
|
|
||
| add_library(stdlib_h INTERFACE "${CMAKE_CURRENT_BINARY_DIR}/std.jsonnet.h") | ||
| add_library(stdlib_h INTERFACE "${CMAKE_CURRENT_BINARY_DIR}/std.jsonnet.h") | ||
| endif() |
There was a problem hiding this comment.
Instead of having conditional logic for cross-compiling and maintaining the to_c_array C++ executable, we can perform the hex-packing at build time using a helper CMake script executed via cmake -P.
This approach is fully cross-platform, works identically for both native and cross-compiling builds, avoids configure-time overhead (since it runs at build time), and allows us to completely eliminate the to_c_array target and to_c_array.cpp source file.
# Generate a helper script at configure time to pack std.jsonnet at build time.
# This works identically for both native and cross-compiling builds,
# and avoids the need for the compiled `to_c_array` helper executable.
file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/pack_std_jsonnet.cmake" [=[
file(READ "${INPUT_FILE}" std_hex HEX)
string(REGEX REPLACE "([0-9a-f][0-9a-f])" "0x\\1," std_bytes "${std_hex}")
file(WRITE "${OUTPUT_FILE}" "${std_bytes}0")
]=])
add_custom_command(
COMMENT "Packing std.jsonnet into std.jsonnet.h"
COMMAND ${CMAKE_COMMAND}
-D "INPUT_FILE=${CMAKE_CURRENT_SOURCE_DIR}/std.jsonnet"
-D "OUTPUT_FILE=${CMAKE_CURRENT_BINARY_DIR}/std.jsonnet.h"
-P "${CMAKE_CURRENT_BINARY_DIR}/pack_std_jsonnet.cmake"
OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/std.jsonnet.h"
MAIN_DEPENDENCY std.jsonnet
)
add_library(stdlib_h INTERFACE "${CMAKE_CURRENT_BINARY_DIR}/std.jsonnet.h")
The CMake build can't currently be cross-compiled.
stdlib/CMakeLists.txtgeneratesstd.jsonnet.hby buildingto_c_arrayand running it onstd.jsonnet. When cross-compiling,to_c_arrayis a target binary, so the custom command fails with exit code 127 on the build machine. We hit this adding linux-aarch64 and osx-arm64 builds to conda-forge'slibjsonnetpackage (conda-forge/libjsonnet-feedstock#2), which cross-compiles both.
With this change, when
CMAKE_CROSSCOMPILINGis set, CMake readsstd.jsonnetas hex at configure time and writes the header itself, in the same layoutto_c_arrayproduces: comma-separated byte values followed by a terminating0. It writes the bytes as hex literals instead of decimal, but the values are the same.std.jsonnetis listed inCMAKE_CONFIGURE_DEPENDS, so editing it still regenerates the header. Native builds keep usingto_c_arrayand are unchanged.This approach doesn't strike me as particularly elegant, I'd appreciate suggestions for a nicer solution. This approach should if I'm not mistaken also work for native builds, so there's no need for the conditional, and maybe no need for
to_c_arrayanymore.The solution was suggested by Claude Opus 5.5 and reviewed and tested by me:
to_c_arrayoutput.jsonnetCLI twice, once normally and once with-DCMAKE_SYSTEM_NAME=Linux(which setsCMAKE_CROSSCOMPILINGwhile still producing a runnable binary). Both give the same output for an expression that uses the stdlib.