-
Notifications
You must be signed in to change notification settings - Fork 19
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 script for node creation and update templates. #213
base: master
Are you sure you want to change the base?
Conversation
s=${s#*"$delimiter"} | ||
CLASS_NAME="$CLASS_NAME${part^}" | ||
done | ||
echo -e "${TERMINAL_COLOR_USER_CONFIRMATION}ClassName guessed from the '$FILE_NAME': '$CLASS_NAME'. Is this correct? If not, provide it as the second parameter.${TERMINAL_COLOR_NC}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line 39, it says the FILE_NAME
is first parameter, PKG_DEPS
is second and CLASS_NAME
is only third. Here it says provide it as the second parameter, which does not work.
If i only call
setup-ros-node my_node
, PKG_DEPS
is ""
, which is expected
However, if i call
setup-ros-node my_node MyFirstNode
, the class name is still being guessed.
Below, I removed the PKG_DEPS
argument, as it is not used anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right! I have fixed nos using PKG_DEPS. Can you check again?
The thing is that as we are using positional arguments, you have to provide empty second argument to set the third.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then, if i want to define a special class name, I have to know to that the second parameter should be empty, which seems convoluted.
For example, now running:
setup-ros-node my_node MyFirstNode
places <depend>MyFirstNode</depend>
in the package.xml
.
In my opinion, this script is trying to do too much.
This script should generate the required files and boilerplate for CMakeLists.txt
and ROS parameters. That is the most tedious part, as I see it.
Adding dependancies should be done by the user, manually, as we usually add dependancies anyways while developing the node.
I suggest we remove adding package dependancies except <build_depend>generate_parameter_library</build_depend>
, and make the custom node name parameter $2
, in the remote case the class name will not correspond to the file name, but CamelCase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the spirit of my previous comment, can we remove the is the package already configured?
prompt?
when using create-new-package
, we are already "configuring the package" by setting up some CMakeLists.txt
code.
We can just make this script add dependancy to generate_parameter_library
, link the new files and call it a day? This prompt also adds some unecessary complexity, in my opinion.
We can always expand the functionality later, as we use the script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the functionality about adding the is already configured
is when you are adding a second node to already setup package. Not wehn you configure your package with create-new-package
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove adding dependecies, but more the simplicity then chaning the interface. The documentation is clear that if you want to add a custom class (Which is rearly the case to be honest) then you have to provde all three arguemnts. That is how bash works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, I am lazy, and would rather do small fixes later for the dependecies then have to add them manually. We are talking here not about 100% dependecy coverage when you gnerate your file, but more about initial dependecies.
--------- Co-authored-by: Daniel Azanov <[email protected]> Co-authored-by: Dr. Denis <[email protected]>
if [[ "$package_configured" == "no" ]]; then | ||
|
||
# Add `THIS_PACKAGE_INCLUDE_DEPENDS` structure to simplify the rest of the file | ||
TEST_LINE=`awk '$1 == "find_package(ament_cmake" { print NR }' CMakeLists.txt` # get line before `ament_cmake` dependency |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
head
and tail
commands are having trouble parsing the expanded string, I'm getting these warnings:
tail: invalid number of lines: ‘+’
head: unrecognized option '--1'
Try 'head --help' for more information.
tail: invalid number of lines: ‘+’
tail: invalid number of lines: ‘+’
tail: invalid number of lines: ‘+’
This is happening both when $package_configured
is no
or yes
Is this working as expected on your machine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the initial state of your package when testing this?
|
||
# Get line with if(BUILD_TESTING) | ||
TEST_LINE=`awk '$1 == "if(BUILD_TESTING)" { print NR }' CMakeLists.txt` | ||
let CUT_LINE=$TEST_LINE-1 | ||
|
||
# Add Plugin library related stuff | ||
echo "# Add ${FILE_NAME} library related compile commands" >> $TMP_FILE | ||
echo "generate_parameter_library(${FILE_NAME}_parameters" >> $TMP_FILE | ||
echo " ${NODE_PARAMS_YAML}" >> $TMP_FILE | ||
echo ")" >> $TMP_FILE | ||
|
||
echo "add_executable($FILE_NAME $NODE_CPP)" >> $TMP_FILE | ||
echo "target_include_directories($FILE_NAME PRIVATE" >> $TMP_FILE | ||
echo ' "$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/include>"' >> $TMP_FILE | ||
echo ' "$<INSTALL_INTERFACE:include/${PROJECT_NAME}>")' >> $TMP_FILE | ||
echo "target_link_libraries($FILE_NAME ${FILE_NAME}_parameters)" >> $TMP_FILE | ||
echo "ament_target_dependencies($FILE_NAME \${THIS_PACKAGE_INCLUDE_DEPENDS})" >> $TMP_FILE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried adding a my_node
in a package where I already have one node set up.
when selecting package_configured = yes
, the script deletes my whole CmakeLists.txt
and writes only
# Add my_node library related compile commands
generate_parameter_library(my_node_parameters
src/my_node.yaml
)
add_executable(my_node src/my_node.cpp)
target_include_directories(my_node PRIVATE
"$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/include>"
"$<INSTALL_INTERFACE:include/${PROJECT_NAME}>")
target_link_libraries(my_node my_node_parameters)
ament_target_dependencies(my_node ${THIS_PACKAGE_INCLUDE_DEPENDS})
ament_export_libraries(
my_node
)
This is not happening when selecting package_configured = no
in the same package. In that case, it adds the above text to the existing CmakeLists.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, this is a bug then. I didn't tested it at already configured packages. I just copy paseted some thing so we have it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are issues with editing existing CMakeLists.txt
which need to be resolved, mainly when using head
and tail
commands.
Also, there are some features I think are unecessary.
echo "install(" >> $TMP_FILE | ||
echo " TARGETS" >> $TMP_FILE | ||
echo " $FILE_NAME" >> $TMP_FILE | ||
echo " RUNTIME DESTINATION lib/${PROJECT_NAME}" >> $TMP_FILE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
single quotes are required here
echo " RUNTIME DESTINATION lib/${PROJECT_NAME}" >> $TMP_FILE | |
echo ' RUNTIME DESTINATION lib/${PROJECT_NAME}' >> $TMP_FILE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? I want that ${PROJECT_NAME} is parsed to the string. so this should work
Co-authored-by: Nikola Banović <[email protected]>
Co-authored-by: Nikola Banović <[email protected]>
No description provided.