search

jsk-ros-pkg / jsk_robot

jsk-ros-pkg/jsk_robot
https://github.com/jsk-ros-pkg/jsk_robot
73 stars 97 forks source link

Update unitree.rosinstall #1859

Closed sktometometo closed 10 months ago

sktometometo commented 10 months ago

Fix https://github.com/jsk-ros-pkg/jsk_robot/issues/1858

tkmtnt7000 commented 10 months ago

Thank you very much for your catch. I wait for CI before approving.

k-okada commented 10 months ago

Humanity needs to do what computers cannot do. Therefore, if you only check and approve the results of CI, your work will be replaced by computers in the future.

It would be a good idea for each of you to check the version of the branch you are using on the actual machine.　See https://github.com/jsk-ros-pkg/jsk_robot/issues/1858#issuecomment-1749834394 for my case, but @tkmtnt7000 may use newer version.

Note that the submodule was added on June 27 (https://github.com/unitreerobotics/unitree_ros/commit/2e566b78bcdabd4d99be1a0ab7c29cf67e96673b), but on August 7. master branch has been passed, so CI may not be working ( https://github.com/jsk-ros-pkg/jsk_robot/actions/runs/5786126188/job/15680207980 )

sktometometo commented 10 months ago

Humanity needs to do what computers cannot do. Therefore, if you only check and approve the results of CI, your work will be replaced by computers in the future.

It is blocked to merge a pull request without passing CI in this repository. So basically reviewers do not have to check the CI result. So code reviewers should focus on other tasks like code quality, bugs, and knowledge sharing.

sktometometo commented 10 months ago

Current CI seems not to run tests about workspace building and cross compile https://github.com/jsk-ros-pkg/jsk_robot/blob/9a1da3dccba70a646dc42c81f911ef2dd2a5475c/jsk_unitree_robot/jsk_unitree_startup/CMakeLists.txt#L24-L36 So I thinks this PR will not have effect on CI result. But I also think it is better to check these things.

tkmtnt7000 commented 10 months ago

Humanity needs to do what computers cannot do. Therefore, if you only check and approve the results of CI, your work will be replaced by computers in the future.

It is blocked to merge a pull request without passing CI in this repository. So basically reviewers do not have to check the CI result. So code reviewers should focus on other tasks like code quality, bugs, and knowledge sharing.

I understand. I say "Wait for CI" because I usually don't press the approve button before CI passes. I want to cover the cross compile section of this repository...

It would be a good idea for each of you to check the version of the branch you are using on the actual machine.　See https://github.com/jsk-ros-pkg/jsk_robot/issues/1858#issuecomment-1749834394 for my case, but @tkmtnt7000 may use newer version.

My version of unitree ros

tsukamoto@mars:~/unitree_ws/src/unitree_ros$ git log
commit 5dee20c5ba1c86dfeb906e24c7870f19cc93d605 (HEAD -> master, origin/master, origin/HEAD)
Author: xiaoliangstd <2303335747@qq.com>
Date:   Tue Apr 18 12:34:55 2023 +0800

    remove unnecessary collisions
pazeshun commented 10 months ago

@sktometometo @tkmtnt7000 Just FYI: https://github.com/jsk-ros-pkg/jsk_robot/pull/1208#issuecomment-1135488176 Maybe @k-okada just wanted to say we should not only check the results of CI. I think pressing the approve button after CI passes is good for reducing @k-okada 's burden. Before CI passes, we can use "WaitForCI" label.

k-okada commented 10 months ago

Yes, please write the reason why you approved the PR. Ideally, "Confirmed by an actual robot" is best. "Approved because CI passed" is no good.

In this case, "I am using XX commit and have confirmed that it is working, so this change looks good.

You can also uses "MergeOK" button.