Updates to colbuild function #91
Conversation
There was a problem hiding this comment.
@tripAarush Great work on this, I think it is a great start for what we are looking for! I requested some changes, let me know if you need any advice/clarification on them.
Also, this applies to any PR, but you'll notice below that your branch is failing the CI checks for merging in. The details show that it is a pre-commit check that is failing, which generally provides linting for the code to keep things consistent between authors. Running "pre-commit run --all-files" will show you those errors locally (many will be automatically fixed) but you then need to make any changes and re-add the files to git before committing and pushing so that it will pass CI.
|
|
||
| #checking for package parameters | ||
| if [ -n "$1" ]; then | ||
| rm -rf build/"$1" install/"$1" log # Cleaning old files before running colcon (to fix electrical_protocol issue) |
There was a problem hiding this comment.
I would remove this line, since removing the build files will force us to recompile the whole package, even if nothing was modified, which greatly increases compile time. We'll likely need to find another way around that issue but I wouldn't worry about it here.
| colcon build --symlink-install # Build the workspace | ||
|
|
||
| #checking for package parameters | ||
| if [ -n "$1" ]; then |
There was a problem hiding this comment.
can you make this take a variable number of arguments, for running things like "colbuild mil_msgs vectornav" we would want it to build both packages.
| # Build the specific package | ||
| colcon build --symlink-install --packages-select "$1" \ | ||
| --event-handlers console_cohesion+ \ | ||
| --cmake-args -DCMAKE_VERBOSE_MAKEFILE=ON #adding more options |
There was a problem hiding this comment.
Instead of having verbose options on by default when building a single package, or off by default when building the full repository, can you implement it as a check for a "--verbose" command line argument, and add those flags if it is there? For example, we want the following functionality "colbuild mil_msgs" and "colbuild" should not run any of the --event-handlers, console_cohesion, etc. but any combination like "colbuild --verbose" "colbuild mil_msgs --verbose" "colbuild --verbose mil_msgs" should all add those. If you need advice for how to implement that lmk and we can chat.
Issue #81
Made the changes, I didn't add the verbose to just running colbuild because it was printing so much info on the logs so its just there for running colbuild and then a specific package. I also made cb work for everything.