google / glog

C++ implementation of the Google logging module
http://google.github.io/glog/
BSD 3-Clause "New" or "Revised" License
7.1k stars 2.07k forks source link

feat(symbolize): Symbolize detection support HAVE_LINK_H #1114

Closed hcoona closed 3 months ago

hcoona commented 3 months ago

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, AKA. define HAVE_SYMBOLIZE if defined(HAVE_LINK_H).

This PR close #1084 and supersede #1085.

There's a few discussion in #1085 about why make changes in src/symbolize.h instead of bazel/glog.bzl. My justification is that this is an enhancement to symbolize detection logic rather than an alignment between bazel and cmake build systems. link.h can provide us ElfW thus can support our implementation.

I would like to open a separate issue if we still want to discuss whether we should define HAVE_SYMBOLIZE in bazel/glog.bzl.

sergiud commented 3 months ago

Thanks for the PR. However, my comment from #1085 still stands as I do not want to rely on assumptions but rather use the build system to detect (or specify, in Bazel case) the feature availability. This also allows to keep the preprocessor logic (which is not straightforward to debug) simple.

hcoona commented 3 months ago

Thanks for the PR. However, my comment from #1085 still stands as I do not want to rely on assumptions but rather use the build system to detect (or specify, in Bazel case) the feature availability. This also allows to keep the preprocessor logic (which is not straightforward to debug) simple.

@sergiud I understand your opinion. To further discuss the difference in our views, we need to define the boundaries of the problem, i.e., what kind of functionality should be detected and provided by the build system, and what kind of functionality should be detected by the macro system.

I believe my change is brief and simple to understand. We have detected HAVE_ELF_H with the preprocessor, why can't we detect HAVE_LINK_H?

I would like you to provide further information to help me understand on what principles we should base our decision on this issue.

sergiud commented 3 months ago

I provided a very clear explanation why the detection should be performed by the build system and not by the preprocessor. What information are you missing exactly?

Also, I do not understand why do you keep pushing for a preprocessor based solution? Why can't you update the Bazel files instead?

hcoona commented 3 months ago

I provided a very clear explanation why the detection should be performed by the build system and not by the preprocessor. What information are you missing exactly?

Also, I do not understand why do you keep pushing for a preprocessor based solution? Why can't you update the Bazel files instead?

@sergiud I feel you are annoyed about the discussion. I agree we shouldn't waste too much time on quite "simple" problem. However, at least 2 warm community contributors are confused in this problem, so I think it's not trivial.

I totally understand you prefer to solve the problem by build system rather than by the preprocessor. And I believe I understand your justification. (But I don't think re-implement the detection logic in each build systems with each build system specific DSL are even better than implement it in the preprocessor.)

What confused me are

  1. We do have had 2 approaches to detect HAVE_SYMBOLIZE.
  2. Current implementation of glog in bazel build system do nothing on detecting HAVE_SYMBOLIZE. It totally rely on the preprocessor.

Now, I want to enhance the detection logic, where should be the correct place? What kind of functionality should be detected and provided by the build system, and what kind of functionality should be detected by the macro system? These questions are exactly what I was asking for.

If the rule is we stop adding any logic to the preprocessor and gradually move all (this detection) logic from the preprocessor to the build system, I would happily follow it.

I mean I never aware of that your are announcing a new rule, are you?

sergiud commented 3 months ago

You keep ignoring my reasoning for implementing the logic using the build system instead of the preprocessor which is why the discussion is not moving forward.

To add to my previous comments:

  1. Your proposed fix for symbolization detection using the preprocessor is insufficient. It is both inconsistent to the CMake approach and solves the problem only partially by relying on assumptions.
  2. The issue is present in Bazel builds only but not those built using CMake. Hence, the fix should be limited to Bazel files only.
  3. 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. There is no announcement necessary for this specific design decision.

Now, I want to enhance the detection logic, where should be the correct place?

I already answered this question here. Please limit the modifications to Bazel files only.

What kind of functionality should be detected and provided by the build system, and what kind of functionality should be detected by the macro system?

This question is irrelevant for solving this specific problem. In general, however, there must be a good reason for using the preprocessor instead of the build system.

hcoona commented 3 months ago

Thanks for your clarification. You have made a design decision but nobody in the community know about it. You said you prefer to put the detection logic in build system but the community didn't know it had been settled without any room for a further discussion. I would rather you just say that we have made a rule, just follow the rule to keep consistency.

I would submit a new PR with the 2 changes:

  1. Add HAVE_SYMBOLIZE for linux only opts in bazel/glog.bzl.
  2. Add comments to stop people adding new changes to the preprocessor of HAVE_SYMBOLIZE detection

Let me know if you have any concerns.

sergiud commented 3 months ago

You have made a design decision but nobody in the community know about it.

Given this is your first contribution to glog, I don't believe you are in the position to speak on behalf of the community.

You said you prefer to put the detection logic in build system but the community didn't know it had been settled without any room for a further discussion. I would rather you just say that we have made a rule, just follow the rule to keep consistency.

You seem to have an attitude problem. I don't appreciate neither your baseless accusations nor your attempts of twisting of my words. I'm asking you to keep the discussions civilized and limited to technical problems only, refraining from any personal attacks. Your behavior will not be tolerated. You have been warned.

hcoona commented 3 months ago

@sergiud , I apologize if my previous messages came across as confrontational. It was never my intention to offend or misrepresent your viewpoints. I recognize there may have been some gaps in our communication, and there are aspects of the project’s background and the discussions that I am not fully aware of, which may have led to a lack of common understanding.

I truly value a harmonious work environment and am committed to improving how I communicate, ensuring it's based on well-substantiated facts rather than assumptions. I appreciate your patience and guidance as we work through this issue, and I am keen to adapt my approach to align better with the project’s methodologies and expectations. Thank you for addressing these concerns directly, and I look forward to making a constructive contribution moving forward.

sergiud commented 3 months ago

Thank you. Let's move forward with #1116.