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

add test to check if ros node is loadable, #463

Merged

Conversation

k-okada
Copy link
Member

@k-okada k-okada commented May 11, 2023

If we use python2 PYTHON_INTERPRETER on 20.04, python2 fails to load rospy in /opt/ros/noetic, because rospy moduels are alraedy updated.

If we use python3 PYTHON_INTERPRETER on 18.04, python3 can load rospy in /opt/ros/melodic by chance.

c.f. #367

@k-okada k-okada force-pushed the fix_ros_google_cloud_language_on_python3 branch 3 times, most recently from 50ae99c to 16b1f58 Compare May 11, 2023 12:02
@k-okada k-okada changed the title check if catkin_virtualenv works add test to check if ros node is loadable, if use python2 PYTHON_INTERPRETER on 20.04, it fails to load rospy in /opt/ros/melodic. On 18.04 and use python3 PYTHON_INTERPRETER, python3 can load rospy in /opt/ros/noetic .... https://github.com/jsk-ros-pkg/jsk_3rdparty/pull/367 add test to check if ros node is loadable, May 11, 2023
@k-okada k-okada force-pushed the fix_ros_google_cloud_language_on_python3 branch 2 times, most recently from cb16ce7 to 6a6ea8a Compare May 18, 2023 07:53
@k-okada k-okada force-pushed the fix_ros_google_cloud_language_on_python3 branch 9 times, most recently from 92457fa to 07ba7b4 Compare May 20, 2023 04:03
@k-okada k-okada force-pushed the fix_ros_google_cloud_language_on_python3 branch from e6143b4 to c40f118 Compare May 21, 2023 05:18
@k-okada k-okada force-pushed the fix_ros_google_cloud_language_on_python3 branch from 0933bf3 to c69ce7f Compare May 27, 2023 03:27
@k-okada k-okada force-pushed the fix_ros_google_cloud_language_on_python3 branch 2 times, most recently from 3257b81 to c98a8d9 Compare May 27, 2023 05:37
If we use python2 PYTHON_INTERPRETER on 20.04, python2 fails to load rospy in /opt/ros/noetic, because rospy moduels are alraedy updated.
If we use python3 PYTHON_INTERPRETER on 18.04, python3 can load rospy in /opt/ros/melodic by chance.

c.f. jsk-ros-pkg#367
@k-okada k-okada force-pushed the fix_ros_google_cloud_language_on_python3 branch from c98a8d9 to 0a9c6a7 Compare May 27, 2023 06:35
…tory from BIN to SHARE, because we want to have same directory structure between devel and install
helper.py depends on dialogflow_task_executive. However, when we add this to the <depend> of package.xml, it appempts to build venv using 'dialogflow_task_executive/requirements.txt'. This requires having the same PYTHON_INTERPRETER for both dialogflow and chat ros package. The issue is that dialogflow_task_executive heavily relies on system Python modules, including ROS, making it difficult to use dialogflow with Python3 on Melodic
@k-okada k-okada force-pushed the fix_ros_google_cloud_language_on_python3 branch 2 times, most recently from bcc2f0e to ed83715 Compare May 27, 2023 08:57
@k-okada k-okada force-pushed the fix_ros_google_cloud_language_on_python3 branch from ed83715 to 019466f Compare May 28, 2023 03:54
Copy link
Member Author

@k-okada k-okada left a comment

Choose a reason for hiding this comment

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

@mqcmd196 @sktometometo please review this PR, I want to merge this and release new version to remove jsk_3rdparty from blacklist, see #468


# python
file(GLOB PYTHON_SCRIPTS scripts/*.py)
catkin_install_python(
PROGRAMS ${PYTHON_SCRIPTS}
DESTINATION ${CATKIN_PACKAGE_BIN_DESTINATION}
)
DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION}/scripts/)
Copy link
Member Author

Choose a reason for hiding this comment

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

This PR changes the installed directory from BIN to SHARE
@mqcmd196

Copy link
Member

Choose a reason for hiding this comment

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

@k-okada
I can understand that *.l libraries prefer to be on the same path for both devel and install. However, are Python files should be the same? They are resolved by rosrun if executable, and if the library, catkin_python_setup, setup.py do. I like to follow the common rule. Or for testing???

Copy link
Member Author

Choose a reason for hiding this comment

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

That's an excellent question. I personaly do not like to install BIN directory, because devel and install should have same structure. Moreover, we've installed scripts and/or nodes under share directory using install CMake command, and it is unclear to me why we should relocate just to support Python3...
However, as an installing under BIN appears to be default behavior and I have changed my mind to fix all CMakeLists.txt, see 15c3e40
http://docs.ros.org/en/jade/api/catkin/html/howto/format2/installing_python.html

Copy link
Member

Choose a reason for hiding this comment

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

devel and install should have same structure.

Yes, I have the same opinion. devel seems to have made the development for ros users easier, but they had to care about the file structures in both devel and install especially when releasing the package. In my understanding, ament and colcon stopped using devel because of it, right...?
https://design.ros2.org/articles/ament.html#:~:text=package%20on%20Windows.-,Devel%20space,-catkin%20has%20the

Anyway, thank you for your update

nfc_ros/CMakeLists.txt Outdated Show resolved Hide resolved
respeaker_ros/scripts/respeaker_node.py Show resolved Hide resolved
ros_google_cloud_language/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Member

@mqcmd196 mqcmd196 left a comment

Choose a reason for hiding this comment

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

lgtm

@sktometometo
Copy link
Contributor

I do not know much about ROS package releasing. But ubuntu-sounds entry for ros_speech_recognition is not merged ros/rosdistro#36857 So ubuntu-sounds entry will not be resolved.
(We have to use sounds in freedesktop-sound-themes).
Does this affect releasing process?

@k-okada
Copy link
Member Author

k-okada commented May 31, 2023 via email

@k-okada k-okada merged commit 81b4ff5 into jsk-ros-pkg:master May 31, 2023
@k-okada k-okada deleted the fix_ros_google_cloud_language_on_python3 branch May 31, 2023 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants