-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
extension: use bool_flag to control extension enablement #14094
Conversation
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
This is initially intended to solve #12574, while I think it has bigger impact by introducing command line flag to control every extension enabled/disabled, so I just post this as a draft. While I think this is great to have but we should probably also discuss whether we want to do flavor that came up before. That might be also doable through flags as described https://docs.bazel.build/versions/master/skylark/config.html but I haven't digged into. @mattklein123 @htuch cc @envoyproxy/maintainers as a side effect this resolves #14027 but incompletely (i.e. notests). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement in readability and Bazel fu. One thing that was a bit unclear is how defaults around wavm have changed.
echo "Building binary with wasm=wavm..." | ||
bazel build "${BAZEL_BUILD_OPTIONS[@]}" --define wasm=wavm "${COMPILE_TIME_OPTIONS[@]}" -c dbg @envoy//source/exe:envoy-static --build_tag_filters=-nofips | ||
echo "Building binary with wavm..." | ||
bazel build "${BAZEL_BUILD_OPTIONS[@]}" "${COMPILE_TIME_OPTIONS[@]}" -c dbg @envoy//source/exe:envoy-static --build_tag_filters=-nofips |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's no longer clear from the CLI that this is wavm, is there an explicit toggle possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because it's now a flag and not a define, it can be override, so I moved back the wavm enable flag into L288 above, and disable when we compile with wasmtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I think this makes it harder to read; having this explicit here rather than under COMPILE_TIME_OPTIONS would make later maintenance/understandability better.
LMK when this is ready for merge review. |
Pulling bazel stuff out of #14094. Use the new kill request extension as example. Signed-off-by: Lizan Zhou <lizan@tetrate.io>
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! |
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Signed-off-by: Lizan Zhou lizan@tetrate.io