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

Non-community group submodules #79

Closed
MikeHolman opened this issue May 27, 2016 · 7 comments
Closed

Non-community group submodules #79

MikeHolman opened this issue May 27, 2016 · 7 comments

Comments

@MikeHolman
Copy link
Member

One thing we've recently been thinking a lot about on Chakra is our testing story for WebAssembly. I think it would be ideal if we could use this as a submodule, and have our testing build wasts using sexpr-wasm on the fly. This would allow us to run WebAssembly tests through our CI without needing to check in a whole bunch of binary files.

However, what makes me hesitant to do this is that V8 is a submodule of sexpr-wasm, and (for obvious reasons) I'd rather not have V8 as a submodule of ChakraCore. So what do you guys think? Feasible to remove that dependency? Also, I'm wondering how V8 handles wasm testing? Presumably you don't have this as a submodule either (that seems like there would be some weird recursion), so do you check in .wasm files, or something else?

@binji
Copy link
Member

binji commented May 28, 2016

Yes, this make sense. Originally d8 was useful for testing as it was the only binary format consumer, and I wanted to make sure I was generating something that could actually work :) Since then I've added sexpr-wasm's wasm-wast and wasm-interp utilities, so at least can prove that it's self-consistent.

A couple thoughts:

  1. I've spent some effort to ensure that sexpr-wasm builds without any submodules. So you could include sexpr-wasm as a non-recursive submodule and it should build properly. (It's not really tested, though, so there likely will be regressions. It shouldn't be too hard to add this to Travis, however.)
  2. I recently changed all references from d8 to js instead, and made it possible to choose the JavaScript engine via the --js-executable flag. I tested this against the SpiderMonkey engine, and added functionality to download+build it, so we could do something similar w/ Chakra. The v8 repo is more self-contained (read: smaller), so I made it a submodule, but AFAICT SpiderMonkey requires the entire mozilla dev repo, so I didn't make it a submodule.

Perhaps the nicest solution here is to demote v8 from submodule, to building the same way SpiderMonkey does, and add similar scripts to build via Chakra. It's nice being able to test against real wasm consumers locally. See scripts/build-d8.sh, scripts/build-sm.sh, scripts/download-sm.sh, etc.

Also: I recently added the WebAssembly/testsuite-js repo, which contains a single JavaScript file per spec test file, generated by sexpr-wasm.

What do you think?

@Cellule
Copy link
Contributor

Cellule commented Jun 7, 2016

WebAssembly/testsuite-js is interesting, however same issue regarding submodule (since it has sexpr-wasm).
I think having sexpr-wasm as a non recursive submodule might do the trick.
I've been fiddling around with to make it compatible on Windows and with ch.exe.
Soon I should have fixes ready for Windows, then I'll start looking into integrating this tool into our CI and I'll which issues comes up then.

@Cellule
Copy link
Contributor

Cellule commented Jun 10, 2016

How would you feel like about breaking down this repo into separate projects.
I was thinking something like

  • WebAssembly/Tools: Would contain most if this repo as in sexpr-wasm, wasm-interp, wasm-wast and maybe wasm-interp-sq. It would also contain necessary testing only for those executables (nothing needed a javascript engine).
  • WebAssembly/CommonTests: A repository of shared test cases to run in the different wasm implementation, but nothing to actually run the tests. Would contain a reference to the Spec testsuite.
  • WebAssembly/EngineTesting: A repository to actually do testing of the different wasm implementation. It would have a reference to WebAssembly/Tools and WebAssembly/CommonTests.

This way, it would allow external project to use WebAssembly/Tools and/or WebAssembly/CommonTests without any reference to things related to the specific implementors

@binji
Copy link
Member

binji commented Jun 13, 2016

What tests would be in WebAssembly/CommonTests? I originally wrote the tests in sexpr-wasm-prototype because the tests in the spec repo were not exhaustive enough. But in general I think it would be better to move anything substantive into the existing testsuite.

Is it enough to just move the engine tests out? I think that makes a lot of sense. It's not really the responsibility of sexpr-wasm to validate the engine's implementations. :)

@binji
Copy link
Member

binji commented Jun 13, 2016

#89 is a quick PR to remove the JavaScript engine stuff. What do you think?

@Cellule
Copy link
Contributor

Cellule commented Jun 14, 2016

I guess I was thinking about WebAssembly/CommonTests because sexpr-wasm contained more tests than the official test suite namely the d8 test folder.
I don't know if it's appropriate to add those tests to the test suite, it might be interesting to look into this.

For the PR, at quick glance it looks good. I made a similar change to test it out, I'll compare with mine and get back to you.

@binji
Copy link
Member

binji commented Jun 16, 2016

I don't know if it's appropriate to add those tests to the test suite, it might be interesting to look into this.

Nah, most of those tests aren't very exhaustive. They include all the different operators, but don't really test much else about them. The testsuite has (or at least should have) better tests for all this. The only thing we may want to include as well are tests that exercise the JavaScript API. But I imagine that should be added to the official testsuite in some way too.

@binji binji closed this as completed in #89 Jun 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants