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:
* 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
If we decided now that we'd allow the above that shouldn't mean that we can add any python related dependency in the future without further discussion.
For any piece of code that we can't fix in our own repository, we should have people that know the respective upstreaming process and can react rather quickly to any issue, IMO.
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