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.

Sign up for GitHub

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

Jump to bottom

formatting: reject incorrect protobuf namespaces #2969

Merged
merged 2 commits into from
Apr 3, 2018
Merged

formatting: reject incorrect protobuf namespaces #2969

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
11e65d1
formatting: reject incorrect protobuf namespaces
akonradi Apr 2, 2018
ce23024
Auto-fix proto namespace errors
akonradi Apr 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
formatting: reject incorrect protobuf namespaces 
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.

Signed-off-by: Alex Konradi <akonradi@google.com>
  • Loading branch information
@akonradi
akonradi committed Apr 2, 2018
commit 11e65d194b09412a4e55e3da0505545aefb47d50
2 changes: 1 addition & 1 deletion source/server/http/admin.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ Http::Code AdminImpl::handlerConfigDump(absl::string_view, Http::HeaderMap&,
for (const auto& key_callback_pair : config_tracker_.getCallbacksMap()) {
ProtobufTypes::MessagePtr message = key_callback_pair.second();
RELEASE_ASSERT(message);
Protobuf::Any any_message;
ProtobufWkt::Any any_message;
any_message.PackFrom(*message);
config_dump_map[key_callback_pair.first] = any_message;
}
Expand Down
23 changes: 23 additions & 0 deletions tools/check_format.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,36 @@ def checkProtobufExternalDepsBuild(file_path):
def checkProtobufExternalDeps(file_path):
if whitelistedForProtobufDeps(file_path):
return True
protobuf_type_errors = {
# Well-known types should be referenced from the ProtobufWkt namespace.
"Protobuf::Any": "ProtobufWkt::Any",
"Protobuf::Empty": "ProtobufWkt::Empty",
"Protobuf::ListValue": "ProtobufWkt:ListValue",
"Protobuf::NULL_VALUE": "ProtobufWkt::NULL_VALUE",
"Protobuf::StringValue": "ProtobufWkt::StringValue",
"Protobuf::Struct": "ProtobufWkt::Struct",
"Protobuf::Value": "ProtobufWkt::Value",

# Maps including strings should use the protobuf string types.
"Protobuf::MapPair<std::string": "Protobuf::MapPair<Envoy::ProtobufTypes::String",

# Other common mis-namespacing of protobuf types.
"ProtobufWkt::Map": "Protobuf::Map",
"ProtobufWkt::MapPair": "Protobuf::MapPair",
"ProtobufUtil::MessageDifferencer": "Protobuf::util::MessageDifferencer"
}

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():
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if error in text:
printError("%s uses an unexpected protobuf namespace reference; instead of %s, use %s"
% (file_path, error, replacement))
return True


Expand Down