[Feature] Mechanism to get an error when we explicitly ask to continue on error

[oliv3r]

Describe the bug
The KiBot DRC handler will return 0 from the binary, when there are actually errors. For ERC, I do not see this issue, as ERC will fail actually ...

kibot:
  version: 1

import:
  - file: 'kici_common.kibot.yaml'

preflight:
  drc:
    all_track_errors: true
    dir: 'reports'
    dont_stop: true
    enabled: true
    format: 'CSV,HTML,JSON,RPT'
    warnings_as_errors: true
  erc:
    dir: 'reports'
    dont_stop: true
    enabled: true
    format: 'CSV,HTML,JSON,RPT'
    warnings_as_errors: true

To Reproduce

% cd /tmp
% docker run \
    --env HOME='/tmp' \
    --interactive \
    --pull always \
    --rm \
    --tty \
    --user "$(id -u):$(id -g)" \
    --volume "$(pwd):/workdir" \
    --workdir "/workdir" \
    'ghcr.io/inti-cmnb/kicad9_auto_full:dev' \
    '/bin/bash'
$ git clone https://gitlab.com/ci-includes/kici.git
$ cd kici/hello
$ git reset --hard v0.10.0
$ export CI_JOB_NAME='KiCi'
$ kibot --defs-from-env  --plot-config '.kibot/kici_drc.kibot.yaml'

Expected behavior
When there are errors, we should always 'return -1' or similar so that CI/CD can catch these failed return values.

Screenshots

I have no name!@8d1bdcc025ab:/workdir/kici/hello$ kibot --defs-from-env --plot-config '.kibot/kici_drc.kibot.yaml'
Using SCH file: hello.kicad_sch
- Running the DRC
ERROR:1 DRC errors detected (kibot.pre_drc - pre_drc.py:151)
ERROR:(lib_footprint_mismatch) Footprint 'CP_Radial_D6.3mm_P2.50mm' does not match copy in library 'Capacitor_THT'.
    @(125.32 mm, 57.46 mm): Footprint C1
    Check: Violations (kibot.pre_drc - pre_drc.py:166)
ERROR:DRC errors: 1 (kibot.gs - gs.py:900)
Found 0 unique warning/s (1 total, 1 filtered)

Additional context
What I really want, in summary as I described a few times :p,
a) Warnings are Treated as Errors. While warnings are 'okay' in some cases, users may want to fix warnings. Since pipelines cannot tell you if it's a warning or an error, all warnings would be blindly 'green' as nothing is deemed wrong.
b) When there are errors, the return code should be non-0, so pipelines can fail on this.
c) Kibot should be able to continue running in certain cases, to produce all outputs. E.g. when doing ERC/DRC, kibot should not stop on the first error, make the user fix it, and then 'oh hey, there was another error'. Instead all errors/warnings should be printed so we can see this output. (e.g. internally a global 'return_status = 0' and whenever something generates an error return value, return_status = error and in the end exit return_status (in poor mans pseudo code) :)

[set-soft]

@oliv3r I don't see any bug here, if you ask to continue on error you are actually ignoring the errors. So I don't see any fail to comply with the specs, no bug.
Also note that KiBot doesn't return "-1" it returns specific codes indicating what failed. And note that we could be ignoring multiple errors, so which one will be reported?
I understand you want to stop, but this is something that can be added, this is why I label it as a new feature.

[set-soft]

BTW: KiBot clearly differentiate warnings and error in its output.
You can parse its output to know about them.

[set-soft]

The above patch adds an option that should exit with error for ignored errors
Note that the behavior on ambiguous situations will be ... ambiguous.

[oliv3r]

@oliv3r I don't see any bug here, if you ask to continue on error you are actually ignoring the errors. So I don't see any fail to comply with the specs, no bug. Also note that KiBot doesn't return "-1" it returns specific codes indicating what failed. And note that we could be ignoring multiple errors, so which one will be reported? I understand you want to stop, but this is something that can be added, this is why I label it as a new feature.

