Add a --robust build option.

[flixr]

Add the --robust option to the 'build' verb:
If given it will try to build all the remaining packages instead of just completing the already started ones.
This is similar to the --robust option that rosmake has.

Also add a build summary at the end that displays for each package that should be built if it was sucessful, failed or not built at all (e.g. because was aborted after an error without the robust option or because of dependency problems).

[jbohren]

Oh this sounds cool.

[mjschuster]

👍 works here, looks cool and is really useful. Thanks @flixr!

[wjwwood]

Thanks for the pull request @flixr, especially for fixing all of my typos 😛.

I like that feature, and the implementation looks solid.

One proposal, what do people think about --continue-on-error or --continue-on-failure, with a shortcut as -c, instead of --robust? My reaction to first reading the title of this issue was: "What does --robust do?" I think a more descriptive option name would be better.

[flixr]

I have no strong opinion on the option name, just chose --robust to be similar to rosmake.
--continue-on-failure might be a bit more clear though.

[tfoote]

Make uses -k, --keep-going , and rosmake supported -k too. It might be good
to use the same syntax.

From make's man page http://unixhelp.ed.ac.uk/CGI/man-cgi?make

   -k, --keep-going
        Continue  as  much  as  possible after an error.  While the target
        that failed, and those that depend on it, cannot  be  remade,  the
        other dependencies of these targets can be processed all the same.

On Thu, Jan 22, 2015 at 10:35 AM, Felix Ruess [email protected]
wrote:

I have no strong opinion on the option name, just chose --robust to be
similar to rosmake.
--continue-on-failure might be a bit more clear though.

—
Reply to this email directly or view it on GitHub
#138 (comment).

[wjwwood]

What I want to avoid here is the confusion between:

• It will try to build all packages even if the packages they depend on failed to build and...
• It will try to build all packages which can be built (its dependencies built successfully), but failed packages and the packages which depend on them will not.

I'm not sure how we can achieve this while continuing to be brief. Maybe something like:

Try to continue building packages whose dependencies built successfully even if some packages fail to build.

[wjwwood]

I think --continue-on-failure (with -c as a shorthand) is the best name for people skimming through the documentation or -h.

I guess I would be ok with having -k and --keep-going as options which do the same thing if we wanted to emulate make, but only as secondary aliases to --continue-on-failure. But honestly my gut feeling is to leave them out.

[wjwwood]

Speaking of documentation, @flixr would you have a look at this page:

https://github.com/flixr/catkin_tools/blob/build_robust/docs/verbs/catkin_build.rst

And maybe add a note or even a whole section dedicated to this feature? It would be great to get that in this pull request.

[wjwwood]

Other than changing the option name and adding/updating the existing documentation I give this a +1.

[flixr]

So changed the option name to --continue-on-failure,-c now and added some short docs.

--keep-going,-k would also be a good name.

[jbohren]

+1 for either name

[wjwwood]

The build summary is nice, but I think that it hides the error summary by scrolling the terminal a lot.

I would propose that it come before the error summary, thoughts?

[flixr]

Personally I prefer to have the build summary at the end to have a better overview of what was built and what not.
If I want to check why packages failed I will have to scroll up in most cases anyway...

[wjwwood]

Maybe we should combine those summaries since they are somewhat overlapping. I'll look into it early next week.

[flixr]

I would prefer it if we don't combine it, as it IMHO gives the quickest overview like it is now and the error summary doesn't really help much in a lot of cases anyway...

[wjwwood]

I agree that the error summary doesn't really help other than telling you what failed, but in the case that I have a bunch of successful packages and one that failed early on, this list makes me scroll to see what failed. At the very least I think we need to get the line like [build] There were '2' errors: to come at the end.

I will work out what I think would work best and propose it with another pull request to this pull request, then we can discuss the details there. Once we have decided we can merge my pull request and then this one.

[mjschuster]

I agree that the error summary doesn't really help other than telling you what failed, [...]

