Fix linking with Protobuf 23 #4104
Conversation
Google introduced abseil as a new dependency of Protobuf, but CMake's FindProtobuf hasn't been updated yet to link with abseil. Thus, CMake is now advised to use the config provided by Protobuf instead of its own. In case no config can be found, we fall back to the old behavior. The Protobuf version constraint is removed since it only applies with `P4C_USE_PREINSTALLED_PROTOBUF=ON` anyway, in which case we'll want to use whatever preinstalled version is available. Furthermore, the variables `PROTOBUF_LIBRARY` and `PROTOBUF_PROTOC_EXECUTABLE` are set correctly so executing `protoc` and linking with Protobuf doesn't fail. Fix p4lang#4055 See also protocolbuffers/protobuf#12292
|
@jkhsjdhjs Have you already tried going through the steps to have a signed ONF CLA on file, and Github isn't recognizing this? |
|
@jafingerhut Yep, I actually signed the CLA before I created this PR, but I only added my email address, not my GitHub username. I thought this would suffice, since the commits also contain the authors email address. |
|
@jkhsjdhjs I have contacted someone at ONF about this issue, to see if they can help determine what is required for your ONF CLA agreement to be recognized. |
|
@jkhsjdhjs Can you please send me an email at andy.fingerhut@gmail.com so we can communicate about the ONF CLA issue? I'd rather not clutter Github issues with such discussion, if it takes a few rounds. |
|
We have merged PRs in the past even though the ONF CLA check failed. I think we can also merge this one. |
|
@fruffy I understand. Since we have this CLA in place, with automated checking mechanisms, and often new contributors have difficulty finding out how to become approved via these automated processes, I would love to get expert ONF staff attention on one particular case, in hopes that we can document any confusing or easily-skipped parts of the process. |
Makes sense. Iirc, there was an additional field with the Github username one had to fill out to get the bot to work. At the bottom of the form. |
|
I filled this field with my username and also hit "sign addendum", my GitHub username is definitely part of the CLA. Not sure why it doesn't work, we're still awaiting a reply from the ONF folks. |
Should we just merge this? |
If you do not mind too much, I would like to give ONF staff another week or so to see if the issue with the ONF CLA checker bot can be corrected here (and ideally for anyone else that wants to contribute in the future, if there is something that needs fixing). |
|
But if merging this is blocking any other work, yeah, go ahead and merge and we can use a separate Github issue to track the ONF CLA testing/fixing. |
|
Alright, problem's solved. It was a Layer 8 issue: I previously added my GitHub username as |
Google introduced abseil as a new dependency of Protobuf, but CMake's FindProtobuf hasn't been updated yet to link with abseil. Thus, CMake is now advised to use the config provided by Protobuf instead of its own. In case no config can be found, we fall back to the old behavior.
The Protobuf version constraint is removed since it only applies with
P4C_USE_PREINSTALLED_PROTOBUF=ONanyway, in which case we'll want to use whatever preinstalled version is available.Furthermore, the variables
PROTOBUF_LIBRARYandPROTOBUF_PROTOC_EXECUTABLEare set correctly so executingprotocand linking with Protobuf doesn't fail.Fix #4055
See also protocolbuffers/protobuf#12292