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

extension: use bool_flag to control extension enablement #14094

Closed
wants to merge 2 commits into from

Conversation

lizan
Copy link
Member

@lizan lizan commented Nov 19, 2020

Signed-off-by: Lizan Zhou lizan@tetrate.io

Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@lizan
Copy link
Member Author

lizan commented Nov 19, 2020

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).

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.

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
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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.

@htuch
Copy link
Member

htuch commented Nov 23, 2020

LMK when this is ready for merge review.
/wait

lizan added a commit that referenced this pull request Dec 8, 2020
Pulling bazel stuff out of #14094. Use the new kill request extension as example.

Signed-off-by: Lizan Zhou <lizan@tetrate.io>
@mattklein123 mattklein123 assigned junr03 and unassigned htuch Dec 18, 2020
Base automatically changed from master to main January 15, 2021 23:01
@github-actions
Copy link

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 Feb 22, 2021
@github-actions
Copy link

github-actions bot commented Mar 2, 2021

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!

@github-actions github-actions bot closed this Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale stalebot believes this issue/PR has not been touched recently waiting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants