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.

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

Commit ssl_debug_helpers.h #5340

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Dec 15, 2021

Having a generated header file broke running make from a fresh checkout. On the CI, this broke the code-coverage job (which we run nightly but not in PR). It didn't break all.sh because that currently always runs make generated_files before doing a build (for historical reasons, we'll change this at some point).

This is a follow-up on #5329 which didn't actually solve the problem (it did with cmake, but not with make): it declared how to generate the header file, but in the makefile, we don't have dependencies on header files, so there was still nothing instructing to generate the header file.

The simplest solution is to not have an automatically generated header file. The content of that header file doesn't look hard to maintain manually, so I'm converting it to a manually updated file.

CI job with code-coverage → PASS

Only applicable to 3.x.

Having an automatically generated header file makes it harder to have
working build scripts. The content of ssl_debug_helpers_generated.h isn't
likely to change often, so we can update it manually.

Signed-off-by: Gilles Peskine <[email protected]>
It's now under version control and meant to be updated manually.

Signed-off-by: Gilles Peskine <[email protected]>
It's no longer generated, so rename it accordingly.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm gilles-peskine-arm added bug needs-review Every commit must be reviewed by at least two team members, component-platform Portability layer and build scripts needs-reviewer This PR needs someone to pick it up for review size-s Estimated task size: small (~2d) labels Dec 15, 2021
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Looks good to me except I think the new file needs a copyright & license notice.

library/ssl_debug_helpers.h Outdated Show resolved Hide resolved
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

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

2 comments, neither critical

for start, instance in preprocess_c_source_code(source_code, EnumDefinition):
if start in definitions:
continue
if isinstance(instance, EnumDefinition):
definition, prototype = instance.generate_tranlation_function()
definition = instance.generate_tranlation_function()
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, preexisting: generate_tranlation_function() -> generate_translation_function() and in declaration etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Preexisting, so I'm definitely not fixing it here. This is an urgent fix for the CI, not a general cleanup.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fix at #5348.

library/ssl_debug_helpers.h Show resolved Hide resolved
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@davidhorstmann-arm davidhorstmann-arm 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, needs-reviewer This PR needs someone to pick it up for review labels Dec 16, 2021
@gilles-peskine-arm gilles-peskine-arm merged commit cb4dc37 into Mbed-TLS:development Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports bug component-platform Portability layer and build scripts size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants