Skip to content

{common} Commonize logic in a single script#1634

Open
okjesus wants to merge 1 commit intoros:masterfrom
okjesus:enhancement/scripts
Open

{common} Commonize logic in a single script#1634
okjesus wants to merge 1 commit intoros:masterfrom
okjesus:enhancement/scripts

Conversation

@okjesus
Copy link

@okjesus okjesus commented Jan 25, 2026

Reason for changes

1) Keep this block in sync

In ros-generate-cache.sh and ros-generate-recipes.sh , some instructions were identical with instructions to keep them in sync like the one below:

# Keep this block in sync with the one in ros-generate-recipes.sh .

This commit brings those instructions into a single ros-generate-common.sh script included in both scripts.

Benefit(s):

  • Commonize code
  • Reduce risk of having logic out of sync

2) SCRIPT_NAME

SCRIPT_NAME was set in both ros-generate-cache.sh and ros-generate-recipes.sh with the respective name of the script.
This commit assign SCRIPT_NAME dynamically.

Benefit(s):

  • In case the name of the scripts changes, there will be no impact in SCRIPT_NAME.

…litate maintenance

Signed-off-by: okjesus <1612729+okjesus@users.noreply.github.com>
@robwoolley
Copy link
Collaborator

Hi @okjesus,

Thanks for the contribution. I appreciate the help.

I notice that the commit doesn't follow the Developer Certificate of Origin. It would be important to identify who is making the contribution. You can find a link to the DCO in https://github.com/ros/meta-ros/blob/master/CONTRIBUTING.md

It appears as if this contribution may have been generated with generative AI. I am open to accepting those contributions, but we would need to follow the policies of both OSRF and the Linux Foundation:

This would be the first submission using GenAI, so it may take a few revisions before we are comfortable to merge it.

The scripts themselves don't change very frequently. Creating a common include file would help prevent them from diverging. However, I'm not able to see what problem this solves. Can you provide an explanation of how you came upon this problem and what this solves?

Thanks,
Rob

@okjesus
Copy link
Author

okjesus commented Jan 26, 2026

Thank you very much @robwoolley for the feedback!

I notice that the commit doesn't follow the Developer Certificate of Origin. It would be important to identify who is making the contribution. You can find a link to the DCO in https://github.com/ros/meta-ros/blob/master/CONTRIBUTING.md

I thought I complied with the DCO since the commit message contains the Signed-off-by: ..., but allow me to double check the instructions 🙇 .

It appears as if this contribution may have been generated with generative AI. I am open to accepting those contributions, but we would need to follow the policies of both OSRF and the Linux Foundation:

I can guarantee you that Gen AI was not used to produce this code.
But that is just my side of the story.
If there is a way to check that it was a code produced by Gen AI and that somehow we confirm that it is, I would be happy to close this PR.
It looks robotic and a little bit too clean maybe, but that is just the way we write code in the company where I work,
which is to say the least, very strict on the rules we have to follow.

Even the examples were typed by me; and the style may divulge that I am not a native English speaker.

The scripts themselves don't change very frequently. Creating a common include file would help prevent them from diverging. However, I'm not able to see what problem this solves. Can you provide an explanation of how you came upon this problem and what this solves?

I agree that they don't change very frequently but it doesn't mean there is nothing we can improve.
In terms of problem solving, here is what I think such changes bring to the table:

function/change rationale other benefits
get_ros_version when a new version of ROS is available we don't have to update the switch case in two places but only in one this reduce developers effort and cognitive load (don't forget to update the other script) and facilitate updates and maintenance.
run_rosdep_update if the command rosdep update changes somehow (new tool or whatever), we can update it once not twice. same as above
detect_uncommitted_modifications Very good utility function but implemented twice in two places, so let's create a function. same as above
check_release_date same as above same as above
SCRIPT_NAME the name of the script may change. In case it changes, we are better off with a command to get it than with the hardcoded value. One can forget to rename the script, but forget to update SCRIPT_NAME and thus spend time trying to understand what's going on. With this change, we remove this potential issue. same as above

Overall in terms of programming this check the following boxes:

  • avoid code duplication
  • avoid hardcoded values when possible (SCRIPT_NAME)
  • comment functions
  • etc.

I believe those are good programming practices that are good to implement whenever we can (of course there are exceptions).

Finally, it all started because of this comment:

# Keep this block in sync with the one in ros-generate-recipes.sh .

What better way to keep those instructions in sync than to create functions?
If there was no intention to keep them in sync, I believe the comments would have been removed.
And if we want to keep them in sync functions are good candidates.

To be totally transparent, this is not even the problem I wanted to solve in the first place.
Since I started to integrate meta-ros, I wanted to fix the current potential issues present in the off-highway-can_1.0.0-1.bb recipe and off-highway-can_1.0.0-1.bbappend because the LICENSE instructions seem malformed:

file potential issue potential fix
off-highway-can_1.0.0-1.bb LICENSE = "Apache-2.0,-MIT" LICENSE = "Apache-2.0 & MIT" or
LICENSE = "Apache-2.0 | MIT"
off-highway-can_1.0.0-1.bbappend LICENSE = "Apache-2.0 && MIT" LICENSE = "Apache-2.0 & MIT"

The parsing of the recipes failed, so I wanted to fix it.

But after seeing that those where generated, I checked the script used to generate them (at least this is what I believe at the moment) and ended up seeing the comment

# Keep this block in sync with the one in ros-generate-recipes.sh .

and thought Why? and then after seeing the other scripts, and noticing that those were identical instructions, I just thought we could create functions and use them instead.

And to avoid forgetting about that, I created the PR and continued to check the LICENSE issue.

I don't mind if the PR gets rejected if it doesn't comply with the rules established by the community, I don't want to be disruptive.

Please let me know if you see any issue in the explanation above.
I will check the rules regarding the contribution to check what I have missed.

Sorry again for the inconvenience 🙇 .

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.

2 participants