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

feat(symbolize): Bazel build system define HAVE_SYMBOLIZE #1116

Merged

Conversation

hcoona
Copy link
Contributor

@hcoona hcoona commented Jul 31, 2024

link.h is usually provided by glibc. Current implementation of symbolize can work on link.h. Enhance the symbolize detection logic to support link.h by defining HAVE_SYMBOLIZE in bazel build description file.

The choice of altering bazel build file is for the following reasons

  1. "glog moved away from using preprocessor logic in recent releases because this approach is error prone. Contributions are expected to follow the methodology for consistency reasons." according to @sergiud
  2. CMake build file have supported it.

This PR close #1084, supersede #1085 and #1114. It also fixes #1111.

Copy link
Member

@drigz drigz left a comment

Choose a reason for hiding this comment

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

LGTM from a Bazel point-of-view, but leaving approval to @sergiud as he's mentioned in the comment.

@codecov-commenter
Copy link

codecov-commenter commented Jul 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.06%. Comparing base (570c7e4) to head (40d47d8).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1116      +/-   ##
==========================================
+ Coverage   64.04%   64.06%   +0.02%     
==========================================
  Files          20       20              
  Lines        2578     2580       +2     
  Branches      898      907       +9     
==========================================
+ Hits         1651     1653       +2     
- Misses        662      663       +1     
+ Partials      265      264       -1     

see 1 file with indirect coverage changes

Copy link
Collaborator

@sergiud sergiud left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Bazel part is looking good. I have just a remark regarding the comment: there is no strict rule for how features should be detected. In this specific instance, however, we prefer the use of the build system for consistency and maintainability reasons.

src/symbolize.h Outdated Show resolved Hide resolved
@sergiud sergiud added the bug label Aug 1, 2024
@sergiud sergiud added this to the 0.7.2 milestone Aug 1, 2024
@hcoona hcoona requested a review from sergiud August 2, 2024 02:33
Symbolization support on Linux and BSD requires link.h which is usually
provided by the GNU C Library (glibc). Assume the header to be present
at all times by unconditionally defining HAVE_SYMBOLIZE on the
corresponding platforms.
@sergiud sergiud force-pushed the dev/shuaizhang/bazel-define-have-symbolize branch from 40d47d8 to f0f7d3c Compare August 2, 2024 18:31
@sergiud sergiud merged commit 65896fe into google:master Aug 2, 2024
135 of 136 checks passed
sergiud pushed a commit that referenced this pull request Aug 2, 2024
Symbolization support on Linux and BSD requires link.h which is usually
provided by the GNU C Library (glibc). Assume the header to be present
at all times by unconditionally defining HAVE_SYMBOLIZE on the
corresponding platforms.
@hcoona hcoona deleted the dev/shuaizhang/bazel-define-have-symbolize branch August 5, 2024 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants