formatting: reject incorrect protobuf namespaces #2969
Conversation
Add some checks to the formatting script so that protobuf-related type references use the correct namespaces. This should prevent the need for later fixups like envoyproxy#2963. Signed-off-by: Alex Konradi <akonradi@google.com>
tools/check_format.py
Outdated
| with open(file_path) as f: | ||
| text = f.read() | ||
| if '"google/protobuf' in text or "google::protobuf" in text: | ||
| printError( | ||
| "%s has unexpected direct dependency on google.protobuf, use " | ||
| "the definitions in common/protobuf/protobuf.h instead." % file_path) | ||
| return False | ||
| for error, replacement in protobuf_type_errors.items(): |
There was a problem hiding this comment.
I don't feel that strongly about it, but can we do all of this on a per-line basis with all the other checks? The format script is getting pretty slow.
There was a problem hiding this comment.
Moved this to fixFileContents since the fixes can be performed automatically. I'm going to do a little digging and see if I can speed up the rest of the script.
There was a problem hiding this comment.
Opened #2974 to parallelize formatting. From profiling, it looks like the majority of the time is still spent running clang-format, with header order checking being the next most time-consuming.
Signed-off-by: Alex Konradi <akonradi@google.com>
| @@ -23,6 +23,25 @@ | |||
| HEADER_ORDER_PATH = os.path.join( | |||
| os.path.dirname(os.path.abspath(sys.argv[0])), "header_order.py") | |||
|
|
|||
| PROTOBUF_TYPE_ERRORS = { | |||
| # Well-known types should be referenced from the ProtobufWkt namespace. | |||
| "Protobuf::Any": "ProtobufWkt::Any", | |||
There was a problem hiding this comment.
The other alternative is to whitelist allowed proto WKT types, since these are bounded. This PR is a good improvement for now, so let's ship it, thanks!
|
|
||
| # Fix incorrect protobuf namespace references. | ||
| for error, replacement in PROTOBUF_TYPE_ERRORS.items(): | ||
| line = line.replace(error, replacement) |
There was a problem hiding this comment.
There was a problem hiding this comment.
+1 thanks, was just about to comment on this.
There was a problem hiding this comment.
Opened #2979 to add to the 'check' flow.
tools/check_format.py was modified in envoyproxy#2969 to automatically fix protobuf type namespace errors in 'fix' mode, but it did not complain about them when run in 'check' mode. This commit fixes the lack of complaints from 'check' mode. Signed-off-by: Alex Konradi <akonradi@google.com>
tools/check_format.py was modified in #2969 to automatically fix protobuf type namespace errors in 'fix' mode, but it did not complain about them when run in 'check' mode. This commit fixes the lack of complaints from 'check' mode. Signed-off-by: Alex Konradi <akonradi@google.com>
Add some checks to the formatting script so that protobuf-related type references use the correct namespaces. This should prevent the need for later fixups like #2963.
Risk Level: Low
Testing: checked out and ran format script on code from before #2963 and found errors as expected.
Release Notes: N/A