Mechanism to error out on removed configuration options #164
No reviewers
Labels
No labels
approved
bug
dependencies
DO-NOT-MERGE
enhancement
good-first-issue
help-wanted
historical-reviewed
historical-reviewing
needs-adoption
needs-ci
needs-design-approval
needs-info
needs-preceding-pr
needs-review
needs-reviewer
needs-work
priority-high
priority-low
priority-medium
priority-scheduled
priority-very-high
single-reviewer
size-l
size-m
size-s
size-xs
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-Mbed-TLS/mbedtls-framework!164
Loading…
Reference in a new issue
No description provided.
Delete branch "config-error-on-removed-options-framework"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Set up a mechanism for erroring out if users try to use removed options in their configuration file. Resolves https://github.com/Mbed-TLS/mbedtls/issues/10147. Specification in https://github.com/Mbed-TLS/mbedtls-framework/pull/184.
In this PR:
config_checks_generator.py: classes for options of a subproject.Status: ready for review. If the consuming brnaches require extra work in the framework, I'll do that in new commits.
PR checklist
Looks pretty good to me, the only blockers are the two issues with option handling in the shell script.
I have generated the files locally using the instructions in the commit messages and verified with sha256sum that I'm getting identical outputs.
@ -0,0 +1,3 @@MBEDTLS_CONFIG_ADJUST_SSL_HNote (on the commit message): I have checked that the commits used are recent merge commits from the right project.
In the description, before/after must be configurable (currently the description for the generated
config_check_after.hsays "before").Also, typo: configuration File.
In general, I don't think "before" without saying before what is a great name. Suggesting
before_userhere.@ -106,6 +106,61 @@ class Internal(Checker):"""Checker for an internal-only option."""Queestion: why can subproject be provided either as an argument to
__init__()or as a class variable by child classes? I would think either would be enough (and perhaps the later a bit cleaner).Minor: IIUC this results in extra entries in the include path. That's probably harmless, but it would seem a bit cleaner to check in which repo we are and adjust the include path accordingly.
Should we really include both in succession?
mbedtls/build_info.his already including the tf-psa-crypto one, and and normal users are likely to only include one of them.@ -0,0 +1,85 @@#!/bin/shQuestion: do we really need this line considering we're setting it in both the
ifand theelsebranch below?Typo:
o:g:notd.Note: must exist.
@ -0,0 +39,4 @@esacdoneshift $((OPTIND - 1))We probably want
shift $((OPTIND-1))before the next paragraph, or something to that effect.@ -0,0 +49,4 @@version=$2# Assert that the git commit existsgit -C "$project_root" cat-file -e "${commit}^{commit}"For my own education: I wasn't familiar with this
^{commit}syntax and I'm not sure what man page I should read to find out more about it?@ -0,0 +63,4 @@## collect_macros VARIANT FILENAME...## Find the macros defined in the given files. Save the result in## $history_dir/config-VARIANT-*.txt.collect_macros () {Perhaps add a comment like "Usage: collect_macros [type] [files...]"?
All fixed in some previous history rewrite.
This used to be before/after [the user config] and changed to before/user/final. I agree that the names aren't ideal, but I don't think
before_user/user/finalis better. It should bebefore_user/after_user/final. Or it could be justbefore_user/after_user, sincefinalis currently unused. But then we could go back tobefore/aftersince there's only one thing to be before/after and it's a pretty natural one (we're doing config checks, so before/after refers to reading the config.)I'm not very happy about the “user” part, whether on its own or in
after_user, because of the ambiguity between “the whole configuration provided by the user” and “MBEDTLS_USER_CONFIG_FILE(or crypto equivalent) specifically”. Any ideas for a better name?Note that we'd better commit to names now, because once https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/269 is merged, changing the names will require back-and-forth between the framework and the consuming branches,
I tend to disagree. When you minimize include paths, usually, later, someone ends up puzzling over why this header file is not found in this specific test script. I prefer to err on the side of robustness.
No longer relevant since some past rework where this script tries to compile
${PROJECT}_config.crather than some crafted input.@ -106,6 +106,61 @@ class Internal(Checker):"""Checker for an internal-only option."""No good reason that I can remember. Maybe I started with the dynamic approach, then realized that class variables worked but didn't go all the way with removing the argument. The oldest commit with subproject handling is
bc55f06134which already has both.Arguably, the ability to specify a subproject per instance would be useful to avoid having to create a pair of child classes (
XxxOptionandXxxInternal) for every subproject. But we only have one subproject, and we're unlikely to add more, so I wouldn't consider this a good reason.I'll remove the argument.
@ -0,0 +49,4 @@version=$2# Assert that the git commit existsgit -C "$project_root" cat-file -e "${commit}^{commit}"gitrevisions, an instance of<rev>^{<type>}. It's the kind of things that I vaguely remember exist, but have to look up when I need it. In this case, I probably got it from some Stack Overflow post on how to check whether a commit exists.@ -0,0 +1,85 @@#!/bin/shCome to think of it,
-Cwould make more sense than-gor -d, since this is essentially “what you pass togit -C”.I've handled (or replied to) all your comments except this name change. A name change will require coordinated action over the three PR so I'd rather get a consensus first.
@ -109,0 +115,4 @@def __init__(self, name: str, suggestion: str = '') -> None:super().__init__(name, suggestion)if self.SUBPROJECT is None:raise ValueError('No subproject specified for ' + name)The
finalfile, introduced in a previous pull request, is currently unused. It was strongly needed in an earlier incarnation (back when the config checks were public headers, included every timebuild_info.hwas included, we needed to clean up macros defined by the previous files). Now there's no immediate need for it, but it might become useful in the future. For example, I think it would be needed if we wanted to reject user config files that try to#undefsomething (which is not a feature, currently, because I'm guessing that nobody does that).At this point, removing it is more work than leaving it in, which is why I've kept it in.
Yeah, sorry, there were some pending comments from a partial review of an early version, which I forgot to clean up.
@ -0,0 +49,4 @@version=$2# Assert that the git commit existsgit -C "$project_root" cat-file -e "${commit}^{commit}"Thanks!
Yes, that's how I meant it, but didn't express it clearly.
Good point. I'm not sure what would be a better name. Perhaps "input"? Or "editable" (sounds a bit weird, but perhaps less ambiguous)? Or
config_file? Ah, perhapseditable_config_files- a bit long, but probably clearer: the plural suggests it includes the mainconfig.handMBEDTLS_USER_CONFIG_FILEif it exists.For "final", I'm wondering about "after_adjust"? But I'm happy with "final" too.
Sure. Also, if we can't converge on better names quickly, I wouldn't block this series of PRs over this. Since this is internal and unlikely to be edited every other day, we can probably just compensate sub-optimal naming with comments.
LGTM, thanks!
I don't consider the remaining naming issue to be a blocker: I'm happy to improve naming if we quickly find a scheme we can all agree on, otherwise getting this merged in time for the release seems more important than getting internal names right.
Looks good, just one note on portability of the newly introduced code.
@ -0,0 +70,4 @@for header in "$@"; dogit -C "$project_root" show "$commit:$header" >>"$temp_file"donesed -n 's![/ ]*# *define *\([A-Z_a-z][0-9A-Z_a-z]*\).*!\1!p' <"$temp_file" |Given that we just introduced config_checks_generator.py it would make sense to do this whole functionality in python, and not rely on external tools like sed and temporary files?
This is even more important if we consider that sed is not using the same syntax across GNU and BSD/MacOS
It would only need to pull regex, subprocess and argparse
The equivalent would be.
parsing_rex = re.compile(r'^[\s/*]*#\s*define\s+([A-Za-z_]\w*)\b', re.MULTILINE)I wont be blocking the PR for this
Given that the main users of this script would be us, I think we can get away with it with a comment, just giving some nomeclature on the names, rather than effectively changing the code.
If as we polish it we come up with better ones, we can do it later.
@ -0,0 +70,4 @@for header in "$@"; dogit -C "$project_root" show "$commit:$header" >>"$temp_file"donesed -n 's![/ ]*# *define *\([A-Z_a-z][0-9A-Z_a-z]*\).*!\1!p' <"$temp_file" |I agree that for maintainability this script should be probably in Python. But I had started to write it in sh, and we needed the result urgently, so I polished and commited the sh code that I had rather than rewrite the thing in Python.
This script won't run on the CI and we'll probably only ever run it manually a couple of times each time we make a new major version. I almost left it in a commit message, but the adjust part was getting a bit too complicated for that.
AFAICT I haven't used any non-POSIX sed feature.
Since we're pressed for time and we don't have a consensus on better names, I'm going ahead and merging with the current names.
@ -0,0 +44,4 @@def __init__(self) -> None:"""Load all files containing historical option lists."""self._history_dir = os.path.join(build_tree.framework_root(), 'history')One thing I think I didn't document properly, so I'll at least leave a note here: why are the historical option lists in the framework, rather than in each consuming project? After all, you'd normally expect each project to know about itself.
First of all, I didn't want running tests to ever need to access another branch. Checking for removed options requires knowledge about two older commits: the end of the previous major version, and the beginning of the current major version. This historical knowledge is stored in the branch under test (including its submodules) so that the branch can be built and tested on its own.
Mbed TLS needs to know about internal options of crypto, so that it doesn't complain if crypto internally sets an option that was public in 3.6 but removed in 1.0/4.0. This could be communicated upwards. But crypto also needs to know about public options of the new version of Mbed TLS, so that it doesn't complain if
crypto_config.htries to set an X.509 or TLS option: we want to allow users to just have a single config file that's used as both the crypto config and the mbedtls config. So projects need information to propagate upwards as well as downwards. Since the crypto build doesn't have access to an mbedtls tree, the information about current Mbed TLS needs to be in crypto or in the framework.Having information about Mbed TLS in crypto doesn't make a lot of sense, and is just as bad in terms of putting things in the wrong project as having that information in the framework. So I put everything in the framework. This way at least it's in one place.
This does mean that we'll sometimes want to update the framework because of a change in a superproject. But this will only be needed when we remove an option, and that normally only ever happens while we're preparing a major version. For 1.0/4.0, because the checks came in so late before the release, we'll probably do just one update of the 1.0/4.0 data after the release. For 2.0/5.0, if this mechanism still exists in its current form, we'll probably do a few more updates, or maybe we'll turn off the 3.6-to-1.0/4.0 checks first and then only add 1.0-to-2.0 checks close to the 2.0 release.