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.

Sign up for GitHub

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

Jump to bottom

wasm: use static registration for runtimes #14014

Merged
merged 7 commits into from
Nov 17, 2020
Merged

wasm: use static registration for runtimes #14014

merged 7 commits into from
Nov 17, 2020

Conversation

lizan
Copy link
Member

@lizan lizan commented Nov 13, 2020

Partially addresses #12574. Refactored test instantiate to removes many ifdefs.

Commit Message:
Additional Description:
Risk Level: Low
Testing: CI
Docs Changes:
Release Notes:
Platform Specific Features:

Signed-off-by: Lizan Zhou [email protected]

@lizan lizan requested a review from PiotrSikora as a code owner November 13, 2020 10:52
@lizan lizan requested a review from mathetake November 13, 2020 10:52
PiotrSikora
PiotrSikora reviewed Nov 13, 2020
View reviewed changes
Copy link
Contributor

@PiotrSikora PiotrSikora left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! Could you verify that this works with --define wasm=disabled when some of those values evaluate to an empty list? Should we include this config in the CI?

CODEOWNERS
@@ -92,6 +92,8 @@ extensions/filters/common/original_src @snowp @klarose
/*/extensions/filters/network/wasm @PiotrSikora @lizan
# webassembly common extension
/*/extensions/common/wasm @PiotrSikora @lizan
# webassembly runtimes
/*/extensions/wasm_runtime/ @PiotrSikora @lizan
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for not putting it under extensions/common/wasm/runtimes or similar?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we put any actual extension registrations under extensions/common, extensions/common is for libraries used by extensions.

@lizan
Copy link
Member Author

lizan commented Nov 13, 2020

Looks good, thanks! Could you verify that this works with --define wasm=disabled when some of those values evaluate to an empty list? Should we include this config in the CI?

Windows CI or Arm64 CI does this :)

@lizan
fix deps metadata
471e7de 
Signed-off-by: Lizan Zhou <[email protected]>
@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).

🐱

Caused by: #14014 was synchronize by lizan.

see: more, trace.

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Nov 13, 2020
@lizan
fix docs
76ea113 
Signed-off-by: Lizan Zhou <[email protected]>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #14014 was synchronize by lizan.

see: more, trace.

lizan added 2 commits November 13, 2020 16:52
@lizan
fix test and tidy
2d29d5b 
Signed-off-by: Lizan Zhou <[email protected]>
@lizan
fix format
5419d41 
Signed-off-by: Lizan Zhou <[email protected]>
@PiotrSikora
Copy link
Contributor

Looks good, thanks! Could you verify that this works with --define wasm=disabled when some of those values evaluate to an empty list? Should we include this config in the CI?

Windows CI or Arm64 CI does this :)

Why? V8 should work fine on both Windows and Arm64.

PiotrSikora
PiotrSikora previously approved these changes Nov 14, 2020
View reviewed changes
mathetake
mathetake previously approved these changes Nov 14, 2020
View reviewed changes
Copy link
Member

@mathetake mathetake 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 so much! 🙂

@mattklein123 mattklein123 self-assigned this Nov 16, 2020
@mattklein123
Copy link
Member

Check CI?

/wait

@lizan
fix CI
050c5e4 
Signed-off-by: Lizan Zhou <[email protected]>
@lizan lizan dismissed stale reviews from mathetake and PiotrSikora via 050c5e4 November 16, 2020 20:38
@lizan
fix CI
af5bb20 
Signed-off-by: Lizan Zhou <[email protected]>
mattklein123
mattklein123 approved these changes Nov 17, 2020
View reviewed changes
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!

@lizan lizan merged commit 77e1372 into envoyproxy:master Nov 17, 2020
@lizan lizan deleted the wasm_vm_registration branch November 17, 2020 01:57
mpuncel added a commit to mpuncel/envoy that referenced this pull request Nov 18, 2020
@mpuncel
Merge branch 'master' into HEAD
ed7a0f1 
* master: (117 commits)
  vrp: allow supervisord to open its log file (envoyproxy#14066)
  [http1] fix H/1 response pipelining (envoyproxy#13983)
  wasm: make dependency clearer (envoyproxy#14062)
  docs: updating 100-continue docs (envoyproxy#14040)
  quiche: fix stream trailer decoding issue (envoyproxy#13871)
  tidy: use last_github_commit script instead of target branch (envoyproxy#14052)
  stats: use RE2 and a better pattern to accelerate a single stats tag-extraction RE (envoyproxy#8831)
  wasm: use static registration for runtimes (envoyproxy#14014)
  grpc-json-transcoder: Add support for configuring unescaping behavior (envoyproxy#14009)
  ci: fix CodeQL-build by removing deprecated set-env command (envoyproxy#14046)
  config: fix crash when type URL doesn't match proto. (envoyproxy#14031)
  Build: Propagate user-supplied tags to external headers library. (envoyproxy#14016)
  [test host utils] use make_shared to avoid memory leaks (envoyproxy#14042)
  jwt_authn: update to jwt_verify_lib with 1 minute clock skew (envoyproxy#13872)
  quiche: update QUICHE tar (envoyproxy#13949)
  sds: improve watched directory documentation. (envoyproxy#14029)
  log the internal error message from *SSL when the cert and private key doesn't match (envoyproxy#14023)
  wasm: fix CPE for Wasmtime. (envoyproxy#14024)
  docs: Bump sphinxext-rediraffe version (envoyproxy#13996)
  CDS: remove warming cluster if CDS response desired (envoyproxy#13997)
  ...
andreyprezotto pushed a commit to andreyprezotto/envoy that referenced this pull request Nov 24, 2020
@lizan @andreyprezotto
wasm: use static registration for runtimes (envoyproxy#14014)
bbfe4fb 
Partially addresses envoyproxy#12574. Refactored test instantiate to removes many ifdefs.

Commit Message:
Additional Description:
Risk Level: Low
Testing: CI
Docs Changes:
Release Notes:
Platform Specific Features:

Signed-off-by: Lizan Zhou <[email protected]>
qqustc pushed a commit to qqustc/envoy that referenced this pull request Nov 24, 2020
@lizan @qqustc
wasm: use static registration for runtimes (envoyproxy#14014)
ca7a6ee 
Partially addresses envoyproxy#12574. Refactored test instantiate to removes many ifdefs.

Commit Message:
Additional Description:
Risk Level: Low
Testing: CI
Docs Changes:
Release Notes:
Platform Specific Features:

Signed-off-by: Lizan Zhou <[email protected]>
Signed-off-by: Qin Qin <[email protected]>
@PiotrSikora PiotrSikora mentioned this pull request May 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

@PiotrSikora PiotrSikora PiotrSikora left review comments

@mathetake mathetake mathetake left review comments

Assignees

@mattklein123 mattklein123

Labels
deps Approval required for changes to Envoy's external dependencies
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lizan @PiotrSikora @mattklein123 @mathetake