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

bump cel-cpp #36661

Merged
merged 1 commit into from
Oct 23, 2024
Merged

bump cel-cpp #36661

merged 1 commit into from
Oct 23, 2024

Conversation

asedeno
Copy link
Contributor

@asedeno asedeno commented Oct 17, 2024

Update cel-cpp to HEAD as of when this PR was made, and drop the now-obsolete patches.

google/cel-cpp@b03438a

Risk Level:
Testing: CI

Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #36661 was opened by asedeno.

see: more, trace.

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Oct 17, 2024
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @phlax

🐱

Caused by: #36661 was opened by asedeno.

see: more, trace.

@asedeno
Copy link
Contributor Author

asedeno commented Oct 17, 2024

This PR will complete and obsolete #36167 and #35514. (cc: @kyessenov)

@asedeno
Copy link
Contributor Author

asedeno commented Oct 17, 2024

We've got a gcc build issue; I'm working on fixing that upstream.

@asedeno asedeno marked this pull request as ready for review October 17, 2024 20:04
@phlax
Copy link
Member

phlax commented Oct 22, 2024

@asedeno this (re-) adds a bit of noise to the build

external/com_google_cel_cpp/common/internal/reference_count.cc:83:33: warning: offset of on non-standard-layout type 'cel::common_internal::(anonymous namespace)::ReferenceCountedString' [-Winvalid-offsetof]
    internal::SizedDelete(that, offsetof(ReferenceCountedString, data_) + size);
                                ^                                ~~~~~
/opt/llvm/lib/clang/14.0.0/include/stddef.h:104:24: note: expanded from macro 'offsetof'
#define offsetof(t, d) __builtin_offsetof(t, d)
                       ^                     ~
INFO: From Compiling internal/well_known_types.cc:
In file included from external/com_google_cel_cpp/internal/well_known_types.cc:15:
external/com_google_cel_cpp/internal/well_known_types.h:1133:52: warning: private field 'fields_key_field_string_type_' is not used [-Wunused-private-field]
  google::protobuf::FieldDescriptor::CppStringType fields_key_field_string_type_;
 INFO: From Compiling common/type_factory.cc:
external/com_google_cel_cpp/common/type_factory.cc:25:6: warning: unused function 'IsValidMapKeyType' [-Wunused-function]
bool IsValidMapKeyType(const Type& type) {
     ^
1 warning generated.

can we resolve upstream or failing that patch it out?

@asedeno
Copy link
Contributor Author

asedeno commented Oct 22, 2024

@asedeno this (re-) adds a bit of noise to the build

Working on it now; which specific build(s) did you pull these from? I want to cross-reference against HEAD to see what new warnings have popped up.

@phlax
Copy link
Member

phlax commented Oct 22, 2024

iirc the ./ci/do_ci.sh release.test_only in prechecks

@phlax
Copy link
Member

phlax commented Oct 22, 2024

https://github.com/envoyproxy/envoy/actions/runs/11391229612/job/31694585390#step:12:583

Signed-off-by: Alejandro R. Sedeño <[email protected]>
@asedeno
Copy link
Contributor Author

asedeno commented Oct 22, 2024

The warnings have been resolved (or silenced) upstream.

@asedeno
Copy link
Contributor Author

asedeno commented Oct 22, 2024

/retest

Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @asedeno

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Oct 23, 2024
@phlax phlax merged commit a5f9f35 into envoyproxy:main Oct 23, 2024
21 checks passed
@asedeno asedeno deleted the update_cel branch October 23, 2024 14:38
krinkinmu added a commit to krinkinmu/cel-cpp that referenced this pull request Nov 4, 2024
gcc complains about FieldBackedMapImpl::ListKeys when overloaded-virtual
check is enabled. When using -Werror, warnings like this are converted
to errors breaking builds.

That's what happened to gcc build of Envoy a while back. The issue in
Envoy was fixed by introducing a patch for cel-cpp that was applied to
the source during Envoy build (see
envoyproxy/envoy#31814).

Unfortunately during cel-cpp version update in Envoy instead of
updating the patch, dropping the parts that are not needed anymore, we
dropped the whole cep-cpp patch all together in
envoyproxy/envoy#36661. And now gcc can't build
Envoy again.

This change makes the change to silence the gcc warning in the cel-cpp
codebase, which hopefully will be a bit more durable than maintaing a
patch on the Envoy side.

Signed-off-by: Mikhail Krinkin <[email protected]>
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.

2 participants