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

dynamic_modules: switch to crate_universe to remove manual bindgen #36240

Merged
merged 11 commits into from
Sep 23, 2024

Conversation

mathetake
Copy link
Member

@mathetake mathetake commented Sep 20, 2024

Commit Message: dynamic_modules: switch to crate_universe to remove manual bindgen
Additional Description:
This is a follow up on #35914 which ended up requiring a manual
step to generate the binding for the ABI headers.
This switches to use crate_universe and use the bindgen as a dev-dependency.
As a result, we can adopt the build.rs script to generate the C bindings at build
time instead of the manual bindgen step.

This also allows us to freely add a crate dependency when it becomes necessary.

Risk Level: N/A
Testing: N/A
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Takeshi Yoneda <[email protected]>
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: #36240 was opened by mathetake.

see: more, trace.

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Sep 20, 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 @moderation

🐱

Caused by: #36240 was opened by mathetake.

see: more, trace.

@mathetake
Copy link
Member Author

cc @phlax - i am trying to enable crate_universe here which I think doesn't work for gcc and compiletime_options jobs. let's see

Signed-off-by: Takeshi Yoneda <[email protected]>
@mathetake
Copy link
Member Author

so crate_universe itself seems to be working. The next is to see whether the bindgen actually works or not

Signed-off-by: Takeshi Yoneda <[email protected]>
@mathetake mathetake changed the title try using crate_universe dynamic_modules: switch to crate_universe to remove manual bindgen step Sep 20, 2024
Signed-off-by: Takeshi Yoneda <[email protected]>
@mathetake mathetake changed the title dynamic_modules: switch to crate_universe to remove manual bindgen step dynamic_modules: switch to crate_universe to remove manual bindgen Sep 20, 2024
Signed-off-by: Takeshi Yoneda <[email protected]>
@mathetake
Copy link
Member Author

i think it's all good now 🤞

Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
@mathetake mathetake marked this pull request as ready for review September 20, 2024 12:31
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Awesome, thanks.

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Sep 23, 2024
@mattklein123 mattklein123 merged commit 6bccf47 into envoyproxy:main Sep 23, 2024
40 checks passed
@mathetake mathetake deleted the bindgen branch September 23, 2024 23:33
@asedeno
Copy link
Contributor

asedeno commented Sep 24, 2024

NB: it appears this broke the s390x build.

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.

4 participants