Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2259693002:
[wasm] binary and test for hosts' integration tests (Closed)
|
Created:
4 years, 4 months ago by Mircea Trofin Modified:
4 years, 4 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[wasm] binary and test for hosts' integration tests
Ensure wasm binaries intended to be used in hosts of v8, such as
chromium, are up to date.
See https://codereview.chromium.org/2255673003/
BUG=v8:5072
Committed: https://crrev.com/93b7251f7482246e9f57ae118295452f4b38dd75
Cr-Commit-Position: refs/heads/master@{#38694}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 21 (12 generated)
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [wasm] binary and test for hosts' integration tests Ensure wasm binaries intended to be used in hosts of v8, such as chromium, are up to date. BUG=v8:5072 ========== to ========== [wasm] binary and test for hosts' integration tests Ensure wasm binaries intended to be used in hosts of v8, such as chromium, are up to date. BUG=v8:5072 ==========
mtrofin@chromium.org changed reviewers: + bradnelson@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [wasm] binary and test for hosts' integration tests Ensure wasm binaries intended to be used in hosts of v8, such as chromium, are up to date. BUG=v8:5072 ========== to ========== [wasm] binary and test for hosts' integration tests Ensure wasm binaries intended to be used in hosts of v8, such as chromium, are up to date. See https://codereview.chromium.org/2255673003/ BUG=v8:5072 ==========
lgtm assuming not https://codereview.chromium.org/2259693002/diff/1/test/mjsunit/wasm/ensure-wa... File test/mjsunit/wasm/ensure-wasm-binaries-up-to-date.js (right): https://codereview.chromium.org/2259693002/diff/1/test/mjsunit/wasm/ensure-wa... test/mjsunit/wasm/ensure-wasm-binaries-up-to-date.js:10: load('test/mjsunit/mjsunit.js'); This doesn't blow up?
https://codereview.chromium.org/2259693002/diff/1/test/mjsunit/wasm/ensure-wa... File test/mjsunit/wasm/ensure-wasm-binaries-up-to-date.js (right): https://codereview.chromium.org/2259693002/diff/1/test/mjsunit/wasm/ensure-wa... test/mjsunit/wasm/ensure-wasm-binaries-up-to-date.js:10: load('test/mjsunit/mjsunit.js'); On 2016/08/18 03:14:45, bradnelson wrote: > This doesn't blow up? Where would it blow up? This is only run on the v8 side. The chrome side is interested in the wasm file.
The CQ bit was checked by mtrofin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [wasm] binary and test for hosts' integration tests Ensure wasm binaries intended to be used in hosts of v8, such as chromium, are up to date. See https://codereview.chromium.org/2255673003/ BUG=v8:5072 ========== to ========== [wasm] binary and test for hosts' integration tests Ensure wasm binaries intended to be used in hosts of v8, such as chromium, are up to date. See https://codereview.chromium.org/2255673003/ BUG=v8:5072 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== [wasm] binary and test for hosts' integration tests Ensure wasm binaries intended to be used in hosts of v8, such as chromium, are up to date. See https://codereview.chromium.org/2255673003/ BUG=v8:5072 ========== to ========== [wasm] binary and test for hosts' integration tests Ensure wasm binaries intended to be used in hosts of v8, such as chromium, are up to date. See https://codereview.chromium.org/2255673003/ BUG=v8:5072 Committed: https://crrev.com/93b7251f7482246e9f57ae118295452f4b38dd75 Cr-Commit-Position: refs/heads/master@{#38694} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/93b7251f7482246e9f57ae118295452f4b38dd75 Cr-Commit-Position: refs/heads/master@{#38694}
Message was sent while issue was closed.
bradnelson@google.com changed reviewers: + bradnelson@google.com
Message was sent while issue was closed.
Meant including mjsunit twice, as would presumably happen when run with the usual runner script. But it's probably fine, JS is forgiving.
Message was sent while issue was closed.
mstarzinger@chromium.org changed reviewers: + mstarzinger@chromium.org
Message was sent while issue was closed.
Not a huge fan of having binary files in the repository. Is the plan to add more .wasm files in the near term?
Message was sent while issue was closed.
On 2016/08/18 07:44:14, Michael Starzinger wrote: > Not a huge fan of having binary files in the repository. Is the plan to add more > .wasm files in the near term? No preference here for checking in binaries either, just couldn't come up with a better alternative to the following problem, and needed to make progress - we can evolve this to a better place if we find a more suitable alternative. Here's the problem: we want to write integration tests for wasm features that require changes to chrome, like the compiled module serialization feature. The wasm format is still in flux. Options considered were: 1) Let chrome tests use a wasm building API (like the one in mjsunit). Problem is, if we make changes to the APIs, we detect the chrome breakage when we update Chrome with the latest v8. 2) Check in binaries to chrome. If we change the format, we break chrome just as above. 3) Current option. If we change the wasm format, we must regen the wasm file to make the v8 test pass. That CL, then, makes it all or nothing on the chrome side, seamlessly bringing in the change. Happy to hear other options! Thanks, Mircea. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
