Skip to content

wasm: upgrade proxy-wasp-cpp-host for shared data apis#19769

Merged
RyanTheOptimist merged 2 commits intoenvoyproxy:mainfrom
jamesmulcahy:wasm-shared-data-keys-remove
Feb 4, 2022
Merged

wasm: upgrade proxy-wasp-cpp-host for shared data apis#19769
RyanTheOptimist merged 2 commits intoenvoyproxy:mainfrom
jamesmulcahy:wasm-shared-data-keys-remove

Conversation

@jamesmulcahy
Copy link
Copy Markdown
Contributor

@jamesmulcahy jamesmulcahy commented Feb 1, 2022

  • Exposes keys() and remove() methods (though not directly through the
    ABI).

Signed-off-by: James Mulcahy jmulcahy@netflix.com

Commit Message:
Additional Description: This updates the wasm host dependency so envoy users can start accessing the SharedData::keys() and SharedData::remove() functions via the WASM foreign functions interface.
Risk Level: low
Testing: no additional testing beyond the automated testing in
Docs Changes:
Release Notes:
Platform Specific Features:

@jamesmulcahy
wasm: upgrade proxy-wasp-cpp-host for shared data apis
a10b97a 
- Exposes keys() and remove() methods (though not directly through the
  ABI).

Signed-off-by: James Mulcahy <jmulcahy@netflix.com>
@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Feb 1, 2022
@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Feb 1, 2022

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: #19769 was opened by jamesmulcahy.

see: more, trace.

@jamesmulcahy
Copy link
Copy Markdown
Contributor Author

jamesmulcahy commented Feb 1, 2022

Commits between these two versions:

