feat: support module-local generated C++ output (#19)#20
Merged
Conversation
The "API dump does not exist" error path referenced `aidlInterface.baseName`, which is not defined in scope — the local variable is `interface` and the attribute is `base_name`. Hitting this path (e.g. a first-time/unfrozen interface) raised a NameError instead of printing the intended guidance. #19
Adds a `layout` knob to the interface definition so generated C++ and versioned AIDL can live in the interface's own directory tree (`<module>/<version>/`) instead of the central `stable/` tree. The central layout remains the default; nothing existing changes. - aidl_interface.py: `layout: module-local` (interface.yaml) points `interface_root_stable` / `interface_gen_dir` at the interface's module directory. The legacy `frozen_location` field is still honoured for back-compat. Store `workspace_root` (first interfaces root) and use it in `get_preprocessed_dir()` instead of the brittle `dirname x 3` derivation, which assumed a `stable/aidl/<module>` path. - aidl_gen_rule.py: in module-local layout the version directory is the interface's own directory name (`current`, `0.1.0.0`, ...). - CMakeLists.inc: decouple the generated-source base (`LINUX_BINDER_AIDL_GENERATED_ROOT`, new, defaults to `LINUX_BINDER_AIDL_ROOT_OUT`) from the intermediate `out/` directory, so a module-local consumer can resolve generated sources under its repo root while keeping intermediates separate. Verified: an interface with `layout: module-local` generates C++ into its own directory and builds; a default (central) interface is unaffected. #19
There was a problem hiding this comment.
Pull request overview
Adds an optional “module-local” artifact layout to the host-side AIDL tooling so generated C++ can be emitted alongside an interface’s own versioned directory (e.g. <module>/<version>/{include,src}) rather than only via a central shared tree, while keeping the existing “central” behavior as the default.
Changes:
- Introduces
layout(centralvsmodule-local) andworkspace_roothandling inAidlInterface, and usesworkspace_rootto compute the preprocessed-AIDL directory without assuming astable/aidl/<module>path shape. - Updates source-generation rules so module-local interfaces derive the on-disk “version directory” from the interface directory name.
- Extends the CMake helper to allow a separate root for generated sources (
LINUX_BINDER_AIDL_GENERATED_ROOT) distinct from the intermediateout/directory, and fixes an existingNameErrorin an error path.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
host/CMakeLists.inc |
Adds LINUX_BINDER_AIDL_GENERATED_ROOT and uses it when resolving generated source/include paths. |
host/aidl_interface.py |
Adds layout + workspace_root; updates stable/gen directory resolution for module-local vs central layouts; fixes preprocessed dir anchoring. |
host/aidl_gen_rule.py |
Adjusts version-directory selection for module-local generation; fixes a NameError in an error message path. |
Comment on lines
+181
to
+185
| # Generated sources live under LINUX_BINDER_AIDL_GENERATED_ROOT (which | ||
| # defaults to LINUX_BINDER_AIDL_ROOT_OUT). For module-local consumers this | ||
| # is the repo root, so the path resolves to <repo>/<module>/<version>/. | ||
| set(source_dir ${LINUX_BINDER_AIDL_GENERATED_ROOT}/${interface_name}/${interface_version}/src) | ||
| set(include_dir ${LINUX_BINDER_AIDL_GENERATED_ROOT}/${interface_name}/${interface_version}/include) |
Comment on lines
+967
to
+972
| # Versioned AIDL and generated C++ live in the interface's own | ||
| # directory tree: <module>/<version>/. interface_root is the | ||
| # <module>/<version> directory, so its parent is the module dir. | ||
| module_dir = path.dirname(self.interface_root) | ||
| self.interface_root_stable = module_dir | ||
| self.interface_gen_dir = module_dir |
Comment on lines
+204
to
+206
| # In module-local layout the version IS the interface's own | ||
| # directory name (e.g. "current", "0.1.0.0"). | ||
| version_dir = path.basename(interface.interface_root) |
- aidl_api.py: update-api (-u) is a no-op in module-local layout — the AIDL already lives in its versioned directory, so the old rsync would copy the directory onto itself. Skip with an info message. - aidl_gen_rule.py: reject -v/--version for a module-local interface with a clear error — the version is selected by which interface directory is built, not by a flag (it was previously ignored silently). - CMakeLists.inc: document the LINUX_BINDER_AIDL_GENERATED_ROOT contract (must resolve to where the generator emits; a mismatch fails loudly at the post-generation re-check, never silently). #19
Contributor
Author
Response to Copilot review (commit
|
…530)
Previously, the AIDL->C++ regeneration step in CMakeLists.inc only ran
when <module>/current/include and <module>/current/src were empty. Once
those directories held any pre-generated artefacts (from a prior build,
a stale checkout, or a snapshot copy) the regen was silently skipped -
even when the source AIDL had changed. Contributors edited AIDL, saw
"Build completed successfully", and shipped PRs whose committed
generated C++ did NOT match the AIDL they actually changed.
This commit applies Option A from rdk-halif-aidl#530:
- Always regenerate when interface_version == "current". The AIDL
compiler overwrites in place, so a fresh regen brings the tree to
byte-equivalent state with the AIDL. Cost is negligible relative
to compilation; deterministic regen eliminates the entire class of
"ship stale generated code" bugs.
- Frozen versions (e.g. 0.1.0.0) continue to skip regen when
pre-committed sources exist - those are part of the release
contract and must remain immutable.
- Defensive fallback: if a regen run produces no files in the
expected source_dir (e.g. a component whose loader maps to a
different versioned directory), fall back to pre-committed
sources with a WARNING rather than failing the build.
Additionally, host/aidl_interface.py now sorts the discovered
interface.yaml locations so that "<module>/current/interface.yaml"
is always inserted last into the aidl_interfaces dict. Without this,
filesystem-dependent os.walk order could leave the dict pointing at
a frozen snapshot's interface.yaml, causing the always-regen above
to write into the snapshot directory and dirty the working tree
(observed on audiomixer/0.1.0.1 and compositeinput/0.2.0.0 in
rdk-halif-aidl). Stable ordering keeps "current" canonical for the
generator regardless of underlying filesystem behaviour.
Verified against the rdk-halif-aidl#530 acceptance reproducer:
1. Edit avclock/current/com/rdk/hal/avclock/IAVClock.aidl
(added a probe method).
2. Run ./build_modules.sh avclock from the rdk-halif-aidl checkout.
3. git diff avclock/current/include/ now shows the regen reflecting
the AIDL edit (previously: empty diff, stale C++ shipped).
./build_modules.sh all also builds 26 libraries clean with no working
tree changes in any snapshot directory.
Three small follow-ups to PR #21 surfaced by Copilot review: 1. Argument quoting (real bug). `interface_version_arg` was set as a single string `"-v ${interface_version}"`, so `execute_process` passed it to `aidl_ops.py` as one argv token. `aidl_ops.py` uses `getopt` and would reject the combined token on clean builds of a frozen version that lacks pre-committed sources. Switched to a CMake list (`set(interface_version_arg -v ${interface_version})`) so the flag and value expand to two separate argv entries. 2. Frozen-version comment. The doc string said frozen versions "never regenerate", which contradicted the elseif branch that bootstraps a frozen version once when no pre-committed sources exist. Tightened the comment to describe the actual two-state behaviour: reuse pre-committed sources when present, generate once to bootstrap otherwise. 3. Error message. Both FATAL_ERROR sites pointed at `./build_interfaces.sh`, which lives in the consumer repo (rdk-halif-aidl), not in linux_binder_idl. Removed the bogus suggestion and pointed users at the aidl_ops.py output above.
fix(build): always regenerate C++ from AIDL on build (rdk-halif-aidl#530)
Comment on lines
+231
to
+247
| if(NOT source_files) | ||
| # Defensive fallback: if regen ran cleanly but produced no files in | ||
| # the expected location (e.g. a component whose interface loader | ||
| # maps to a different versioned directory than CMake's "current" | ||
| # path), fall back to any pre-committed sources so the build | ||
| # remains usable. This preserves the previous behaviour for those | ||
| # components without losing the new always-regen guarantee for the | ||
| # common case. | ||
| if(existing_sources) | ||
| list(LENGTH existing_sources list_length) | ||
| message(WARNING "Regeneration produced no files in ${source_dir}; " | ||
| "falling back to ${list_length} pre-committed source(s). " | ||
| "Generated C++ may be stale relative to AIDL.") | ||
| set(source_files ${existing_sources}) | ||
| else() | ||
| message(FATAL_ERROR "Source generation completed but no C++ files found in ${source_dir}") | ||
| endif() |
| @@ -166,29 +186,81 @@ function(AddInterfaceLibrary aidl_target) | |||
|
|
|||
| list(GET interfaces_dir 0 first_interfaces_root) | |||
Comment on lines
+195
to
+206
| # Regeneration policy (#530, Option A): | ||
| # - "current" version: ALWAYS regenerate. The AIDL under <module>/current/ | ||
| # is mutable and the previous "skip if include/src already populated" | ||
| # guard silently shipped stale C++ when the AIDL changed but the | ||
| # generated tree happened to exist (e.g. from a prior build or a | ||
| # stale checkout). The AIDL compiler overwrites in place, so a | ||
| # successful regen brings the tree to byte-equivalent state with | ||
| # the AIDL; if a method or type is removed, its primary file is | ||
| # overwritten with the new content. Regen is cheap relative to | ||
| # compile and eliminates the "ship stale generated code" class of | ||
| # bug entirely. | ||
| # - Frozen versions (e.g. v1, v2): pre-committed C++ is part of the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #19. Lets the AIDL→C++ generator emit generated code into the
interface's own directory (
<module>/<version>/{include,src}) instead of acentral
stable/generated/tree.This unblocks the rdk-halif-aidl #493
restructure (self-contained component directories). It is additive — the
central layout stays the default and existing behaviour is unchanged; the
aidl_ops -u/-f/.hashmachinery is untouched.Changes
host/aidl_interface.py— newlayoutfield in the interface definition:central(default) — sharedstable/tree, as today.module-local—interface_root_stable/interface_gen_dirpoint at theinterface's own module directory; AIDL is read in place, generated C++ is
co-located.
frozen_locationfield is still honoured (interface→module-local).workspace_rootand use it inget_preprocessed_dir()instead of thebrittle
dirname x 3derivation (which assumed astable/aidl/<module>pathand broke for any other layout).
host/aidl_gen_rule.py— inmodule-locallayout the version directory isthe interface's own directory name (
current,0.1.0.0, …).host/CMakeLists.inc— newLINUX_BINDER_AIDL_GENERATED_ROOT(defaults toLINUX_BINDER_AIDL_ROOT_OUT) decouples the generated-source base from theintermediate
out/directory, so a module-local consumer can resolve generatedsources under its repo root.
NameError(aidlInterface→interface.base_name)in
gen_cpp_sources's error path.Verification
In a consumer repo, an interface with
layout: module-localwas built: generatedC++ landed in
<module>/current/{include,src}and the module library compiled.A default (
central) interface in the same build was unaffected — no regression.No "frozen registry"
This is intentionally not Android-style frozen-AIDL machinery. Released
<module>/<version>/directories are controlled releases (immutable byconvention + review); a release carries a
.hashfor integrity, which thegenerator already reads from the interface's own directory when present.
Enforced cross-version compatibility is a separate future decision.