Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use config.py as a module in depends.py #6493

Merged
merged 5 commits into from
Mar 2, 2023
Merged

Use config.py as a module in depends.py #6493

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3f93012
Use config.py as a module in depends.py
Oct 26, 2022
3b0215d
depends.py: merge set/unset config option into one function
Jan 23, 2023
576803f
depends.py: improve expected argument type
Jan 24, 2023
81cf5ad
Improve tests/scripts/depends.py code
Feb 6, 2023
72082dc
Improve tests/scripts/depends.py code
Feb 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 16 additions & 22 deletions tests/scripts/depends.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@
import subprocess
import sys
import traceback

# Add the Mbed TLS Python library directory to the module search path
import scripts_path # pylint: disable=unused-import

This comment was marked as resolved.

Sign in to view

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done :)

import config

Expand Down Expand Up @@ -140,22 +140,22 @@ def option_exists(conf, option):
return False
return True

def set_config_option(conf, option, colors, value=None):
"""Set configuration option, optionally specifying a value"""
def set_config_option_value(conf, option, colors, value):
"""Set/unset a configuration option, optionally specifying a value"""
if not option_exists(conf, option):
log_line('Symbol {} was not found in {}'.format(option, conf.filename), color=colors.red)
return False
log_command(['config.py', 'set', option])
conf.set(option, value)
return True

def unset_config_option(conf, option, colors):
"""Unset configuration option if it exists"""
if not option_exists(conf, option):
log_line('Symbol {} was not found in {}'.format(option, conf.filename), color=colors.red)
return False
log_command(['config.py', 'unset', option])
conf.unset(option)
if value is False:
log_command(['config.py', 'unset', option])
conf.unset(option)
else:
if value is True:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that value can be of Boolean or String type?
Not sure if this is good solution. Safer would be to keep value always as String and assume possible values like: Enable, Disable, <Other Value>.

Then we would have:
set_config_option_value(conf, 'MBEDTLS_TEST_HOOKS', colors, 'Disable')

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to distinguish #define FOO from #define FOO Enable, so no, the current code is right in having a string for #define FOO ... vs True for #define FOO and False for no definition at all.

It would help to convey this through type annotations. value : Union[bool, str] and documentation explaining what I wrote above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added as a new commit.

log_command(['config.py', 'set', option])
conf.set(option)
else:
log_command(['config.py', 'set', option, value])
conf.set(option, value)
AndrzejKurek marked this conversation as resolved.
Show resolved Hide resolved
return True

def set_reference_config(conf, options, colors):
Expand All @@ -165,9 +165,9 @@ def set_reference_config(conf, options, colors):
# Turn off options that are not relevant to the tests and slow them down.
log_command(['config.py', 'full'])
conf.adapt(config.full_adapter)
unset_config_option(conf, 'MBEDTLS_TEST_HOOKS', colors)
set_config_option_value(conf, 'MBEDTLS_TEST_HOOKS', colors, False)
if options.unset_use_psa:
unset_config_option(conf, 'MBEDTLS_USE_PSA_CRYPTO', colors)
set_config_option_value(conf, 'MBEDTLS_USE_PSA_CRYPTO', colors, False)

class Job:
"""A job builds the library in a specific configuration and runs some tests."""
Expand Down Expand Up @@ -201,13 +201,7 @@ def configure(self, conf, options, colors):
'''Set library configuration options as required for the job.'''
set_reference_config(conf, options, colors)
for key, value in sorted(self.config_settings.items()):
ret = False
if value is True:
ret = set_config_option(conf, key, colors)
elif value is False:
ret = unset_config_option(conf, key, colors)
else:
ret = set_config_option(conf, key, colors, value)
ret = set_config_option_value(conf, key, colors, value)
if ret is False:
return False
return True
Expand Down