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

Update unitree.rosinstall #1859

Merged
merged 2 commits into from
Oct 6, 2023
Merged

Update unitree.rosinstall #1859

merged 2 commits into from
Oct 6, 2023

Conversation

sktometometo
Copy link
Contributor

Fix #1858

@tkmtnt7000
Copy link
Member

tkmtnt7000 commented Oct 6, 2023

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

@k-okada
Copy link
Member

k-okada commented Oct 6, 2023

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 #1858 (comment) for my case, but @tkmtnt7000 may use newer version.

Note that the submodule was added on June 27 (unitreerobotics/unitree_ros@2e566b7), 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
Copy link
Contributor Author

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
Copy link
Contributor Author

Current CI seems not to run tests about workspace building and cross compile

if(CATKIN_ENABLE_TESTING)
find_package(catkin REQUIRED COMPONENTS roslaunch roslint)
file(GLOB LAUNCH_FILES launch/*.launch)
# TODO: Do not do roslaunch_add_file_check()
# foreach(LAUNCH_FILE ${LAUNCH_FILES})
# roslaunch_add_file_check(${LAUNCH_FILE})
# endforeach()
set(ROSLINT_PYTHON_OPTS --max-line-length=180 --ignore=E221,E222,E241) # skip multiple spaces before/after operator
roslint_python()
roslint_add_test()
endif()

So I thinks this PR will not have effect on CI result. But I also think it is better to check these things.

@tkmtnt7000
Copy link
Member

tkmtnt7000 commented Oct 6, 2023

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 #1858 (comment) 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 <[email protected]>
Date:   Tue Apr 18 12:34:55 2023 +0800

    remove unnecessary collisions

@pazeshun
Copy link
Contributor

pazeshun commented Oct 6, 2023

@sktometometo @tkmtnt7000 Just FYI: #1208 (comment)
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
Copy link
Member

k-okada commented Oct 6, 2023

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.

@k-okada k-okada merged commit b9acd23 into master Oct 6, 2023
8 checks passed
@k-okada k-okada deleted the sktometometo-patch-1 branch October 6, 2023 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Unitree] duplicated ROS packages in environment
4 participants