feat: add runtime_executable adapter for foreign_cc binaries#1539
feat: add runtime_executable adapter for foreign_cc binaries#1539jsun-splunk wants to merge 2 commits into
Conversation
6d453cb to
981f69e
Compare
2ff317b to
b6f6b34
Compare
| )) | ||
|
|
||
| selected_binary = runtime_info.binaries[binary] | ||
| executable = ctx.actions.declare_file(ctx.label.name) |
There was a problem hiding this comment.
This creates a raw Bash script as the rule executable. Existing runnable_binary uses rules_shell's sh_binary wrapper, and this repo runs examples on Windows, so can we confirm this works for bazel run and tools = [...] on Windows? If not, this may need a rules_shell launcher, a .cmd-compatible wrapper, or an explicit target compatibility restriction.
There was a problem hiding this comment.
I've refactored to use sh_binary as the frontend for runtime_executable.
runtime_executable is now a macro that wraps both the private _runtime_executable_wrapper and the public sh_binary.
| ) | ||
|
|
||
| if ( | ||
| runtime_library_search_directories_enabled(ctx) and |
There was a problem hiding this comment.
Design question: should this be a warning, or should enabling runtime_library_search_directories with out_shared_libs require shared_ldflags_vars?
As written, the feature can be explicitly enabled but still silently produce shared libraries without the intended rpaths unless the user also knows the build-system- specific flag variable. That seems like a sharp edge for a feature that otherwise looks like a single opt-in switch.
There was a problem hiding this comment.
I've considered this, it is a problem of wrapping bazel around another build system. If we force runtime_library_search_directories to be tied to shared_ldflags_vars when out_shared_lib is define, we will need the upstream to have a clear separation for exe and shared flags via makevars. Otherwise, the user cannot enabled runtime_library_search_directories at all, even though by default it will still work for its out_binaries.
There was a problem hiding this comment.
Could we make this strict only for explicit per-target enablement?
runtime_library_search_directories = "enabled": fail if out_shared_libs are present, shared runtime search flags were derived, and shared_ldflags_vars is unset.runtime_library_search_directories = "auto": keep the warning when enabled by the global build setting, so broad migrations can still proceed.
That keeps the executable-only/migration case possible, but avoids presenting explicit "enabled" as a single switch when shared outputs are only partially covered.
There was a problem hiding this comment.
This essentially means:
- If enabled via attr in rule, use fail
- if enabled via build setting, use warn
which I think confuses more than clarifies.
I still feel like we should either:
- tighten the API to use the feature by explicitly failing instead of warn or
- keep the looser API and try to add better documentations.
There was a problem hiding this comment.
Ok so I've landed on a variation of what you suggested and tightened the API.
The description on the attr reads:
"runtime_library_search_directories": attr.string(
doc = (
"Controls whether this target derives runtime library search " +
"directories. Use 'auto' to follow the global " +
"@rules_foreign_cc//foreign_cc/settings:runtime_library_search_directories " +
"build setting, 'enabled' to force it on for this target, or " +
"'disabled' to force it off. Global enablement is a strict Unix " +
"conformance switch: Unix targets that declare shared-library " +
"outputs must support the rule-specific shared-link flag hook or " +
"set runtime_library_search_directories = 'disabled'. This is " +
"not supported for Windows C++ toolchains; explicit per-target " +
"'enabled' fails on Windows, while 'auto' with global enablement " +
"is ignored on Windows. On macOS, derived runtime search paths also " +
"require compatible Mach-O @rpath install names and load " +
"commands. This feature passes runtime search directories to " +
"link actions, but does not rewrite installed dylib " +
"IDs or load commands after the upstream install step. Upstream " +
"builds may need to emit or preserve @rpath/... install names."
),
mandatory = False,
values = ["auto", "enabled", "disabled"],
default = "auto",
),Effectively this becomes:
| Platform | runtime_library_search_directories attr |
Global setting | Effective runtime-search state | Additional rule/output checks |
|---|---|---|---|---|
| Unix/macOS | disabled |
any | Off | Fails only if additional_dynamic_runtime_library_search_origins or additional_executable_runtime_library_search_origins is set |
| Unix/macOS | auto |
disabled |
Off | Fails only if additional_dynamic_runtime_library_search_origins or additional_executable_runtime_library_search_origins is set |
| Unix/macOS | auto |
enabled |
On | For make / configure_make: if out_shared_libs is non-empty and shared linker flags exist, shared_ldflags_vars must be set. For meson: shared_ldflags_option must be set. ninja / boost_build fail because runtime search is unsupported. |
| Unix/macOS | enabled |
any | On | Same as above: make / configure_make require shared_ldflags_vars; meson requires shared_ldflags_option; ninja / boost_build fail. |
| Windows | disabled |
any | Off | No runtime-search checks |
| Windows | auto |
disabled |
Off | No runtime-search checks |
| Windows | auto |
enabled |
Off, silently ignored | No runtime-search checks |
| Windows | enabled |
any | Unsupported explicit request | Always fails: Windows does not support explicit per-target runtime search |
| Rule | Runtime search on + out_shared_libs non-empty + shared linker flags exist |
Required attr |
|---|---|---|
make |
Fails if missing | shared_ldflags_vars |
configure_make |
Fails if missing | shared_ldflags_vars |
meson |
Fails if missing | shared_ldflags_option |
ninja |
Always fails on Unix/macOS when runtime search is on | Unsupported |
boost_build |
Always fails on Unix/macOS when runtime search is on | Unsupported |
There was a problem hiding this comment.
ok I've pushed this in changeset.
Primary change is replacing the WARNING messages with enforce_runtime_search_shared_ldflags_attr which will fail analysis based on conditions of runtime attrs.
There was a problem hiding this comment.
Also pushed changeset, to refactor the analysis tests to runtime into 2 separate files.
There was a problem hiding this comment.
and then another refactor of runtime_library_search_directories.bzl into 2 libraries instead.
| is_executable = True, | ||
| ) | ||
|
|
||
| runfiles = ctx.runfiles( |
There was a problem hiding this comment.
Design question: should runtime_executable depend on all DefaultInfo.files from the foreign_cc target? That can pull headers, static libraries, and unrelated declared outputs into the executable's runfiles. Would a runtime-only provider/fileset be cleaner, e.g. selected binary + shared libraries + data outputs needed at runtime?
There was a problem hiding this comment.
Thanks.
I've extended ForeignCcRuntimeExecutableInfo to have the appropriate runtime_files and use that in executable_runtime instead of DefaultInfo.files.
| def _dynamic_libraries(linker_input): | ||
| dynamic_libraries = [] | ||
| for library in linker_input.libraries: | ||
| if library.dynamic_library: |
There was a problem hiding this comment.
This only considers library.dynamic_library when deriving dependency runtime search dirs. Should this mirror the framework’s existing library extraction logic, which also handles resolved_symlink_dynamic_library and interface-library variants? Otherwise a dep whose CcInfo exposes the runtime artifact through the resolved symlink field may be copied/available elsewhere but omitted from the derived rpaths.
There was a problem hiding this comment.
I thought about this during implementation. From what I understand, LibraryToLink.dynamic_library is the solibs symlinks that points to the resolved_symlink_dynamic_library. From my testing, I haven't seen a need to add rpath for resolved_symlink_dynamic_library.
However, I do think adding it does add some coverage if we are not operating in a runfile environment.
05fbd00 to
b1a06e4
Compare
6ba6b7f to
eebcfa5
Compare
| ctx.attr.out_data_files[0].lstrip("/"), | ||
| ) | ||
|
|
||
| return None |
There was a problem hiding this comment.
so each of the outputs are tried, but what happens if it falls through to None
There was a problem hiding this comment.
It is failing a bit later in _origin_short_paths_from_install_tree().
but looking at it now I think that is confusing. I've refactored.
After other refactors, this now looks like this.
3451bb5 to
cee8f57
Compare
Add opt-in runtime library search directory derivation for foreign_cc
outputs. This lets foreign-built binaries and shared libraries resolve
shared libraries from `deps`, `dynamic_deps`, and this rule's own declared
shared-library outputs without relying on loader environment variables such
as `LD_LIBRARY_PATH`.
Public attrs:
- `runtime_library_search_directories`
- `additional_dynamic_runtime_library_search_origins`
- `additional_executable_runtime_library_search_origins`
Example:
```python
configure_make(
name = "python",
out_binaries = ["python3.10"],
out_shared_libs = ["libpython3.10.so"],
runtime_library_search_directories = "enabled",
additional_dynamic_runtime_library_search_origins = [
"lib/python3.10/lib-dynload",
],
deps = [":openssl"],
)
```
`runtime_library_search_directories` accepts `auto`, `enabled`, and
`disabled`. The global build setting defaults to `disabled`, so existing
targets keep the previous behavior unless a target opts in or the build
setting is enabled.
Default origins are derived from declared output `File.short_path` values.
Shared-library link actions use declared shared-library output directories.
Executable link actions use declared binary output directories. Additional
origins are install-tree-relative paths joined under this rule's INSTALLDIR.
The implementation derives dependency origins from `LibraryToLink` dynamic
libraries and adds Bazel `_solib` sibling search paths for solib symlink
layouts. Runtime search derivation is skipped for Windows C++ toolchains.
Wire the runtime search path flags as common implementation flags.
However, support is limited to `cmake`, `configure_make`, `make` and
`meson`. `Ninja` and `boost` support is currently disabled.
Add unit coverage for enablement, default origins, additional origins,
self-output search paths, `_solib` paths, deduplication, and global
build-setting resolution. Add integration coverage for runtime
dependency chains and self-contained output bundles.
cee8f57 to
eebf5f3
Compare
Introduce a `runtime_executable` rule for turning selected foreign_cc binary outputs into executable Bazel targets. This cannot live directly on the existing foreign_cc rules because foreign_cc outputs are not always expected to contain a binary, so those rules cannot always expose an executable. The adapted executable may require `runtime_library_search_directories = "enabled"` on the producing foreign_cc target when it depends directly or transitively on foreign_cc-produced shared libraries. `runtime_executable` is a more Bazel-native version of `runnable_binary` because it exposes the executable through `DefaultInfo.files_to_run` for downstream consumers. The API is intentionally kept similar to `runnable_binary`. For now, it cannot completely replace `runnable_binary` because `runtime_library_search_directories` defaults to `"disabled"` to minimize blast radius while these features mature.
eebf5f3 to
5e616fc
Compare
| self_library_origin_short_paths = _origin_short_paths_from_files(shared_files) | ||
| dep_library_origin_short_paths = _dynamic_library_origin_short_paths_from_deps(ctx) | ||
|
|
||
| return struct( |
There was a problem hiding this comment.
shared_search_directories / executable_search_directories
they are depsets, and every time I read shared or executable I'm thinking a single file, not a list of paths
This PR does 2 things:
runtime_library_search_directoriesfeature to enable rpath on outputs within bazel.runtime_executionrule to wrap foreign_cc outputs into an executable target that is compatible with FilesToRunProvider.runtime_library_search_directoriesfeatureAdd opt-in runtime library search directory derivation for foreign_cc
outputs. This lets foreign-built binaries and shared libraries resolve
shared libraries from
deps,dynamic_deps, and this rule's own declaredshared-library outputs without relying on loader environment variables such
as
LD_LIBRARY_PATH.Public attrs:
runtime_library_search_directoriesadditional_dynamic_runtime_library_search_originsadditional_executable_runtime_library_search_originsExample:
runtime_library_search_directoriesacceptsauto,enabled, anddisabled. The global build setting defaults todisabled, so existingtargets keep the previous behavior unless a target opts in or the build
setting is enabled.
Default origins are derived from declared output
File.short_pathvalues.Shared-library link actions use declared shared-library output directories.
Executable link actions use declared binary output directories. Additional
origins are install-tree-relative paths joined under this rule's INSTALLDIR.
The implementation derives dependency origins from
LibraryToLinkdynamiclibraries and adds Bazel
_solibsibling search paths for solib symlinklayouts. Runtime search derivation is skipped for Windows C++ toolchains.
Wire the runtime search path flags as common implementation flags.
However, support is limited to
cmake,configure_make,makeandmeson.Ninjaandboostsupport is currently disabled.runtime_executablerule to used with FilesToRunProvider compatible consumers.Introduce a
runtime_executablerule for turning selected foreign_cc binary outputs into executable Bazel targets. This cannot live directly on the existing foreign_cc rules because foreign_cc outputs are not always expected to contain a binary, so those rules cannot always expose an executable.The adapted executable may require
runtime_library_search_directories = "enabled"on the producing foreign_cc target when it depends directly or transitively on foreign_cc-produced shared libraries.runtime_executableis a more Bazel-native version ofrunnable_binarybecause it exposes the executable throughDefaultInfo.files_to_runfor downstream consumers. The API is intentionally kept similar torunnable_binary. For now, it cannot completely replacerunnable_binarybecauseruntime_library_search_directoriesdefaults to"disabled"to minimize blast radius while these features mature.