ConfigParser code should be separated

[Julian-O]

Versions

• Buildozer: 1.5.1

Description

[I am preparing a PR for this, but it is blocked by other higher-priority ones, so I am documenting the plan as a placeholder.]

The Buildozer class has a config object that is used by itself and the targets. It is a variant of the Python-standard ConfigParser, that supports case-sensitivity, lists and env variable overrides.

It has a number of minor issues:

• The responsibility of parsing the config doesn't need to be in the class Buildozer. (See Buildozer class too complex; should be split. #1629)
• New methods are added (and overridden) via monkey-patching on the instance, where a simple subclass would suffice.
• There are no unit tests (probably because above decisions make them difficult).
• Inconsistent method naming.
• Config specs are made case-sensitive, which might be necessary but no justification is given. (Not that Env Variable overrides are case-insensitive.)
• Reimplements existing functionality (ConfigParser already supports defaults as "fallbacks")
• Inconsistent use of defaults (sometimes they are parsed, sometimes they are returned as is).
• Environment variable overrides are applied at construction time and then reapplied on each access.
• List-handling is re-implemented several times (with different parameters defined).
• The term "raw" is used in a method differently to how ConfigParser uses it (untrimmed values versus uninterpolated values).

Proposal

In a separate file (specparser.py), define SpecParser, a subclass of ConfigParser.

Implement a single getlist method with parameters to support defaults (returned as is), fetching "values", stripping, section separators and split characters.

Add convenience functions that map existing calls to getlist/get/getboolean with appropriate parameters. Deprecate them, because they are unnecessary.

Apply the env var overrides automatically, after the text is read and parsed, before it is accessed. (Note: This means changes to the environment variables while Buildozer is running won't take effect on the config - but I haven't found any examples of that. One could just set the config option directly in these cases.)

Remove the term "raw".

Retain case-sensitivity until it can be shown to be unnecessary.

Write unit-tests.

Update Buildozer class to instantiate the new class rather than ConfigParser. Remove all the monkey patching and the now unnecessary call to set env var overrides.

Update android.py to use getlist with appropriate params rather than the "raw" function.

[RobertFlatt]

FYI, in addition here are a couple of buildozer.spec parser issues that result in non-deterministic behavior, and trip up users:

• There are Python style trailing comments in the buildozer.spec file, or comment characters inside a list. Comment characters must be the first character on a line in the buildozer.spec file.

• There is a white space or a tab before an uncommented option, an option must start on the first character of a line (this only applies to the first line of multi-line options).

[Julian-O]

Thanks, @RobertFlatt. Knowing about existing minefields helps me not repeat mistakes.

There are Python style trailing comments in the buildozer.spec file,

I agree that could be confusing. It is a general rule in ConfigParser's "ini" files. I don't propose to change it.

Comment characters must be the first character on a line in the buildozer.spec file.

That's not always true. Is there some special case where it is true?

There is a white space or a tab before an uncommented option, an option must start on the first character of a line (this only applies to the first line of multi-line options).

Ahhh. I see. If someone starts a second option indented deeper than the first option, it will be treated as a multiline option. I agree that could be confusing. Again, this is part of the standard ConfigParser formatting, and I don't propose to change it.

I added a test case to my prototype code in an effort to make sure I have understood your concerns, and show how I expect the result to work. I hope it is clarifying:

def test_controversial_cases(self):
    """Some aspects of the config syntax seem to cause confusion.
    This shows what the code is *specified* to do, which might not be
    expected.
    """
    sp = SpecParser()
    sp.read_string(
        """
            [section]
                # Comments can be indented.
            option1=a  # This is not considered a comment
            option2=this is
               a multiline string (not a list!)
               # This is considered a comment.
               this_is_not_an_option=it is still part of the multiline
            option3=this, is, one, way, of, representing, lists 
            
            [section:option4]
            this_is
            another_way
            # This is a comment.
            of # This is not a comment.
            representing=4
            lists
        """
    )

    assert (
        sp.get("section", "option1")
        == "a  # This is not considered a comment"
    )
    assert (
        sp.get("section", "option2")
        == "this is\na multiline string (not a list!)\n"
           "this_is_not_an_option=it is still part of the multiline"
    )
    assert sp.getlist("section", "option3") == [
        "this",
        "is",
        "one",
        "way",
        "of",
        "representing",
        "lists",
    ]
    assert sp.getlist("section", "option4") == [
        "this_is",
        "another_way",
        "of # This is not a comment.",
        "representing",
        "lists",
    ]

I haven't shown here the special case, only used in Android's log filtering option, in which the lists are not trimmed of whitespace. [No idea why.]

Given you have identified two confusing cases which are general .ini file issues, not Buildozer-specific, I propose we modify the default.spec to warn people about them. (I'd suggest some short comment at the top saying there are some hints at the bottom, and a longer comment at the bottom warning users about these pitfalls.)

[RobertFlatt]

Thanks @Julian-O
Adding a statement at the top that the syntax is .ini syntax is a really good idea.
Expecting users to read documentation about .ini syntax in order to use Buildozer for the first time, not so much.

The original choice of a config parser for a user's first encounter with Buildozer was a poor one, nothing practical to be done about that. Config parser does indeed not have good error checking, but in any viable software a human's input must be error checked. Specially in the case where parse errors do not necessarily mean build errors, and usually mean strange run time errors.

I understand you view additional functionality as mission creep, but the technical term for tidying stuff up without fixing the issues is I believe turd polishing.

[Julian-O]

Just to the last point, this is getting off-topic (we can take it to Discord if you like), but here are my motivations:

I want to make some significant changes to the Buildozer class, but it currently is way too large and has too many responsibilities. It is hard to understand, hard to unit test and thus hard to safely change.

I just had a PR merged which takes the responsibility for logging out of the Buildozer class and puts it in a separate file. I wish the logging was done using the standard Python logging module rather than from scratch. I spent a lot of effort on Kivy a year ago making it work better with the standard Python logging module, but I had a stronger motivation because it was wreaking havoc with my libraries that were being included. I don't plan a similar effort on Buildozer, but if someone wants to do it, it is all in one place now.

I plan to do another PR for this issue which takes the responsibility for parsing out of the Buildozer class and puts it in a separate file. If someone wants to tackle the issues you describe, they will be easier and self-contained.

I plan to do another PR which takes the responsibility for the low-level operations (file renames, directory deletions, running commands, etc.) out of the Buildozer class and put them in a separate file. This is where I want to spend my effort, because then I can make them platform independent. That will bring us one small step closer to making Buildozer be able to build Android binaries on native Windows. (Yes, much more effort will be required to enable pythonforandroid to work, but baby steps...)

So, yes, I am tidying stuff up without fixing the issues, but that is because (a) my eyes are on a different prize, and I need it out of my way, and (b) I think my efforts to focus on Windows (with a cost in reviewer time and risk of bugs) will be more palatable to the non-Windows developers if I tidy up the common code as I go.

I genuinely think that, if someone does want to replace the parser, then my work to get rid of the existing monkeypatching, move the code to a separate file, remove the duplicate implementations and provide some unit-testing for reassurance that the refactoring doesn't break things is a useful first step.

[misl6]

Fixed via #1639