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

deps: update cel-cpp #27414

Merged
merged 3 commits into from
May 17, 2023
Merged

Conversation

kyessenov
Copy link
Contributor

@kyessenov kyessenov commented May 15, 2023

Commit Message: Updates cel-cpp to latest in OSS. There's another export coming so this PR will be followed up. However, the new export needs the latest protoc, absl, and re2 - which may cause other problems. There is a macro pollution problem with proxy-wasm/proxy-wasm-cpp-sdk#154 addressed by a local patch. I will upstream cel-cpp patch separately.

Risk Level: low
Testing: unit
Docs Changes: none

Signed-off-by: Kuat Yessenov <[email protected]>
@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label May 15, 2023
@repokitteh-read-only
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 @htuch

🐱

Caused by: #27414 was opened by kyessenov.

see: more, trace.

@kyessenov
Copy link
Contributor Author

MSVC does not support weak linkage with ABSL_ATTRIBUTE_WEAK.
I'm not sure how to resolve this apart from disabling CEL on windows. CC @jcking

Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov
Copy link
Contributor Author

It turns out the weak linkage is not necessary for this version of CEL since the new code is not active.

htuch
htuch previously approved these changes May 16, 2023
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

+1 on upstreaming the CEL patches, they look painful to maintain.

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label May 16, 2023
Signed-off-by: Kuat Yessenov <[email protected]>
@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label May 16, 2023
@phlax
Copy link
Member

phlax commented May 16, 2023

@kyessenov it seems this is throwing a lot of warnings in our build - a lot seem to be essentially the same warning

Im guessing we can ~safely ignore the warnings - if so im wondering if we can suppress them in the patch or similar

Copy link
Member

@tyxia tyxia left a comment

Choose a reason for hiding this comment

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

Thank you very much for the help with updating CEL!

@kyessenov
Copy link
Contributor Author

kyessenov commented May 16, 2023

@phlax Yeah I'll fix those warnings upstream. I wanted to unblock @tyxia while we figure out how to build the latest (still not exported) CEL.

@kyessenov
Copy link
Contributor Author

A new update to cel-cpp is in progress google/cel-cpp#167, but it requires very recent protoc/absl - which will not happen quickly.

@yanavlasov
Copy link
Contributor

Will apply the changes in the patch to the cel-cpp SoT repo. However we still need to sort out some issues before we can use cel-cpp HEAD. At this point this will allow CEL matcher work to proceed and patch will go away once we can finally depend on the most recent cel-cpp.

@yanavlasov yanavlasov merged commit 0f378c2 into envoyproxy:main May 17, 2023
reskin89 pushed a commit to reskin89/envoy that referenced this pull request Jul 11, 2023
* deps: update cel-cpp

Signed-off-by: Kuat Yessenov <[email protected]>
Signed-off-by: Ryan Eskin <[email protected]>
phlax pushed a commit to phlax/envoy that referenced this pull request Oct 9, 2023
* deps: update cel-cpp

Signed-off-by: Kuat Yessenov <[email protected]>

Signed-off-by: Kuat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps Approval required for changes to Envoy's external dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants