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

Fix missing port_undef #20052

Closed
wants to merge 1 commit into from
Closed

Conversation

wilstoff
Copy link
Contributor

unparser.cc, arenaz_sampler_test.cc, and message_module.cc included port_def.inc but forgot to include the undef file. This can cause some issues on which absl macros are defined:

In file included from
/opt/protobuf-29/include/google/protobuf/arena.h:36: In file included from
/opt/protobuf-29/include/google/protobuf/arena_align.h:63: /opt/protobuf-29/include/google/protobuf/port_def.inc:542:5: error: function-like macro 'ABSL_HAVE_FEATURE' is not defined #if ABSL_HAVE_FEATURE(address_sanitizer)

@wilstoff wilstoff requested a review from a team as a code owner January 20, 2025 16:00
@wilstoff wilstoff requested review from zhangskz and removed request for a team January 20, 2025 16:00
Copy link

google-cla bot commented Jan 20, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@mkruskal-google mkruskal-google added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 22, 2025
@mkruskal-google
Copy link
Member

Please sign the CLA

@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 22, 2025
@mkruskal-google mkruskal-google added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 22, 2025
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 22, 2025
@@ -347,3 +347,5 @@ PyMODINIT_FUNC PyInit__message() {

return m;
}

#include "google/protobuf/port_undef.inc
Copy link
Member

Choose a reason for hiding this comment

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

Missing end-quote here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for delay, been busy at work upgrading protobuf and coming back to this side fix. Comedy of errors you can tell i'm not actually building locally before pushing. Will reach out again when i consider this ready.

Copy link
Contributor Author

@wilstoff wilstoff Jan 30, 2025

Choose a reason for hiding this comment

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

I believe i should have fixed the author and the silly typos. Tried to do some local building, but our work machines have some clashes with the version of ccache you expect vs we're using. But i believe the changes to be relatively safe to run on your build nodes.

ERROR: /home/bstoffel/.cache/bazel/_bazel_bstoffel/d6c92e1c5e534d31833b4c3ac5594187/external/com_google_absl/absl/base/BUILD.bazel:60:11: Compiling absl/base/log_severity.cc failed: (Exit 1): ccache failed: error executing CppCompile command (from target @@com_google_absl//absl/base:log_severity) /usr/bin/ccache -U_FORTIFY_SOURCE -fstack-protector -Wall -Wthread-safety -Wself-assign -Wunused-but-set-parameter -Wno-free-nonheap-object -fcolor-diagnostics -fno-omit-frame-pointer '-std=c++14' -MD ... (remaining 34 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
/usr/bin/ccache: invalid option -- 'U'
Target //python:google/protobuf/pyext/_message.so failed to build

Copy link
Member

Choose a reason for hiding this comment

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

How are you running tests? You shouldn't need ccache

@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 30, 2025
@google-cla google-cla bot added cla: yes and removed cla: no labels Jan 30, 2025
unparser.cc, arenaz_sampler_test.cc, and message_module.cc included
port_def.inc but forgot to include the undef file.  This can cause
some issues on which absl macros are defined:

In file included from
/opt/protobuf-29/include/google/protobuf/arena.h:36:
In file included from
/opt/protobuf-29/include/google/protobuf/arena_align.h:63:
/opt/protobuf-29/include/google/protobuf/port_def.inc:542:5: error:
function-like macro 'ABSL_HAVE_FEATURE' is not defined
\#if ABSL_HAVE_FEATURE(address_sanitizer)
@acozzette
Copy link
Member

@wilstoff I actually suspect that this is not the right fix for your compiler error. The main purpose of port_undef.inc is to prevent our internal macros from leaking out into non-protobuf code. While it is important to include port_undef.inc at the end of our header files, it doesn't do anything useful in .cc files.

@mkruskal-google mkruskal-google added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 31, 2025
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 31, 2025
@mkruskal-google
Copy link
Member

mkruskal-google commented Jan 31, 2025

@acozzette I assumed this was a unity build situation 🤷‍♂️. We've gotten changes like this before, and in the past have agreed to accept minimal PRs fixing unity issues like this

copybara-service bot pushed a commit that referenced this pull request Jan 31, 2025
We are seeing some GHA breakages; they may have been caused by
#20052 as the Bazel build rules
couldn't find the necessary headers.

PiperOrigin-RevId: 721895082
copybara-service bot pushed a commit that referenced this pull request Jan 31, 2025
We are seeing some GHA breakages; they may have been caused by
#20052 as the Bazel build rules
couldn't find the necessary headers.

PiperOrigin-RevId: 721895082
copybara-service bot pushed a commit that referenced this pull request Jan 31, 2025
We are seeing some GHA breakages; they may have been caused by
#20052 as the Bazel build rules
couldn't find the necessary headers.

PiperOrigin-RevId: 721895082
copybara-service bot pushed a commit that referenced this pull request Jan 31, 2025
We are seeing some GHA breakages; they may have been caused by
#20052 as the Bazel build rules
couldn't find the necessary headers.

PiperOrigin-RevId: 721910271
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants