-
Notifications
You must be signed in to change notification settings - Fork 81
Improve CI #199
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
Improve CI #199
Conversation
|
I edited the post so that the links point to the correct URL for the |
|
@senceryazici Thanks for doing this! It looks like an improvement to me and I'm ready to try it out, but I have a question about your change to the code_check.sh script. I leave you a comment inline. |
| elif [ $QUICK_CHECK -eq 0 ]; then | ||
| echo $CPPLINT_FILES | xargs python tools/cpplint.py 2>&1 | ||
| fi | ||
| echo "" |
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.
@senceryazici What does this addition do?
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.
Since it is definitely harmless, I'm going to go ahead and merge, but if you can explain I'd appreciate 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.
While testing on my local with 'act' (a tool for running GitHub actions on your local) the code check script constantly failed for some reason. Just for debugging I added a line to the end to see the result and it started passing.
Honestly (just like you've said) since it's totally harmless I didn't spend extra time on it's reason and let it be that way. But I'll definitely take a look at it when I have available time.
M1chaelM
left a comment
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.
This looks good, and we're going to try it out. It sounds like removing the dave repos from dave_sim.repos would ultimately make sense also, though we will do that later just for logistical reasons relating to our upcoming release.
| elif [ $QUICK_CHECK -eq 0 ]; then | ||
| echo $CPPLINT_FILES | xargs python tools/cpplint.py 2>&1 | ||
| fi | ||
| echo "" |
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.
Since it is definitely harmless, I'm going to go ahead and merge, but if you can explain I'd appreciate it.
@M1chaelM If you haven't done it yet, could you post an issue to the tracker to describe what needs to be done so that we have it on our list for post release? |
Hi,
I've configured ROS
industrial_ci(which is almost standard for building ros workspaces) for you to builddave. Also added cmake args for building withlldas linkerscript (faster than ld).Please note that, even though the
daverepository is included in dave_sim.repos (causing CI to build & test it twice), it builds faster due tolld.industrial_ciwill builddaveasupstream_wsfirst (since it's in dave_sim.repos ), and then will build astarget_wssince it's the actual repository, I have not removeddaverepository from dave_sim.repos. You can either remove it from there (preffered), or create another.reposfiles withoutdave, to make CI even faster (about 17 mins.)Also,
.reposfiles with ssh links ( such asgit@github:...) will fail inindustrial_ci. I've added a sed command to replace them withhttpsin this line.