mavgen: always report generator errors in the exit code

[MaEtUgR]

I'm thankful that @nexton-winjeel added the option to have the exit code indicate an error in #863. I'm asking myself why this isn't default. I imagine the generator to be called automatically by CI or like in my case a build step in PX4. Shouldn't it fail by default instead of silently succeeding without a certain dialect properly generated? I would have expected that.

I changed it for PX4 to explicitly add the flag in PX4/PX4-Autopilot#23313 but want to save the next developer having the hassle.

EDIT: To clarify some mistakes e.g. using a non-existing type in a message definition makes the generation throw an exception and fail early. Other mistakes like referring to a non-existing enum don't throw an exception but rather print Enum X in X does not exist and make the mavgen.mavgen() function return False. These should be caught early to not cause problems down the road in the build workflow.

[khancyr]

agree

[hamishwillee]

@peterbarker Can we get some eyes on this? @auturgy ?

[peterbarker]

I'll make sure this comes up at DevCallEU. Frankly I think we should remove the option entirely and just terminate abnormally in case of error. This may break people's workflows somehow, but they were probably already subtly broken....

[MaEtUgR]

I think we should remove the option entirely and just terminate abnormally in case of error. This may break people's workflows somehow, but they were probably already subtly broken....

These are exactly my thoughts. I can adjust the pull request to do that instead. I just tried not to stand on anyone's foot/workflow but it must be broken if it doesn't fail upon a failure.

[tridge]

as long as with the XML in github.com/ArduPilot/mavlink and github.com/mavlink/mavlink doesn't throw errors if we enable the exit code then I'm fine making exiting with an error the default

[hamishwillee]

@peterbarker @MaEtUgR Tested as requested by @tridge above using all.xml and ardupilotmega.xml, using both the current --exit-code and the proposed --disable-exit-code. The value has no effect on our current XML in either mavlink/mavlink or ArduPilot/mavlink message sets.

I'd prefer errors always enabled, so if that change is made would you be able to merge this @peterbarker ?

[peterbarker]

I'd prefer errors always enabled, so if that change is made would you be able to merge this @peterbarker ?

Yes.

[MaEtUgR]

Thanks for the feedback! I simplified the PR to always report failures via the exit code when mavgen.mavgen() returns False. I also updated the PR description to clarify that some message definition errors throw exceptions, but others are caught only by this mechanism. I retested it in the PX4 environment to confirm it behaves as expected.

[peterbarker]

LGTM

[hamishwillee]

Thanks @peterbarker and @MaEtUgR

I've pulled this up to mavlink/mavlink in mavlink/mavlink#2138 (Matthias, this gets auto-pulled into PX4 on a regular basis)