Skip to content

Fix building extensions as WebAssembly modules (again).#2528

Merged
istio-testing merged 3 commits intoistio:masterfrom
PiotrSikora:fix_wasm_again
Nov 7, 2019
Merged

Fix building extensions as WebAssembly modules (again).#2528
istio-testing merged 3 commits intoistio:masterfrom
PiotrSikora:fix_wasm_again

Conversation

@PiotrSikora
Copy link
Copy Markdown
Contributor

@PiotrSikora PiotrSikora commented Nov 6, 2019 •
edited
Loading

Those WebAssembly modules are not built as part of the CI yet,
because we cannot run Docker-in-Docker.

Signed-off-by: Piotr Sikora piotrsikora@google.com

@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Nov 6, 2019
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Nov 6, 2019
@istio-testing istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 6, 2019
@PiotrSikora
Fix building extensions as WebAssembly modules (again).
d713245 
Those WebAssembly modules are not built as part of the CI yet,
because we cannot run Docker-in-Docker.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora PiotrSikora added the cherrypick/release-1.4 Set this label on a PR to auto-merge it to the release-1.4 branch label Nov 6, 2019
@PiotrSikora PiotrSikora marked this pull request as ready for review November 6, 2019 16:35
@PiotrSikora PiotrSikora requested a review from a team November 6, 2019 16:35
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Nov 6, 2019
kyessenov
kyessenov reviewed Nov 6, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@kyessenov kyessenov left a comment

Choose a reason for hiding this comment

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

Is there a way to avoid running docker in CI? Maybe use runc or gvisor? It's generally insecure to run docker in docker.

@bianpengyuan
Copy link
Copy Markdown
Contributor

bianpengyuan commented Nov 6, 2019

If it is not expensive, how about adding emscripten etc into proxy builder image? This PR will need update after #2527

@PiotrSikora
Copy link
Copy Markdown
Contributor Author

PiotrSikora commented Nov 6, 2019

Yeah, I think we have to install wasmsdk inside the build images to make this work, but that's separate from this PR.

@PiotrSikora
Merge remote-tracking branch 'origin/master'
3a40446 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@bianpengyuan
Copy link
Copy Markdown
Contributor

bianpengyuan commented Nov 7, 2019

Can you please incorporate change from #2527

@PiotrSikora
Copy link
Copy Markdown
Contributor Author

PiotrSikora commented Nov 7, 2019

Can you please incorporate change from #2527

I believe that PR is already included in the latest merge? Or did you have anything else in mind?

@PiotrSikora
Copy link
Copy Markdown
Contributor Author

PiotrSikora commented Nov 7, 2019

/test proxy-presubmit-tsan

@bianpengyuan
Copy link
Copy Markdown
Contributor

bianpengyuan commented Nov 7, 2019

It adds two new files extensions/common/util.{h.cc} which I believe should also be added into stats plugin's source?

@PiotrSikora
review: propagate build failures.
7a055c4 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Copy link
Copy Markdown
Contributor Author

PiotrSikora commented Nov 7, 2019

@bianpengyuan good catch, it looks that build_wasm.sh scripts were not exiting on failures, so this error was masked by metadata plugin building successfully. Fixed now.

@PiotrSikora
Copy link
Copy Markdown
Contributor Author

PiotrSikora commented Nov 7, 2019

Also, note that this removes extensions/stats/plugin.wasm, which presumably was committed by accident?

/hold

bianpengyuan
bianpengyuan approved these changes Nov 7, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@bianpengyuan bianpengyuan left a comment

Choose a reason for hiding this comment

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

Yes the wasm is checked in by accident. Thanks for removing it! :)

@istio-testing istio-testing merged commit c56a27e into istio:master Nov 7, 2019
@istio-testing
Copy link
Copy Markdown
Collaborator

istio-testing commented Nov 7, 2019

In response to a cherrypick label: #2528 failed to apply on top of branch "release-1.4":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	Makefile.core.mk
M	extensions/common/node_info_cache.cc
Falling back to patching base and 3-way merge...
Removing extensions/stats/plugin.wasm
Auto-merging extensions/common/node_info_cache.cc
CONFLICT (content): Merge conflict in extensions/common/node_info_cache.cc
Auto-merging Makefile.core.mk
Patch failed at 0001 Fix building extensions as WebAssembly modules (again).

@PiotrSikora PiotrSikora mentioned this pull request Nov 7, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kyessenov kyessenov kyessenov left review comments

+1 more reviewer

@bianpengyuan bianpengyuan bianpengyuan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cherrypick/release-1.4 Set this label on a PR to auto-merge it to the release-1.4 branch cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@PiotrSikora @bianpengyuan @istio-testing @kyessenov @googlebot