Support scoped=True for IncludeLaunchDescription action#953
Open
paulsohn wants to merge 2 commits intoros2:rollingfrom
Open
Support scoped=True for IncludeLaunchDescription action#953paulsohn wants to merge 2 commits intoros2:rollingfrom
scoped=True for IncludeLaunchDescription action#953paulsohn wants to merge 2 commits intoros2:rollingfrom
Conversation
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
Signed-off-by: Taeseung Sohn <taeseung.sohn@tier4.jp>
5c73d60 to
9ef7ea2
Compare
|
I'm unsure if this completely addresses #801, as this forwards the external scope. This unintentional forwarding could also affect the inner launchfile in unintended ways if it is not accounted for. There are situations where this unintended passing is intended; however, this is not the case described in #801. |
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
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.
Description
Currently
IncludeLaunchDescription(equivalently,<include>in xml) does not have any scoping option, so it leaks the launch configurations and set environment variables to the caller.This variable leak is actually a feature that some ROS projects (e.g. Autoware) deliberately exploit to reuse common preset (i.e. the set of variables) -- so the backward compatibility should be kept, while we also need the ability to lint whether this leakage is intentional.
The behavior of
<include scoped="true" (...) />is equivalent to already possible<group scoped="true"><include (...) /></group>.The implementation is mostly a duplication of
group_action.py. The only difference is that I deferred the support offorwardingfor now (it acts likeforwarding=True.)Fixesafter a discussion I found that #801 also exposes other issues, but nevertheless this PR is highly relevant for what we are missing.#801Compared to its own importance, we've been spending too much time on orthogonal issue about how to make
forwarding=False/ filter what to forward to the includee.Is this user-facing behavior change?
The change is backward compatible, as
IncludeLaunchDescription(scoped=False)is the default behavior. On the other hand the user can specifyIncludeLaunchDescription(scoped=True)to make scoped include.Did you use Generative AI?
The implementation part is done by myself.
Claude Code (Opus 4.6) helped me in generating test cases. A thorough human review involved.
Additional Information