$ git log --decorate=off --oneline 44a63ebe58770a07e36ff1ff149f176fd8371810..819dcc02bd2bc6fdec07720379c4d522d6b7da08
819dcc0 Add keys() and remove() methods to SharedData. (#203)
10d5b06 wamr: remove dependency on LLVM. (#239)
d6b6d50 build: increase reach of the buildifier check. (#238)
bab2ef1 Update Bazel to v5.0.0. (#235)
b42c336 Update rules_foreign_cc to v0.7.1. (#236)
8a38042 Update rules_rust to latest (with Rust v1.58.1). (#237)
6640fcf build: stop installing Ninja. (#234)

@jamesmulcahy
Copy link
Copy Markdown
Contributor Author

jamesmulcahy commented Feb 1, 2022

cc/ @PiotrSikora @mathetake

@moderation
Copy link
Copy Markdown
Contributor

moderation commented Feb 1, 2022

/lgtm deps

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Feb 1, 2022
@PiotrSikora
Copy link
Copy Markdown
Contributor

PiotrSikora commented Feb 1, 2022 

10d5b06 wamr: remove dependency on LLVM. (#239)

Since you're propagating this change, could you remove the LLVM dependency from WAMR in bazel/foreign_cc/BUILD?

Also, it might be best not to mention those Shared Data APIs, since we're not exposing them to the end users.

@jamesmulcahy
Copy link
Copy Markdown
Contributor Author

jamesmulcahy commented Feb 1, 2022

Since you're propagating this change, could you remove the LLVM dependency from WAMR in bazel/foreign_cc/BUILD?

Sure, does that involve removing the LLVM_DIR cache entry too? Or just the deps = [":llvm"] line?

Also, it might be best not to mention those Shared Data APIs, since we're not exposing them to the end users.

It's not mentioned in any release notes, and the commit message is clear that we're not exposing them in the ABI. I did want to convey my intent for the PR though, which seems reasonable.

@mathetake
Copy link
Copy Markdown
Member

mathetake commented Feb 2, 2022

curious what's the plan/timeline for user facing ABI change?

@PiotrSikora
Copy link
Copy Markdown
Contributor

PiotrSikora commented Feb 2, 2022

Sure, does that involve removing the LLVM_DIR cache entry too? Or just the deps = [":llvm"] line?

Both.

It's not mentioned in any release notes, and the commit message is clear that we're not exposing them in the ABI. I did want to convey my intent for the PR though, which seems reasonable.

Sure, although we update dependencies without any justification other than "there is new version" all the time.

curious what's the plan/timeline for user facing ABI change?

There is no plan to expose those functions in 0.2.x ABI (although, I might revisit this if the next ABI slips again).

In the meantime, @jamesmulcahy is going to access them via Foreign Functions.

@phlax
Copy link
Copy Markdown
Member

phlax commented Feb 2, 2022 •
edited
Loading

...snip...

apologies: i misread this as being on the bazel/wasm pr - please ignore me 8/

@phlax
Copy link
Copy Markdown
Member

phlax commented Feb 2, 2022 •
edited
Loading

cc @moderation ^^

@jamesmulcahy
wasm: remove wamr dependency on llvm
7d7194b 
Signed-off-by: James Mulcahy <jmulcahy@netflix.com>
@jamesmulcahy
Copy link
Copy Markdown
Contributor Author

jamesmulcahy commented Feb 2, 2022

@PiotrSikora Added a commit to remove the wamr LLVM dependency

PiotrSikora
PiotrSikora approved these changes Feb 2, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Thanks!

wrowe
wrowe reviewed Feb 3, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@wrowe wrowe left a comment

Choose a reason for hiding this comment

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

LGTM

@RyanTheOptimist RyanTheOptimist enabled auto-merge (squash) February 4, 2022 16:20
@RyanTheOptimist RyanTheOptimist merged commit 33a1129 into envoyproxy:main Feb 4, 2022
joshperry pushed a commit to joshperry/envoy that referenced this pull request Feb 13, 2022
@jamesmulcahy
wasm: upgrade proxy-wasp-cpp-host for shared data apis (envoyproxy#19769
2b65bb7 
)

Exposes keys() and remove() methods (though not directly through the
ABI).
Signed-off-by: James Mulcahy jmulcahy@netflix.com

Commit Message:
Additional Description: This updates the wasm host dependency so envoy users can start accessing the SharedData::keys() and SharedData::remove() functions via the WASM foreign functions interface.
Risk Level: low
Testing: no additional testing beyond the automated testing in

Signed-off-by: Josh Perry <josh.perry@mx.com>
phlax pushed a commit to phlax/envoy that referenced this pull request Nov 15, 2022
@jamesmulcahy @phlax
wasm: upgrade proxy-wasp-cpp-host for shared data apis (envoyproxy#19769
6f86eac 
)

Exposes keys() and remove() methods (though not directly through the
ABI).
Signed-off-by: James Mulcahy jmulcahy@netflix.com

Commit Message:
Additional Description: This updates the wasm host dependency so envoy users can start accessing the SharedData::keys() and SharedData::remove() functions via the WASM foreign functions interface.
Risk Level: low
Testing: no additional testing beyond the automated testing in

Signed-off-by: Ryan Northey <ryan@synca.io>
phlax pushed a commit that referenced this pull request Nov 18, 2022
@jamesmulcahy @phlax
wasm: upgrade proxy-wasp-cpp-host for shared data apis (#19769)
98092f0 
Exposes keys() and remove() methods (though not directly through the
ABI).
Signed-off-by: James Mulcahy jmulcahy@netflix.com

Commit Message:
Additional Description: This updates the wasm host dependency so envoy users can start accessing the SharedData::keys() and SharedData::remove() functions via the WASM foreign functions interface.
Risk Level: low
Testing: no additional testing beyond the automated testing in

Signed-off-by: Ryan Northey <ryan@synca.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RyanTheOptimist RyanTheOptimist RyanTheOptimist approved these changes

+2 more reviewers

@wrowe wrowe wrowe left review comments

@PiotrSikora PiotrSikora PiotrSikora approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@wrowe wrowe

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@jamesmulcahy @moderation @PiotrSikora @mathetake @phlax @wrowe @RyanTheOptimist