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

Unable to compile protobufs in pb namespace #8349

Closed
rowillia opened this issue Feb 26, 2021 · 7 comments · Fixed by #8423
Closed

Unable to compile protobufs in pb namespace #8349

rowillia opened this issue Feb 26, 2021 · 7 comments · Fixed by #8423

Comments

@rowillia
Copy link
Contributor

In Nov 2020 protobuf reserved the namespace pb for protobuf to move to in the future (

// Protobuf intends to move into the pb:: namespace.
namespace protobuf_future_namespace_placeholder {}
namespace pb = ::protobuf_future_namespace_placeholder;
). This is very problematic for us as all of our protobufs are in the pb namespace and breaks our ability to upgrade. This seems to be quite a common pattern in the industry - there's over 10k examples on github alone - https://github.com/search?l=Protocol+Buffer&p=1&q=%22package+pb%22&type=Code .

If at all possible can we roll back that change? I suspect it has the potential to break a huge number of users beyond just us.

What version of protobuf and what language are you using?
Version: 3.15.0
Language: C++

What operating system (Linux, Windows, ...) and version?
OSX

What runtime / compiler are you using (e.g., python version or gcc version)
clang --version
Apple clang version 12.0.0 (clang-1200.0.32.28)
Target: x86_64-apple-darwin19.6.0
Thread model: posix

What did you do?
Steps to reproduce the behavior:

Create file pb/people/models/people.proto:

syntax = "proto3";

package pb.people.models;

message Person {
  string name = 1;
  int32 id = 2;
  string email = 3;
}
protoc -I. --cpp_out=. pb/people/models/people.proto
clang -g -fwrapv -O3 -Wall -I. -I /usr/local/include/ -c pb/people/models/people.pb.cc -o a.o -std=c++11

What did you expect to see
No output, Code compiles

What did you see instead?

./pb/people/models/people.pb.h:57:11: error: redefinition of 'pb' as different kind of symbol
namespace pb {
          ^
../cytobuf/third_party/protobuf/src/google/protobuf/port.h:44:11: note: previous definition is here
namespace pb = ::protobuf_future_namespace_placeholder;
          ^
In file included from pb/people/models/people.pb.cc:4:
./pb/people/models/people.pb.h:70:18: error: no member named 'people' in namespace 'protobuf_future_namespace_placeholder'
template<> ::pb::people::models::Person* Arena::CreateMaybeMessage<::pb::people::models::Person>(Arena*);
           ~~~~~~^
./pb/people/models/people.pb.h:70:74: error: no member named 'people' in namespace 'protobuf_future_namespace_placeholder'
template<> ::pb::people::models::Person* Arena::CreateMaybeMessage<::pb::people::models::Person>(Arena*);
...
...

Make sure you include information that can help us debug (full error message, exception listing, stack trace, logs).

Anything else we should know about your project / environment

@haberman
Copy link
Member

Thanks for the report, and I'm sorry this is causing trouble for you.

We'll be monitoring this bug and (any other reports we get about this problem) to get a sense for how many users are affected. We don't want to break users, but given how widely used protobuf is, it's hard to make a change like this that won't break anybody. A fair number of the results of the GitHub search appear to be false positives, so it's difficult to quantify how many users are affected.

@rowillia
Copy link
Contributor Author

Thanks so much for taking a look! I can empathize with how hard it is to make changes in a project relied on my so many. That said, I can comment that this will break how we at Lyft use protobuf and require us to do an extremely costly migration, never upgrade past 3.14, or maintain our own fork - none of which are appealing options. I can see if I can poke around in sourcegraph or another system to find other places in OSS that will be hurt by this.

What's going to happen to the well known types that are already in the google.protobuf namespace?

Happy to brainstorm any solutions here, would an alternative namespace like pb_implementation or pb_internal work for you folks?

@haberman
Copy link
Member

Thanks for the extra info about the impact, it's really helpful. To answer your questions:

What's going to happen to the well known types that are already in the google.protobuf namespace?

Unless the protobuf language itself gets type aliases, it's not possible to change these in a backward-compatible way. So these are currently out of scope.

Happy to brainstorm any solutions here, would an alternative namespace like pb_implementation or pb_internal work for you folks?

The main goal of this change is that users can refer to pb::Message as a new public namespace. It's not about an internal namespace change.

This change is motivated by the fact that the namespace is currently bifurcated: open-source protobuf uses google::protobuf::Message while Google internally uses proto2::Message. Neither of these is ideal: google::protobuf:: is too long, while proto2:: suggests that the namespace is related to syntax = "proto2", while they are actually unrelated.

Several other alternatives we have evaluated have already been deemed infeasible (pbuf::, proto::, etc).

dlj-NaN added a commit to dlj-NaN/protobuf that referenced this issue Mar 23, 2021
This closes protocolbuffers#8349, although we will probably still pursue some other name in the future.
dlj-NaN added a commit to dlj-NaN/protobuf that referenced this issue Mar 23, 2021
This closes protocolbuffers#8349, although we will probably still pursue some other name in the future.
@rowillia
Copy link
Contributor Author

Thanks so much @dlj-NaN and @haberman!

@georgthegreat
Copy link
Contributor

I believe that migration to namespace pb is still possible.
In order to resolve possible conflicts, namespace pb should be used by the library itself, and google::protobuf should be used as an alias:

// in every protobuf source and header file
namespace pb {
  class Message {
    ....
  };  // end of class Message
} // end of namespace pb

// in port.h or some other common place
namespace pb {};
namespace google {
namespace protobuf = ::pb;
} //

@haberman, @acozzette, @dlj-NaN, do you think if such approach might work? This will put the code from package pb into the same namespace as the library itself, but I do not think it would cause any troubles.

I am a real fan of migrating into shorter namespace, and pb is a great candidate for it.

@haberman
Copy link
Member

haberman commented Apr 3, 2021

Hi @georgthegreat, thanks for the feedback. It's true that if our symbols were declared directly in pb, then they could coexist with user types declared in that namespace. But it is not a good long-term strategy for us to share a namespace with user types. It opens up the possibility of conflicts between the library and users.

I think we would want to adopt compatibility guarantees like ABSL, which states:

You may not open namespace absl. You are not allowed to define additional names in namespace absl, nor are you allowed to specialize anything we provide. We expect there may eventually be templates that are valid extension points provided by Abseil — there are not currently. (This is similar to the restriction in the C++ standard — there are very few things you are allowed to do in namespace std.)

You may not forward declare Abseil APIs. This is actually a sub-point of “do not depend on the signatures of Abseil APIs” as well as “do not open namespace absl”, but can be surprising. Any refactoring that changes template parameters, default parameters, or namespaces will be a breaking change in the face of forward-declarations.

@georgthegreat
Copy link
Contributor

But it is not a good long-term strategy for us to share a namespace with user types.

I agree, but this does not allow to change the status quo: for every possible short namespace there will be some company named accordingly, and this company will be using protobufs enclosed into corresponding namespace. When the code does not belong to Plethora Brands Company, PB Company Limited or PB Products (none of three looks like an IT company to me), enclosing the code into package pb does not makes a lot of sense to me. Backward compatibility matters though.

IMO, the strategy suggested above should be declared and the migration should take place disregarding possible co-existence of a user-defined namespace / package. This should happen is a separate major release (3.16 is a good candidate).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants