search

Mbed-TLS / mbedtls-framework

TF-PSA-Crypto and Mbed TLS version-independent build and test framework
Other
2 stars 16 forks source link

Move `config.py` functionalities to the framework #41

Closed gabor-mezei-arm closed 2 months ago

gabor-mezei-arm commented 3 months ago

Move the part of the config.py which is common in the TF-PSA_Crypto and the MbedTLS repo to the framework repo.

Related to Mbed-TLS/mbedtls#9470

gabor-mezei-arm commented 2 months ago

During the fixes I found that the implementation and documentation of get function is differ. This function was not used before and was never changed after the initial commit. Should I change the documentation or the implementation?

https://github.com/Mbed-TLS/mbedtls-framework/blob/025c8e7419164d6d2819141e034628b8f6d3eb13/scripts/mbedtls_framework/config_common.py#L85-L96

I think it is meant to be:

        if name in self:
            return self.settings[name].value
gilles-peskine-arm commented 2 months ago

I found that the implementation and documentation of get function is differ.

I don't see the problem, can you clarify?

This function was not used before and was never changed after the initial commit.

When I wrote config.py, I expected that we'd use get often in Python scripts that depend on the configuration. But it turns out that we haven't written many scripts that depend on the configuration until now: we still mostly use config.py only through the command line.

I think it is meant to be:

    if name in self:
        return self.settings[name].value

I don't see the difference: the __contains__ method of self defines … in self to be equivalent to … in self.settings.

gabor-mezei-arm commented 2 months ago

I don't see the problem, can you clarify?

The problem here is the get function returns the value of an inactive setting (disabled macro) and the documentation says in this case the default parameter should be returned.

I don't see the difference: the __contains__ method of self defines … in self to be equivalent to … in self.settings.

The difference is the ... in self/__contains__ returns true only when a setting is active (the macro is enabled) and the ... in self.settings returns true if a setting is present (active or inactive). So for the inactive settings the __contains__ returns false which is what the documentation wants.

gilles-peskine-arm commented 2 months ago

The difference is the ... in self/contains returns true only when a setting is active (the macro is enabled) and the ... in self.settings returns true if a setting is present (active or inactive).

Oh, right. Yes, I think the intent of get was to be the way it's documented, not the way it behaves.

It doesn't matter much since we aren't using that aspect, but please do fix it. It's always better when the code matches the documentation. Thanks for spotting that!

(This script should really have unit tests, but that's just one item in the huge tech debt pile.)

davidhorstmann-arm commented 2 months ago

I've left 2 comments, LGTM otherwise. This review is intertwined with that of the other PR (Mbed-TLS/mbedtls#9470) as well, so once this one is approved the other one should also be fine.

ronald-cron-arm commented 2 months ago

The CI on #9470 and #9511 is green. I've run test_config_script.py on the base and head of the two PRs and it does not detect any regression. This validates this PR thus merging.