-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: generate filepaths for rosout and rosmaster #18
Conversation
I should add I currently cannot test this as there is a problem with the |
template = "deps.py.tpl", | ||
outs = ["deps.py"], | ||
cmd = """cat <<EOF > $@ | ||
ROSMASTER_PATH = '$(rootpath @ros_comm//:rosmaster)' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, you can't do rootpath on py_binary (as it has >1 files in defaultinfo, known issue). You need to wrap a binary in a symlink (feel free to copy https://github.com/mvukov/rules_ros2/blob/main/third_party/symlink.bzl) and then do rootpath on the symlink target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I cherry pick this branch to my bzl-mod
branch, this is what I get:
$ cat bazel-bin/third_party/ros/roslaunch/deps.py
ROSMASTER_PATH = 'external/_main~non_module_dependencies~ros_comm/rosmaster'
ROSCORE_XML_PATH = 'third_party/ros/roslaunch/roscore.xml'
Is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this patch works as far as I can see. Have you had a chance to test it out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, it was a bit busy here. I just left some small comments.
ROSCORE_XML_PATH = '$(rootpath roscore.xml)' | ||
EOF""", | ||
srcs = ["roscore.xml"], | ||
tools = ["@ros_comm//:rosmaster"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rosmaster should be IMO in srcs, if it's in tools then the target is going to be built in exec config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'll be built in the exec
config in order to get the correct path out, but is that a problem for a python script? It won't actually compile anything and it won't forward on its dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case this is a worry, I've updated it so that the tooling appears in srcs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now what I get when I cherry-pick this onto my bzlmod branch:
$ cat bazel-bin/third_party/ros/roslaunch/roscore.xml
<launch>
<group ns="/">
<node type="external/_main~non_module_dependencies~ros_comm/rosout" name="rosout" respawn="true"/>
</group>
</launch>
$ cat bazel-bin/third_party/ros/roslaunch/deps.py
ROSMASTER_PATH = 'external/_main~non_module_dependencies~ros_comm/rosmaster'
ROSCORE_XML_PATH = 'third_party/ros/roslaunch/roscore.xml'
</group> | ||
</launch> | ||
EOF""", | ||
tools = ["@ros_comm//:rosout"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly here: rosout should be IMO in srcs, if it's in tools then the target is going to be built in exec config.
a75a6bf
to
9344077
Compare
9344077
to
11dfa28
Compare
template = "deps.py.tpl", | ||
outs = ["deps.py"], | ||
cmd = """ | ||
ROSMASTER=($(rootpaths @ros_comm//:rosmaster)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hacky solution IMO, it would be cleaner to go with a symlink as I originally proposed. Right now this is fine, but the contents of $(rootpaths @ros_comm//:rosmaster)
could change and then this could break. To be fixed when needed.
As-is works as expected on the main branch in noblzmod mode. |
No description provided.