Reduce backward-compatibility blast radius of the TestNode changes#1171
Closed
facontidavide wants to merge 2 commits into
Closed
Reduce backward-compatibility blast radius of the TestNode changes#1171facontidavide wants to merge 2 commits into
facontidavide wants to merge 2 commits into
Conversation
The move of TestNode's state behind a PImpl (#1169) removed two things that consumers may rely on: - the transitive includes of script_parser.hpp and timer_queue.h, which test_node.h had always pulled in. Restore them (tagged IWYU keep) so code using e.g. BT::ScriptFunction through this header keeps compiling. - the protected _config member, visible to subclasses since TestNode was introduced. Restore it as a direct member, first in the class so it also sits at the same byte offset as in 4.9. Only the internal machinery (executors, timer, completed flag) stays behind the PImpl; new state must be added there. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The changes since 4.9.1 (TestNodeConfig gaining a field, TestNode and StatusChangeLogger moving state behind a PImpl) are a one-time ABI break: binaries built against 4.9 headers must be recompiled. Bump the minor version so this cannot ship as a patch release that package managers would treat as ABI-compatible. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
Follow-up to #1169 that shrinks the remaining backward-compatibility impact of the TestNode rework, and makes sure the ABI break is signaled by the version number.
Restore TestNode source compatibility
script_parser.hppandtimer_queue.hintest_node.h(taggedIWYU pragma: keep). They had been part of this header since 4.x, so consumers using e.g.BT::ScriptFunctionthrough it keep compiling._configmember that subclasses have seen since TestNode was introduced, placed first in the class so it sits at the same byte offset as in 4.9.1. Only the internal machinery (script executors, timer, completed flag) stays behind the PImpl; any future state goes there.Net effect: every pre-existing field of both
TestNodeandTestNodeConfigkeeps its 4.9 byte offset; onlysizeofgrows, which is the irreducible part of the feature.Bump version to 4.10.0
Master contains one-time ABI breaks (
TestNodeConfiggrowth, TestNode and StatusChangeLogger PImpl) but still declared 4.9.1. Bumping the minor version prevents this from shipping as a patch release that package managers would treat as ABI-compatible.Verification
behaviortree_cpp_test)Not included (release-policy decisions)
libbehaviortree_cpp.so, noSOVERSION), so an ABI-mismatched binary loads and corrupts silently instead of failing at load time. This release would be the cheapest moment to introduce a versioned soname, since consumers must relink anyway._configmust migrate.🤖 Generated with Claude Code