Skip to content
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

Ensure all ROS 2 Bazel projects use .bazelrc testing setup #297

Merged

Conversation

EricCousineau-TRI
Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI commented Aug 18, 2023

As noticed in #295 (review)


This change is Reviewable

Copy link
Collaborator Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+@IanTheEngineer for review, please! I could use some help on organizing, though - if you have suggestions, could I ask you to make a meta PR?
\cc @calderpg-tri

Reviewable status: 0 of 23 files reviewed, all discussions resolved (waiting on @IanTheEngineer)

Copy link
Collaborator Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 23 files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI and @IanTheEngineer)

a discussion (no related file):
Working: This PR currently does two things - try to ensure build flags are the same to help caching, and then ensure test flags are the same to force isolation.

I'm going to split the caching portion off of this, and make this PR only responsible for isolation flags.


@EricCousineau-TRI EricCousineau-TRI changed the title Ensure all Bazel projects use same .bazelrc file Ensure all ROS 2 Bazel projects use .bazelrc testing setup Sep 25, 2023
Copy link
Collaborator Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 23 files reviewed, all discussions resolved (waiting on @IanTheEngineer)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: This PR currently does two things - try to ensure build flags are the same to help caching, and then ensure test flags are the same to force isolation.

I'm going to split the caching portion off of this, and make this PR only responsible for isolation flags.

Done.


Copy link
Member

@IanTheEngineer IanTheEngineer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:LGTM: Thanks for the update, Eric. These changes look good to me, and seem reasonable to help with ROS test isolation.

@adityapande-1995 , the CI failure in this PR claims to be due to running out of space. I know you've been looking into this on other PR's. Do you have any insight on solutions?

Reviewed 18 of 19 files at r2, 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI)


bazel_ros2_rules/ros2/resources/bazel_ros_env/bazel_ros_env.py line 1 at r3 (raw file):

# TODO(eric.cousineau): Move RMW + ROS isolation to a location accessible by

BTW: The TODO is perfectly fine here, but to keep track a a higher level an issue would be helpful.

Move drake_ros...bazel_ros_testing to bazel_ros2_rules...bazel_ros_env
Copy link
Collaborator Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

the CI failure in this PR claims to be due to running out of space

Aye, mentioned in #247 (comment)
My plan is to run testing locally, then merge it.

Reviewable status: 22 of 23 files reviewed, all discussions resolved (waiting on @IanTheEngineer)


bazel_ros2_rules/ros2/resources/bazel_ros_env/bazel_ros_env.py line 1 at r3 (raw file):

Previously, IanTheEngineer (Ian McMahon) wrote…

BTW: The TODO is perfectly fine here, but to keep track a a higher level an issue would be helpful.

Done.

@EricCousineau-TRI
Copy link
Collaborator Author

Local testing passed, merging forward.

@EricCousineau-TRI EricCousineau-TRI merged commit bfa0058 into RobotLocomotion:main Sep 26, 2023
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants