search

ilbers / isar

Integration System for Automated Root filesystem generation
Other
177 stars 72 forks source link

isar-bootstrap.inc::get_distro_primary_source_entry() should not return ["", "", ""] by default #72

Closed HenningPuttnies closed 2 years ago

HenningPuttnies commented 2 years ago

From my point of view, this function: https://github.com/ilbers/isar/blob/master/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc#L165

should not return ["", "", ""] by default. Instead, is should throw an error.

The rational is as follows: returning ["", "", ""] leads to ambiguous errors, if, e.g.,:

ismagulb commented 2 years ago

Hello Henning,

we'd like to understand the context, can you provide the steps to reproduce?

If you also have a suggestion in form of a patch, feel free to share.

henning-schild commented 2 years ago

I guess the way to repro would be to use an "invalid" i.e. empty sources.list

If that happens we indeed could probably detect that early instead of returing something that will just break in a weird way further down the line.

Maybe return None, None, None or simply raise/bbfatal

HenningPuttnies commented 2 years ago

@henning-schild I would highly appreciate that. In my case, the list was not empty. However, I made a typo in the distro name. Thus, nothing was read from the list.

It took me a long time to figure out the root cause. I think throwing an error in this function at isar layer would massively ease debugging.

henning-schild commented 2 years ago

In this case i am afraid the example file would be indeed needed. Because you maybe did not even run into the ["", "", ""] but instead the parser returned your invalid distro. Which i think we can not do anything about, because the parser should not know all valid values ... would be too hard to maintain.

amikan commented 2 years ago

Just have proposed the patch at [PATCH] isar-bootstrap: Fail on invalid apt source list. The only inconvenience is that the error during the parsing will be followed by do_bootstrap contents which can be confusing.

HenningPuttnies commented 2 years ago

@amikan Thanks. I will try it and let you know.

HenningPuttnies commented 2 years ago

@amikan It works perfectly. Thanks.

I had to slightly rework you patch as we actually work with this version: https://github.com/ilbers/isar/commit/76eac233f1b1d0d117b3f05b3dec8a6da1cdf4c7