Skip to content

Remove rapidjson direct dependency#19826

Closed
keith wants to merge 1 commit intoenvoyproxy:mainfrom
keith:ks/remove-rapidjson-direct-dependency
Closed

Remove rapidjson direct dependency#19826
keith wants to merge 1 commit intoenvoyproxy:mainfrom
keith:ks/remove-rapidjson-direct-dependency

Conversation

@keith
Copy link
Copy Markdown
Member

@keith keith commented Feb 4, 2022

This has been replaced by nlohmann/json

#4705

Unfortunately this library cannot be removed entirely because it is used
by opencensus-cpp. We continue to load it here because upstream they do
not pin the dependency to a specific version, which could lead to random
failures as that library is updated upstream.

Signed-off-by: Keith Smiley keithbsmiley@gmail.com

@repokitteh-read-only
Copy link
Copy Markdown

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: #19826 was opened by keith.

see: more, trace.

@keith
Copy link
Copy Markdown
Member Author

keith commented Feb 4, 2022

This might be too early, but I wanted to see what the impact of this change would be

This has been replaced by nlohmann/json

envoyproxy#4705

Unfortunately this library cannot be removed entirely because it is used
by opencensus-cpp. We continue to load it here because upstream they do
not pin the dependency to a specific version, which could lead to random
failures as that library is updated upstream.

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@keith keith force-pushed the ks/remove-rapidjson-direct-dependency branch from edb394f to 92b833e Compare February 4, 2022 21:27
@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Feb 4, 2022
@repokitteh-read-only
Copy link
Copy Markdown

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 @wrowe

🐱

Caused by: #19826 was synchronize by keith.

see: more, trace.

@moderation
Copy link
Copy Markdown
Contributor

Related issue: #9958

OpenCensus is deprecated - can we just remove this extension enabling the path to remove RapidJSON?

@keith
Copy link
Copy Markdown
Member Author

keith commented Feb 5, 2022

Oh good to know! Based on https://github.com/census-instrumentation/opencensus-cpp/search?q=rapidjson I think I would have to remove zipkin entirely, is that something that could be done now or does that need to wait for that issue?

@moderation
Copy link
Copy Markdown
Contributor

Zipkin is its own tracer extension and I don't believe removing the OpenCensus extension will impact that

https://github.com/envoyproxy/envoy/tree/main/source/extensions/tracers

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 7, 2022

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Mar 7, 2022
@keith keith closed this Mar 7, 2022
@keith keith deleted the ks/remove-rapidjson-direct-dependency branch March 7, 2022 20:11
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 stale stalebot believes this issue/PR has not been touched recently

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants