On Thu, Sep 30, 2021 at 8:37 AM Nico Huber nico.h@gmx.de wrote:
Hi Jack,
On 30.09.21 15:22, Jack Rosenthal via coreboot wrote:
On Thu, Sep 30, 2021 at 2:27 AM Arthur Heymans arthur@aheymans.xyz
wrote:
As a rule of thumb, any project involving a substantial amount of Python
always ends up needing a Docker container to build. So I'm in the "no"
camp
for making Python a dependency, however I think it's fine to keep
things
as-is where it can be used for helper scripts and utilities for
specific
purposes such that they aren't critical to building the tree.
I'm on the same side here. Building the documentation with python sphinx is a pain and I ended up needing docker. The same can be said about edk2/tianocore which also uses a lot of
python
in critical parts of its build system.
For just Kconfiglib, the requirements are Python 2.7 or Python 3.2+, including no additional library installation (standard libraries only). Using docker in this scenario would be completely ridiculous.
please keep in mind that this thread tries to discuss a general python requirement. Not Kconfiglib in particular. I suppose nobody doubts that Kconfiglib is one of the better/easier to integrate python projects.
IMHO, we should discuss every added dependency separately. E.g. not just all of python at once but every piece of code that we can't fix in our own repository. I guess, in the case of Kconfiglib that would be:
Agreed. Some sort of specification of what the dependency is we are adding is absolutely required.
As an example, we could say "Python 3.6 or newer, no additional libraries required". (this is just an example, the actual version numbers etc we settle on should be discussed).
I bet most distributions are shipping with 3.6+ these days too...
- python (the interpreter)
- std libs (I guess part of the above)
- Kconfiglib itself unless we pull it into our repo like we did with Kconfig
I believe the previous discussion around Kconfiglib was to pull it into the coreboot repo directly so we don't need to make it a dependency for people. We don't need to clone the entire repository into our repos, everything is pretty much self-contained to kconfiglib.py, as well as any UIs we want to enable (e.g., menuconfig.py and guiconfig.py).
Looking at the current Kconfiglib implementation we would be replacing
the
C code with 21873 lines of Python code that is now taking the code to deviate from what the Linux kernel is doing. I am having a hard time
seeing
a "net benefit" in this scenario. Given the mess that Python 2 to
Python 3
conversion has been (and still is), this is just inviting a lot of
trouble
into what has been a fairly stable part of coreboot for the last decade.
21,873 lines of code is including the tests. Wouldn't it be nice if the C Kconfig implementation even had tests? :P
Not sure, I never looked close enough at the code to tell if it's bad enough to justify the overhead of tests.
IMO, any codebase is significantly easier and safer to maintain if there are tests.
In my experience that is only true for projects that don't focus enough on code reviews and refactoring. I often see the necessity of tests as lack of review. In some projects tests are even added to provide false confidence that one would need less review. IMO, the opposite is the case. Tests encourage less experienced developers to push patches with- out looking much into the code, because all tests are green, right? In such a scenario the effort to integrate a patch shifts from authors to reviewers, IMO not good.
Nico
Tests and code review serve two separate functions: tests allowing automatic validation that a change won't break everything, and code review to keep the code-quality high of the code that lands. Obviously, as you point out, there is some overlap, and tests cannot ever be 100% perfect. But enough with the philosophy.
With respect to Kconfig, we (at Google) encountered a lovely build flake after the Kconfig uprev last month in the coreboot tree that took a couple of weeks to sort out and resolve. Some sort of automated validation that the code is working could have possibly helped. Of course, the C implementation of Kconfig has no tests at all. Some tests is better than nothing.