Skip to content

Remove note about DWARF patch being required#2645

Merged
RyanTheOptimist merged 1 commit intoenvoyproxy:mainfrom
RyanTheOptimist:NoDwarf
Nov 8, 2022
Merged

Remove note about DWARF patch being required#2645
RyanTheOptimist merged 1 commit intoenvoyproxy:mainfrom
RyanTheOptimist:NoDwarf

Conversation

@RyanTheOptimist
Copy link
Contributor

As per #1274 (comment), this patch is no longer required.

Signed-off-by: Ryan Hamilton rch@google.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Signed-off-by: Ryan Hamilton <rch@google.com>
@jpsim
Copy link
Contributor

jpsim commented Nov 1, 2022

Envoy's linux build config still passes --features=per_object_debug_info: https://github.com/envoyproxy/envoy/blob/66e5e6f9ee201a4ebdb7c33cd83d30a72ca357d4/.bazelrc#L44

And that passes -gsplit-dwarf: https://github.com/bazelbuild/bazel/blob/19b8d24531fb728701a85ec050361d434600b5d2/tools/cpp/unix_cc_toolchain_config.bzl#L406

And bloaty still doesn't support that. So from what I can tell, this is still needed?

@RyanTheOptimist
Copy link
Contributor Author

Envoy's linux build config still passes --features=per_object_debug_info: https://github.com/envoyproxy/envoy/blob/66e5e6f9ee201a4ebdb7c33cd83d30a72ca357d4/.bazelrc#L44

And that passes -gsplit-dwarf: https://github.com/bazelbuild/bazel/blob/19b8d24531fb728701a85ec050361d434600b5d2/tools/cpp/unix_cc_toolchain_config.bzl#L406

And bloaty still doesn't support that. So from what I can tell, this is still needed?

Hm! That's curious. Both @alyssawilk and I were able to follow the instructions on that page without applying the patch and got bloaty output successfully. As a result, it seemed like it wasn't required.

On the other hand, @abeyad did have some issue getting bloaty to work. Ali, if you apply the patch, are you able to get bloaty to work?

@abeyad
Copy link
Contributor

abeyad commented Nov 1, 2022

Even applying that patch, I still get:

bloaty: Don't know how to parse DWARF form: 39

Note that the patch doesn't seem to be fully relevant anymore: I could comment out the lines in .bazelrc as JP mentioned, but in bazel/envoy_internal.bzl, the -gsplit-dwarf option no longer exists in that file, so there is nothing to patch there.

Regardless, applying what's left of the patch still gives me the error above.

@RyanTheOptimist
Copy link
Contributor Author

@jpsim WDYT?

@abeyad
Copy link
Contributor

abeyad commented Nov 4, 2022

I forgot to follow up here, but after running bazel build with --config=release-android, the DWARF problem went away, so I don't think we need the patch, and it's not fully accurate now since some of the patch doesn't apply anymore.

@RyanTheOptimist
Copy link
Contributor Author

@jpsim friendly ping

Copy link
Contributor

@jpsim jpsim left a comment

Choose a reason for hiding this comment

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

I'm ok removing this if it's not helpful.

@RyanTheOptimist RyanTheOptimist merged commit 4b2e54d into envoyproxy:main Nov 8, 2022
jpsim added a commit that referenced this pull request Nov 14, 2022
…builder-function

* origin/main:
  ci: hopefully fixing bes timeout failures (#2666)
  Update Envoy (#2660)
  bazel: update rules_jvm_external to 4.5 (#2665)
  Remove note about DWARF patch being required (#2645)
  Bump Lyft Support Rotation (#2661)
  build: remove alwayslink
  set enablePlatformCertificateValidation to false on iOS by default (#2663)
  bump Envoy dep (#2659)
  Implement an iOS platform certificate verifier. (#2638)

Signed-off-by: JP Simard <jp@jpsim.com>
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 this pull request may close these issues.

3 participants