-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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.
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
Use config.py
as a module in depends.py
#6493
Conversation
319c227
to
be268d5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR looks good.
Spotted only 2 places for potential improvements.
Tested script locally. Executed few configs. Works fine and colors are really cool.
It seems that CI is unhappy, but the results are stale.
@@ -72,6 +72,9 @@ | |||
import sys | |||
import traceback | |||
|
|||
import scripts_path # pylint: disable=unused-import |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :)
tests/scripts/depends.py
Outdated
conf.set(option, value) | ||
return True | ||
|
||
def unset_config_option(conf, option, colors): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Body of set_config_option
and unset_config_option
is very similar. I suggest to optimize the code and handle set/unset in auxiliary function and then only call auxiliary function with corresponding parameters from set_config_option/unset_config_option
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :)
Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>
Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>
tests/scripts/depends.py
Outdated
log_command(['config.py', 'unset', option]) | ||
conf.unset(option) | ||
else: | ||
if value is True: |
There was a problem hiding this comment.
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')
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Requested config option can be either boolean or a string. Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing my comments.
LGTM!
Only one thing is strange now that only one parameter has specified type. But it is well described.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is a bit roundabout in places, but that's not a big deal and it does what it's supposed to do.
return False | ||
return True | ||
|
||
def set_config_option_value(conf, option, colors, value: Union[bool, str]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this function (without the logging) be in config.py
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be an improvement if other scripts provide similar code.
As suggested by gilles-peskine-arm. Co-authored-by: Gilles Peskine <gilles.peskine@arm.com> Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>
81cf5ad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the improvements!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
CI needs to be restarted.
As suggested by gilles-peskine-arm. Co-authored-by: Gilles Peskine <gilles.peskine@arm.com> Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>
72082dc
Force-pushed the last commit without changes to re-run the CI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
depends.py
now usesconfig.py
to get the available configuration options as well as manipulate the config file.Fixes #6440.
Backport here