Ahhah! I finally understand our miss-communication on this topic!!

You are absolutely right, all these things are not problems or bugs with KiBot!

However, this is not how traditional unix tools, or CI pipelines tend to work.

Lets take for example the following pipeline script:

ls /tmp/testfile
cp /tmp/testfile /tmp/testfile2

(Please ignore the fact that I use ls to check for the existance of a file, you are supposed to use test or stat even for that :p )

Now lets assume that /tmp/testfile does not exist. Doing this from the console, means ls fails with no such file or directory, and you would argue 'there is nothing wrong with the tool' so it should return 0.

So if set -x is done, which pipeline executors do, both commands would return a failure if they cannot list or copy the files, even though this is not a bug in the program.

Of course, parsing the output is certainly possible.

So in the end, especially when wanting to keep existing behavior, is introduce a flag, where during exit you'd have something:

if flag && error != 0; return error;
return status;

Edit: you are so fast! thank you @set-soft !!

[oliv3r]

BTW, we've added quite a few flags to influence this behavior over the past year. I wonder if it makes sense to revisit them and see what may be more logical. I'm just thinking out loud here, and can imagine that in hind-sight, '--treat-warnings-as-errors' and '--continue-on-errors' and '--return-error-as-failure' in the end is the summary what we've been trying to accomplish maybe?

[set-soft]

BTW, we've added quite a few flags to influence this behavior over the past year. I wonder if it makes sense to revisit them and see what may be more logical.

To do what after this?

[oliv3r]

Don't know, just thinking out loud :)

[oliv3r]

So I only now got around to actually implement this, but I think I caught a bug @set-soft

I did

Usage:
...

e.g. I have a warning here, which I want to be treated as an error (from a return value point of view) and I want the run to continue (to get all warnings/errors so I don't have to run things 20 times, one for each warning/error; e.g. solve one, get (just one) new one).

Removing 'stop-on-warnings' (which logically conflicts verbally with dont-stop) removes the 'usage' output, but then results in a 'happy end'.

kibot --plot-config '.kibot/kici_pdf.kibot.yaml' --defs-from-env --dont-stop --fail-on-ignored  --schematic world.kicad_sch
- 'Exports the schematic to the most common exchange format. Suitable for printing.' (pdf_sch_print) [pdf_sch_print]
WARNING:(W165) `repeat_for_layer` specified, but nothing to repeat (kibot - out_pcb_print.py:372)
- 'Exports the PCB to the most common exchange format. Suitable for printing.' (pdf_pcb_print) [pcb_print]
Found 1 unique warning/s (2 total, 1 filtered)
I have no name!@491bbd474e00:/workdir/world$ echo $?
0

so a) there's some conflict of options and i'm not told what, or b) usage shouldn't have printed here ...

[set-soft]

Please post a small test case to reproduce the problem, the smallest KiBot config possible.
Explain, what do you expect and what do you get.
Don't use the whole KiCi as example.

[oliv3r]

Please post a small test case to reproduce the problem, the smallest KiBot config possible. Explain, what do you expect and what do you get. Don't use the whole KiCi as example.

I think it's as simple as doing

--dont-stop --fail-on-ignored  --stop-on-warnings

with anything. I just get 'usage' without telling me why (that the combination of arguments is wrong.

so the example is not even relevant as this fails on any kibot invocation.

In the end what I want is simple.

Given a file (project, schematic, pcb) that contains both (multiple) errors and warnings I want KiBot to:

• Continue analyzing the files so we get an overview of all issues (e.g. not stop on the first found; --dont-stop)
• Threat Warnings as Errors (from a return code point of view; --fail-on-ignored) (e.g. also warnings should produce a non-zero exit code)
• Return a non-zero exit code indicating 'something' was found (warning/error; --stop-on-warnings)