Replace config.pl by config.py #2765

gilles-peskine-arm · 2019-07-27T22:18:08Z

Replace config.pl by config.py. I went for feature parity: config.py should be a drop-in replacement for config.pl, and has no new command line feature. The two scripts may behave differently in edge cases, but they support the same commands (get, set, unset, full, realfull, baremetal) and give identical results for each configuration.

To verify that config.py is compatible with the former config.pl:

git show development:scripts/config.pl >|config.pl 
chmod +x config.pl
./tests/scripts/test_config_script.py -d old -s ./config.pl 
./tests/scripts/test_config_script.py -d new
diff -ruw old new

This should only report a differing nonzero error code in some error cases.

Reasons to switch to Python:

config.py can be used as a library in other Python scripts to read and modify the configuration simply and efficiently.
config.pl had relatively convoluted logic and was in need a major dust-up anyway.
More of the team knows Python than Perl these days, and we're generally moving away from Perl and using Python instead for new scripts.

I think this should be backported to ease maintenance, but it does add a new build-time requirement. Python is a requirement for testing in all maintained branches, but not for building. — For the time being, we've decided to keep config.pl in the LTS branches. We might backport it later when we start adding new features to config.py and we want to backport test scripts that use those new features.

k-stachowiak

I raised some minor questions regarding the code, which I consider more of a design discussion than review issues.

Unfortunately I found some minor errors as well:

Incorrect formatting of the "templates",
The --force option not working properly.

OTOH, I compared the config.pl and config.py outputs for the config presets and found them to be identical, so I can confirm the main functionality being fine.

scripts/config.py

gilles-peskine-arm · 2019-08-01T21:47:33Z

I've ported and improved test-config-script.pl, now tests/scripts/test_config_script.py. I don't know if it'll get a lot of use, but at least it was useful to test during development and I can now be confident that I've handled all the tested cases correctly.

There's a CI failure on Windows which I haven't investigated yet.

RonEld · 2019-08-26T10:11:43Z

CMakeLists.txt

                         "${WARNING_BORDER}")

 find_package(PythonInterp)
 find_package(Perl)


I assume this needs to be

Suggested change

find_package(Perl)

find_package(Python)

Well-spotted. Perl is no longer needed. But PythonInterp is already present. There's also a FindPython but only on recent CMake versions. And PYTHON_FOUND below should actually be PYTHONINTERP_FOUND.

And on my machine FindPython actually finds python which is Python 2 but we need Python 3!

gilles-peskine-arm · 2019-09-04T20:55:58Z

I fixed some issues, but this now needs rebasing 😭

gilles-peskine-arm · 2019-09-05T18:26:24Z

The CI failure is one Windows job which calls perl config.pl, which relays to python3 config.py, which fails because python3 is not on %PATH.

gilles-peskine-arm · 2019-09-06T18:39:02Z

I rebased on top of development due to conflicts in config.pl and all.sh. The changes during the rebase were to cope with one symbol added to config.pl (added to config.py at the relevant points in its history) and with many places where config.pl was added or removed in all.sh (handled in the commit that replaces config.pl with config.py).

The previous history is in https://github.com/gilles-peskine-arm/mbedtls/tree/config_py-2. It started at ff645d9. The new history starts at 4197f0e,

$ git diff ff645d9838f4797c726562d90c0a9ae5207ea86f..49fcbeab14dba5ca8d9160eed520da775b67ca38 -- scripts/config.pl
diff --git a/scripts/config.pl b/scripts/config.pl
index 5b13fc9..3942584 100755
--- a/scripts/config.pl
+++ b/scripts/config.pl
@@ -104,6 +104,7 @@ MBEDTLS_NO_64BIT_MULTIPLICATION
 MBEDTLS_PSA_CRYPTO_SPM
 MBEDTLS_PSA_INJECT_ENTROPY
 MBEDTLS_ECP_RESTARTABLE
+MBEDTLS_ECDH_VARIANT_EVEREST_ENABLED
 _ALT\s*$
 );
 