It should also help to see which packages have not been built due to dependencies that failed (and when not using the "-c" option, also to see which packages have not started their build process at all because a failure of another package stopped the whole execution beforehand)

Another suggestion to work around the scrolling issue might be to have columns for the summary output, which should significantly shorten the required number of lines. A one-line summary in the very end, as @wjwwood suggested, sounds also good to me.

I agree with @flixr and personally also prefer the new summary to the error summary, as it gives a quick overview. In contrast, to address the individual errors, I have to scroll and look up the context anyway. The failed command and error code alone aren't that helpful for me in most cases. However, one way to combine them might be to give a short hint on the reason of failure in the summary, e.g. "cmake error", "make error", "dependency error", ...

[wjwwood]

Ok, sorry for the delay, I opened a pull request against this pull request: flixr#1

I tried to balance the points made here, but it still feels a bit much to have at the tail end of the build. I thought about excluding the "not built" packages from the summary and just including them in the number on the second to last line, but I didn't try that.

Feedback is welcome.

[flixr]

IMHO it is quite nice now as it gives a good overview/summary and doesn't use that much space.
Especially considering that most workspaces are probably have a few packages less than indigo-desktop, since they are overlays on top of that ;-)
And please leave the "not built" packages info in there...

[wjwwood]

Ok, I guess we can keep the "not built" packages, but I still believe it is overly verbose on large workspaces, and that just mentioning the number not built in the second to last line is good enough for most cases.

If anyone else has feedback, please speak up now. Also, I'll cross post the images from my latest iteration which @flixr merged into this pull request:

[flixr]

One option could be to only show the detailed summary with all packages listed if it was invoked with the --continue-on-failure option.
And then maybe add an option --summary to explicitly enable it for any case...

But IMHO this can still be done after merging this PR, as it is already much more useful/usable than without this...

[jbohren]

I don't think showing the unbuilt and successful packages is that helpful, and it's sort of painful with over 150 packages in a workspace and a 1366x768 screen. I'm fine with an additional option like --build-summary, though.

[wjwwood]

One final thing I wanted to try to resolve before merging this was the behavior when you do ctl-c, I would like that even when --continue-on-failure is passed that it stops trying after a ctrl-c.

I noticed it before, but I hesitated because I didn't know the best way to do it and still have the signal travel down through the executors. I'll have a look at that and merge this at the same time.

[wjwwood]

Never mind, the exception handling seems to be working ok, there aren't any changes related to that which need to be made.

@jbohren So what exactly would you suggest instead (maybe you could outline a more detailed scenario and how you'd like it to look and act)? Should it not print any of the "build summary" unless asked for? I tend to agree that it isn't useful most of the time, but it dose make it easier to answer "what was not built?". If you don't summarize that then figuring it out otherwise involves looking at what was to be built and what failed and then trying to manually figure out what has and has not been built.

[jack-oquin]

Does the "not built" list include everything in the workspace, even those not requested on this build? If so, I am against it. I often have lots of packages sitting around in my workspace that are irrelevant to the current build operation.

If you are only listing the packages that were requested to build, but were not, that seems OK.

[wjwwood]

@jack-oquin I believe it only lists the packages that would have been built, so in the case that you specify one package, only it and its recursive dependencies would be in that list, and other packages in the workspace would not.

[flixr]

Yes, only the requested packages and their dependencies (if they are in the same workspace and could be built) are listed.
An option to enable the summary would be ok, although I still think it makes sense to enable it by default when you passed the --continue-on-failure option.

[wjwwood]

I merged this manually to master with a few changes. I've updated the behavior to follow these rules:

• A build summary is printed always when --summarize is used.
• A build summary is never printed when --no-summarize/--no-summary is used.
• A build summary is printed when there are errors and --continue-on-error is used.
• Otherwise a build summary is not printed.

I think that's a good set of defaults, but if you guys would like to continue discussing and refining that, please open a new issue. Keep in mind that you can always add the --summary option to a verb alias if you prefer it to always print.

[flixr]

👍 Thanks!