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

Pin version of empy to <4 #632

Merged
merged 6 commits into from
Dec 1, 2023
Merged

Conversation

Taka-Kazu
Copy link
Contributor

@Taka-Kazu Taka-Kazu commented Dec 1, 2023

The main purpose of this PR is to use old version (<4) of Empy.

See colcon/colcon-core#603 for details.

Other changes are made by the git hooks.

@Taka-Kazu Taka-Kazu requested a review from a team as a code owner December 1, 2023 13:15
@Taka-Kazu Taka-Kazu requested review from emersonknapp and christophebedard and removed request for a team December 1, 2023 13:15
Signed-off-by: Kazuki Takahashi <tkhsh.schoen@gmail.com>
Copy link

codecov bot commented Dec 1, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7293e28) 92.47% compared to head (f7ce8ef) 92.47%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #632   +/-   ##
=======================================
  Coverage   92.47%   92.47%           
=======================================
  Files           8        8           
  Lines         186      186           
  Branches       22       22           
=======================================
  Hits          172      172           
  Misses         14       14           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@christophebedard
Copy link
Member

christophebedard commented Dec 1, 2023

Thanks for the PR! Could we instead try to fix it by bumping to colcon-core==0.15.1? I think that would be preferable, but I'm not sure if we still need to then pin empy<4.

…oling#633)

Signed-off-by: Christophe Bedard <christophe.bedard@apex.ai>
@christophebedard
Copy link
Member

Also, please rebase on master to fix CI.

dependabot bot and others added 3 commits December 1, 2023 11:21
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Kazuki Takahashi <tkhsh.schoen@gmail.com>
@Taka-Kazu
Copy link
Contributor Author

@christophebedard Thank you for your comment.

Thanks for the PR! Could we instead try to fix it by bumping to colcon-core==0.15.1? I think that would be preferable, but I'm not sure if we still need to then pin empy<4.

I tried several configuration about colcon-core and empy.

ROS_DISTRO colcon-core empy result
humble 0.11.0 (no constraint) passed
humble 0.15.1 (no constraint) passed
humble 0.11.0 <4 passed
humble 0.15.1 <4 passed
noetic 0.11.0 (no constraint) failed
noetic 0.15.1 (no constraint) failed
noetic 0.11.0 <4 passed but with many version conflict errors
noetic 0.15.1 <4 passed

It seems that the problem is already fixed for humble without additional modification.
But for noetic, the workflow is still broken even with colcon-core==0.15.1.
It might be because colcon is installed via apt for humble but pip for noetic in setup-ros workflow.

So I think it's better to specify both colcon-core==0.15.1 and empy<4. What do you think?

@christophebedard
Copy link
Member

Yeah, you're right. For some reason, I thought that empy was only used by colcon-core, but of course that's not true!

So I think it's better to specify both colcon-core==0.15.1 and empy<4. What do you think?

Sounds good!

Signed-off-by: Kazuki Takahashi <tkhsh.schoen@gmail.com>
Copy link
Member

@christophebedard christophebedard left a comment

Choose a reason for hiding this comment

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

Looks good, thank you very much for the fix!

I'll create a new release after merging this.

@christophebedard christophebedard merged commit 722f1dd into ros-tooling:master Dec 1, 2023
19 of 20 checks passed
@christophebedard
Copy link
Member

christophebedard commented Dec 1, 2023

Released as 0.7.1/v0.7.

@Taka-Kazu
Copy link
Contributor Author

It works! Thank you!

@beniaminopozzan
Copy link

@christophebedard , could this fix be backported to v6 for those who build against EOL distribution?

@christophebedard
Copy link
Member

could this fix be backported to v6 for those who build against EOL distribution?

Please see #634

christophebedard pushed a commit that referenced this pull request Dec 5, 2023
Signed-off-by: Kazuki Takahashi <tkhsh.schoen@gmail.com>
christophebedard pushed a commit that referenced this pull request Dec 19, 2023
Signed-off-by: Kazuki Takahashi <tkhsh.schoen@gmail.com>
moriarty added a commit to PickNikRobotics/ros2_robotiq_gripper that referenced this pull request Feb 8, 2024
- colcon/colcon-core#602
- ros-tooling/setup-ros#632

Signed-off-by: Alex Moriarty <alex.moriarty@picknik.ai>
moriarty added a commit to PickNikRobotics/ros2_robotiq_gripper that referenced this pull request Feb 15, 2024
- colcon/colcon-core#602
- ros-tooling/setup-ros#632

Signed-off-by: Alex Moriarty <alex.moriarty@picknik.ai>
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.

3 participants