$ git diff config_py-2..config_py -- scripts/config.py
diff --git a/scripts/config.py b/scripts/config.py
index b70d8be..02172de 100755
--- a/scripts/config.py
+++ b/scripts/config.py
@@ -165,6 +165,7 @@ def include_in_full(name):
         return True
     if name in [
             'MBEDTLS_DEPRECATED_REMOVED',
+            'MBEDTLS_ECDH_VARIANT_EVEREST_ENABLED',
             'MBEDTLS_ECP_RESTARTABLE',
             'MBEDTLS_HAVE_SSE2',
             'MBEDTLS_NO_64BIT_MULTIPLICATION',

To verify that config.py is compatible with the former config.pl:

git checkout config_py
git show 4197f0e28e15c42e907f873eea292fac31bfa7e6:scripts/config.pl >|config.pl 
chmod +x config.pl
/tests/scripts/test_config_script.py -d old -s ./config.pl 
/tests/scripts/test_config_script.py -d new
diff -ruw old new

The only differences are the return status on error (before it was a happenstance value of$! due to the way die was used in the perl script, now it's 1).

gilles-peskine-arm · 2019-09-09T08:39:14Z

This will need another rebase once #2469 is merged :(

This is meant to be a drop-in replacement for config.pl which can additionally be used as a library in a Python script. So far this script supports the commands 'get', 'set' and 'realfull' but not the other built-in configurations.

Also fix 'realfull' to only affect the appropriate sections. Tested to produce the same results as config.pl on the default configuration. This commit deliberately contains a direct copy the lists of symbol names from config.pl.

These options haven't existed for a long time.

They're easier to maintain that way. The old lists were partly alphabetized, partly based on config.h order, and partly in the order in which symbols had been added to config.pl.

git grep -Fl /config.pl | xargs sed -i -e 's!/config\.pl!/config.py!g' Also: * Change one comment in include/mbedtls/check_config.h. * Change PERL to PYTHON in CMakeLists.txt.

Keep config.pl in Perl in case people are running "perl config.pl".

Also make that error message end with a newline.

Run config.py with various options and store the results in files. This script also supports the now-removed config.pl. This is a framework to run non-regression tests on config.py: run it with the old version, run it with the new version, and compare the output. This is deliberately not a functional test suite so that we don't need to maintain a set of known outputs. When something changes in config.py (or config.h), run the script before, run it after, and check manually whether any differences in the output are acceptable.

Perl is no longer needed. Python must be version 3. Version 2 is not suitable. The variable is PYTHONINTERP_FOUND, not PYTHON_FOUND.

The test suite generator has been a Python script for a long time, but tests/CMakeLists.txt still looked for Perl. The reference to PYTHON_INTERP only worked due to a call to find_package(PythonInterp) in the toplevel CMakeLists.txt, and cmake would not have printed the expected error message if python was not available.

Normally a valueless symbol remains valueless and a symbol with a value keeps having one. But just in case a symbol does get changed from valueless to having a value, make sure there's a space between the symbol and the value. And if a symbol gets changed from having a value to valueless, strip trailing whitespace. Add corresponding tests. Also fix the case of a valueless symbol added with the set method, which would have resulted in attempting to use None as a string. This only happened with the Python API, not with the command line API.

gilles-peskine-arm · 2019-09-13T09:06:25Z

Rebased again. Previous version in https://github.com/gilles-peskine-arm/mbedtls/tree/config_py-3.

Patater

Looks pretty close to good to me.

scripts/config.pl

include/mbedtls/check_config.h

scripts/config.py

tests/scripts/test_config_script.py

Patater · 2019-09-13T15:58:11Z

tests/scripts/test_config_script.py

        run_one(options, ['--force', 'set', symbol])
        run_one(options, ['unset', symbol])
    for symbol in TEST_SYMBOLS_WITH_VALUE:
+        run_one(options, ['get', symbol])


This exercises get, but doesn't check you get a sane value back from it.

This script doesn't check anything for sanity. It's purely a non-regression test: record the outcome, and then you can compare it with the previous version. See the comment at the top of the script and the PR description.

How do you know that get works as well as it worked with config.pl?

I run

./tests/scripts/test_config_script.py -d old -s ./config.pl ./tests/scripts/test_config_script.py -d new diff -ruw old new

and it doesn't find any difference.

When I run ./tests/scripts/test_config_script.py -d old -s ./config.pl, I get an error about ./config.pl not found. When I run ./tests/scripts/test_config_script.py -d old -s ./scripts/config.pl, I see no output at all.

Ran again with newest config.pl (I had been using an older one).

diff -ruw old/config-set-CUSTOM_SYMBOL-value.status new/config-set-CUSTOM_SYMBOL-value.status --- old/config-set-CUSTOM_SYMBOL-value.status 2019-09-19 13:52:16.000000000 +0100 +++ new/config-set-CUSTOM_SYMBOL-value.status 2019-09-19 13:52:28.000000000 +0100 @@ -1 +1 @@ -25 +1 diff -ruw old/config-set-CUSTOM_SYMBOL.status new/config-set-CUSTOM_SYMBOL.status --- old/config-set-CUSTOM_SYMBOL.status 2019-09-19 13:52:16.000000000 +0100 +++ new/config-set-CUSTOM_SYMBOL.status 2019-09-19 13:52:28.000000000 +0100 @@ -1 +1 @@ -25 +1

I still see this diff, even on Linux.

$ ./scripts/config.pl set CUSTOM_SYMBOL value A #define for the symbol CUSTOM_SYMBOL was not found in include/mbedtls/config.h $ echo $status 1 $ ./config.pl set CUSTOM_SYMBOL value A #define for the symbol CUSTOM_SYMBOL was not found in include/mbedtls/config.h $ echo $status 25

Error code returned is different. Not sure we care, but is this different from what you see? You said you see no difference in behavior.

Yes, that's expected. See the description:

This should only report a differing nonzero error code in some error cases.

The old code was returning 25 on some errors because die returns errno and the value of errno just happens to be ENOTTY at that point for whatever reason. I mostly kept the new script bug-compatible but not that bug-compatible.

OK. I had trouble reconciling what I saw with your comment in "#2765 (comment)" and wanted to confirm.

We currently test setting a symbol with a value even if it didn't originally had one and vice versa. So there's no need to have separate lists of symbols to test with. Just test everything we want to test with each symbol.

The tests were not covering get for a symbol with a value. No symbol has an uncommented value in the default config.h. (Actually there's _CRT_SECURE_NO_DEPRECATE, but that's a bit of a hack that this script is not expected to handle, so don't use it). Add tests of "get FOO" after "set FOO" and "set FOO value", so that we have coverage for "get FOO" when "FOO" has a value.

k-stachowiak

Thanks for addressing my comments. Looks good to me.

scripts/config.pl

Patater

One question about error code returned, but otherwise OK.

Patater

LGTM

gilles-peskine-arm added enhancement mbed TLS team needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. component-platform Portability layer and build scripts labels Jul 27, 2019

gilles-peskine-arm force-pushed the config_py branch 2 times, most recently from a3a4467 to 9620857 Compare July 29, 2019 21:45

k-stachowiak suggested changes Aug 1, 2019

View reviewed changes

gilles-peskine-arm force-pushed the config_py branch from bdcfc94 to 216fe59 Compare August 1, 2019 21:50

RonEld reviewed Aug 26, 2019

View reviewed changes

gilles-peskine-arm force-pushed the config_py branch from c60850a to 812fd0f Compare September 6, 2019 18:35

gilles-peskine-arm force-pushed the config_py branch from 812fd0f to 9a7368e Compare September 6, 2019 18:40

gilles-peskine-arm requested review from RonEld and k-stachowiak September 6, 2019 18:46

gilles-peskine-arm removed the needs-review Every commit must be reviewed by at least two team members, label Sep 9, 2019

gilles-peskine-arm removed request for RonEld and k-stachowiak September 9, 2019 08:39

gilles-peskine-arm added 8 commits September 13, 2019 10:49

Implement the 'full' and 'baremetal' configurations

53d41ae

Also fix 'realfull' to only affect the appropriate sections. Tested to produce the same results as config.pl on the default configuration. This commit deliberately contains a direct copy the lists of symbol names from config.pl.

Remove obsolete options from config.py

6c2d078

These options haven't existed for a long time.

Sort symbol lists in alphabetical order

2d89ccc

They're easier to maintain that way. The old lists were partly alphabetized, partly based on config.h order, and partly in the order in which symbols had been added to config.pl.

Uniformize whitespace in commented-out defines

5639aef

Support writing to a different file

40f103c

Invoke config.py instead of config.pl

5d46f6a

git grep -Fl /config.pl | xargs sed -i -e 's!/config\.pl!/config.py!g' Also: * Change one comment in include/mbedtls/check_config.h. * Change PERL to PYTHON in CMakeLists.txt.

Replace config.pl by a redirection to config.py

bf359c7

Keep config.pl in Perl in case people are running "perl config.pl".

gilles-peskine-arm added 9 commits September 13, 2019 11:04

Fix "--force set" without a value sneaking a None in

0c7fcd2

Fix "#define ... not found" error when using the default file name

221df1e

Also make that error message end with a newline.

Documentation improvements

c190c90

cmake: fix Python requirement

2fd7ffa

Perl is no longer needed. Python must be version 3. Version 2 is not suitable. The variable is PYTHONINTERP_FOUND, not PYTHON_FOUND.

Remove redundant test case

e3734bd

Compatibility redirect: if python3 is not available, try python

a103c18

gilles-peskine-arm force-pushed the config_py branch from 9a7368e to a103c18 Compare September 13, 2019 09:05

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-backports Backports are missing or are pending review and approval. labels Sep 13, 2019

gilles-peskine-arm requested review from Patater and k-stachowiak September 13, 2019 09:08

Patater suggested changes Sep 13, 2019

View reviewed changes

scripts/config.pl Show resolved Hide resolved

include/mbedtls/check_config.h Show resolved Hide resolved

scripts/config.py Show resolved Hide resolved

tests/scripts/test_config_script.py Show resolved Hide resolved

gilles-peskine-arm added 2 commits September 13, 2019 15:17

Compatibility redirect: add copyright notice

0409bcd

config.py testing: also test the get command

61695e7

gilles-peskine-arm requested a review from Patater September 13, 2019 13:18

Patater reviewed Sep 13, 2019

View reviewed changes

gilles-peskine-arm added 2 commits September 19, 2019 12:18

Consolidate tests for set with/without values

fd7ad33

We currently test setting a symbol with a value even if it didn't originally had one and vice versa. So there's no need to have separate lists of symbols to test with. Just test everything we want to test with each symbol.

gilles-peskine-arm requested a review from Patater September 19, 2019 10:22

k-stachowiak approved these changes Sep 19, 2019

View reviewed changes

scripts/config.pl Show resolved Hide resolved

Patater reviewed Sep 19, 2019

View reviewed changes

Patater approved these changes Sep 19, 2019

View reviewed changes

Patater added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Sep 19, 2019

Patater merged commit 16a25e0 into Mbed-TLS:development Sep 24, 2019

dgreen-arm mentioned this pull request Nov 13, 2019

Replace config.pl by config.py ARMmbed/mbed-crypto#321

Merged

Replace config.pl by config.py #2765

Replace config.pl by config.py #2765

Uh oh!

Conversation

gilles-peskine-arm commented Jul 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k-stachowiak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gilles-peskine-arm commented Aug 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm commented Sep 4, 2019

Uh oh!

gilles-peskine-arm commented Sep 5, 2019

Uh oh!

gilles-peskine-arm commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gilles-peskine-arm commented Sep 9, 2019

Uh oh!

gilles-peskine-arm commented Sep 13, 2019

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Patater Sep 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k-stachowiak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

gilles-peskine-arm commented Jul 27, 2019 •

edited

Loading

gilles-peskine-arm commented Sep 6, 2019 •

edited

Loading

Patater Sep 19, 2019 •

edited

Loading