Use def_ref_codec_lib instead of including def_ref_code.{cc|hpp} as sources #37
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ources def_ref_codec.cc is listed as a source of both object_codec_lib and def_ref_codec_lib targets. It does not cause problems for binaries that only depend on one of those targets, but when you start depending on both you might hit some issues. Specifically, in Envoy, we have an extension that depends on //hessian2:codec_impl_lib and //hessian2/basic_codec:object_codec_lib (see https://github.com/envoyproxy/envoy/blob/d82453231f89cb021eb716a23805073cff1f7179/source/extensions/filters/network/dubbo_proxy/BUILD#L12-L22). As a result def_ref_codec.cc file is referenced as a source twice: 1. as one of the sources of //hessian2/basic_codec:object_codec_lib 2. as one of the sources of //hessian2/basic_codec:def_ref_codec_lib that is a dependency of //hesian2:codec_impl_lib. Using the same source file multiple times as a source in Bazel does not seem to be common, but it also may cause issues. For once, in C++ in principle having the same object file listed multiple times when linking may result in multiple definitions error (depending on the actual content of the object file), but in my case I hit a different issue. I'm working on a project in which I have to build Envoy, but, unfortunately, I cannot use the Envoy CI image with all the tools preinstalled, as a result environment in which I build Envoy is somewhat different from the one provided by Envoy CI Docker image. And when I try to build Envoy, at some point Bazel calls dwp tool to collect debugging information from dwo files into a single dwp file. The command Bazel uses to call dwp references def_ref_codec.dwo twice because def_ref_codec.cc is referenced twice as a source. dwp tool is not equipped to deal with that and complains about duplicate debug info and ultimately fails build: ``` error: duplicate DWO ID (852C89B14FE85A8F) in 'external/com_github_alibaba_hessian2_codec/hessian2/basic_codec/def_ref_codec.cc' (from 'bazel-out/k8-opt/bin/external/com_github_alibaba_hessian2_codec/hessian2/basic_codec/_objs/def_ref_codec_lib/def_ref_codec.dwo') and 'external/com_github_alibaba_hessian2_codec/hessian2/basic_codec/def_ref_codec.cc' (from 'bazel-out/k8-opt/bin/external/com_github_alibaba_hessian2_codec/hessian2/basic_codec/_objs/object_codec_lib/def_ref_codec.dwo') ``` I know that when you build Envoy with Envoy CI docker image the same issue does not happen, but after debugging for a few days, I still don't quite understand why. I tried different versions of the dwp tool, but they all return the same error, which makes me think that the issue is likely not in the dwp tool itself. Even though I still don't know why it works currently in Envoy, it seems that this change makes sense (we should not list the same cc file multiple times as a source) and it should be safe (the same code is still listed as a dependency), so I figured that folks might be open to accept the change even with some unknowns. Signed-off-by: Mikhail Krinkin <krinkin.m.u@gmail.com>
|
|
|
+cc @wbpcode |
krinkinmu
added a commit
to krinkinmu/envoy
that referenced
this pull request
Jan 3, 2025
It's not immediately clear why in Dalec hessian2_codec does not build successfully, while using Envoy CI Docker image it builds just fine. However, it's pretty clear to me that hessian2 repo contains an issue with Bazel build rules and fixing it resolves the Dalec build problem. I've sent a patch to the upstream hessian2_codec repository (see alibaba/hessian2-codec#37). The patch, if accepted will fix the problem for future versions of Envoy, but in 1.32 we are stuck with this issue. Signed-off-by: Mikhail Krinkin <krinkin.m.u@gmail.com>
krinkinmu
added a commit
to krinkinmu/envoy
that referenced
this pull request
Jan 3, 2025
It's not immediately clear why in Dalec hessian2_codec does not build successfully, while using Envoy CI Docker image it builds just fine. However, it's pretty clear to me that hessian2 repo contains an issue with Bazel build rules and fixing it resolves the Dalec build problem. I've sent a patch to the upstream hessian2_codec repository (see alibaba/hessian2-codec#37). The patch, if accepted will fix the problem for future versions of Envoy, but in 1.32 we are stuck with this issue. Signed-off-by: Mikhail Krinkin <krinkin.m.u@gmail.com>
krinkinmu
added a commit
to krinkinmu/envoy
that referenced
this pull request
Jan 3, 2025
It's not immediately clear why in Dalec hessian2_codec does not build successfully, while using Envoy CI Docker image it builds just fine. However, it's pretty clear to me that hessian2 repo contains an issue with Bazel build rules and fixing it resolves the Dalec build problem. I've sent a patch to the upstream hessian2_codec repository (see alibaba/hessian2-codec#37). The patch, if accepted will fix the problem for future versions of Envoy, but in 1.32 we are stuck with this issue. Signed-off-by: Mikhail Krinkin <krinkin.m.u@gmail.com>
krinkinmu
added a commit
to krinkinmu/envoy
that referenced
this pull request
Jan 3, 2025
It's not immediately clear why in Dalec hessian2_codec does not build successfully, while using Envoy CI Docker image it builds just fine. However, it's pretty clear to me that hessian2 repo contains an issue with Bazel build rules and fixing it resolves the Dalec build problem. I've sent a patch to the upstream hessian2_codec repository (see alibaba/hessian2-codec#37). The patch, if accepted will fix the problem for future versions of Envoy, but in 1.32 we are stuck with this issue. Signed-off-by: Mikhail Krinkin <krinkin.m.u@gmail.com>
|
Hey Folks, it seems like the PR didn't get a reviewer automatically assigned, is there something that I need to do to make it happen? Thank you! |
|
@krinkinmu Sorry to reply late. It looks good to me. |
krinkinmu
added a commit
to krinkinmu/envoy
that referenced
this pull request
Jan 14, 2025
The new version includes alibaba/hessian2-codec#37 that contains a fix for one of the issues listeed in envoyproxy#37911 (number 5 on the list). TL;DR: the issue is that in hessian2-codec the same cc file is used as a source for two different libraries. The code that depends on both of those libraries at the same time might have an issue because of the same source used several times. Signed-off-by: Mikhail Krinkin <mkrinkin@microsoft.com>
RyanTheOptimist
pushed a commit
to envoyproxy/envoy
that referenced
this pull request
Jan 14, 2025
Commit Message: The new version includes alibaba/hessian2-codec#37 that contains a fix for one of the issues listeed in #37911 (number 5 on the list). TL;DR: the issue is that in hessian2-codec the same cc file is used as a source for two different libraries. The code that depends on both of those libraries at the same time might have an issue because of the same source used several times. Additional Description: Related to #37911 Risk Level: Low Testing: Tested that Envoy builds with introduced changes, additionally run unit tests in `//test/extensions/common/dubbo/...` `//test/extensions/filters/network/dubbo_proxy/...` and `//test/extensions/filters/network/generic_proxy/codecs/dubbo/...`. dubbo is the extension that uses hessian2 codec (the full test suite will run as part of the PR checks as well). Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a --------- Signed-off-by: Mikhail Krinkin <mkrinkin@microsoft.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
def_ref_codec.cc is listed as a source of both object_codec_lib and def_ref_codec_lib targets. It does not cause problems for binaries that only depend on one of those targets, but when you start depending on both you might hit some issues.
Specifically, in Envoy, we have an extension that depends on //hessian2:codec_impl_lib and //hessian2/basic_codec:object_codec_lib (see https://github.com/envoyproxy/envoy/blob/d82453231f89cb021eb716a23805073cff1f7179/source/extensions/filters/network/dubbo_proxy/BUILD#L12-L22). As a result def_ref_codec.cc file is referenced as a source twice:
Using the same source file multiple times as a source in Bazel does not seem to be common, but it also may cause issues.
For once, in C++ in principle having the same object file listed multiple times when linking may result in multiple definitions error (depending on the actual content of the object file), but in my case I hit a different issue.
I'm working on a project in which I have to build Envoy, but, unfortunately, I cannot use the Envoy CI image with all the tools preinstalled, as a result environment in which I build Envoy is somewhat different from the one provided by Envoy CI Docker image.
And when I try to build Envoy, at some point Bazel calls dwp tool to collect debugging information from dwo files into a single dwp file. The command Bazel uses to call dwp references def_ref_codec.dwo twice because def_ref_codec.cc is referenced twice as a source. dwp tool is not equipped to deal with that and complains about duplicate debug info and ultimately fails build:
I know that when you build Envoy with Envoy CI docker image the same issue does not happen, but after debugging for a few days, I still don't quite understand why. I tried different versions of the dwp tool, but they all return the same error, which makes me think that the issue is likely not in the dwp tool itself.
Even though I still don't know why it works currently in Envoy, it seems that this change makes sense (we should not list the same cc file multiple times as a source) and it should be safe (the same code is still listed as a dependency), so I figured that folks might be open to accept the change even with some unknowns.