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

x/build: add js/wasm Chrome/Firefox capable trybot #26051

Open
myitcv opened this issue Jun 25, 2018 · 23 comments
Open

x/build: add js/wasm Chrome/Firefox capable trybot #26051

myitcv opened this issue Jun 25, 2018 · 23 comments
Labels
arch-wasm WebAssembly issues Builders x/build issues (builders, bots, dashboards) FeatureRequest NeedsFix The path to resolution is known, but the work has not been done. new-builder
Milestone

Comments

@myitcv
Copy link
Member

myitcv commented Jun 25, 2018

Follow on from #26015 (comment)

With a Chrome and/or Firefox capable trybot we'd be able to do more interesting WASM tests in a headless browser-based JS VM, and so have access to the DOM etc.

This issue is to decide whether or not such a trybot would be useful and therefore worth the effort.

@myitcv myitcv added Builders x/build issues (builders, bots, dashboards) NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. arch-wasm WebAssembly issues labels Jun 25, 2018
@gopherbot gopherbot added this to the Unreleased milestone Jun 25, 2018
@bradfitz
Copy link
Contributor

@neelance, would a browser-based builder be helpful for anything? I imagine yes, at least for DOM interaction.

@neelance
Copy link
Member

Hmm... I don't expect the core packages to do much DOM interaction. That should all go into libraries.

@bradfitz
Copy link
Contributor

In any case, since 99% of users will use Go's wasm support to target browsers, testing in browsers instead of Node still seems like it makes sense, even if v8 is the same in at least Chrome's case.

@agnivade
Copy link
Contributor

agnivade commented Jul 4, 2018

This looked interesting, so I took a stab at this. I looked into headless Chrome and used the puppeteer library to interact with the browser through an API.

It took some fighting to figure out the kinks but now I have been able to run the tests completely using puppeteer. So all wasm tests are running completely inside the browser ! I am yet to run all the tests. But encoding/, strings/ run fine :)

I have 2 concerns

  • The script needs the puppeteer library installed. So this needs the usual npm install/yarn add business. Adding a package.json would be ugly.
  • And during go test, by default it looks for go_$GOOS_$GOARCH, so do I need to put this in a different folder ? Or do we set the -exec parameter manually ? The wasm_exec.js file is shared, so I would prefer both to remain in the same folder.

Thoughts ?

I can put up a DNS CL for people to play around, but just wanted to get the conversation going here.

@agnivade agnivade added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jul 4, 2018
@myitcv
Copy link
Member Author

myitcv commented Jul 4, 2018

I looked into headless Chrome and used the puppeteer library to interact with the browser through an API.

Interesting @agnivade; for running GopherJS tests in browser with https://github.com/myitcv/gjbt (very alpha) I went with https://godoc.org/github.com/sclevine/agouti, on the basis it can drive any browser (although for now gjbt is hardcoded to work with Chrome). agouti requires chromedriver to work with Chrome, but no Javascript package dependency.

The script needs the puppeteer library installed. So this needs the usual npm install/yarn add business. Adding a package.json would be ugly.

Assuming we need an npm package, which might not be the case, there will still be a requirement to have Chrome/Firefox etc available on such a builder. So is this such an issue?

On the plus side, if we can get a good, robust setup in place it will, like misc/wasm/wasm_exec.js, be usable by library authors etc who also want to run their tests in browser (selfishly I'd be one such user).

Clearly this doesn't require that we ship something with the core Go distribution, but this brings us back to @bradfitz's comment in #26051 (comment)

I can put up a DNS CL for people to play around, but just wanted to get the conversation going here.

Would be interesting to have a look through, thanks.

@agnivade
Copy link
Contributor

agnivade commented Jul 5, 2018

Ah nice, a Go solution is certainly preferred by me. Since we would like to have a common interface to drive any browser, how about https://github.com/tebeka/selenium ? Have you had any experience with it ? I was wondering if it is better than agouti or are they both kinda the same.

Assuming we need an npm package, which might not be the case, there will still be a requirement to have Chrome/Firefox etc available on such a builder. So is this such an issue?

It just adds the headache of maintaining an extra package.json. Otherwise, it is fine by me. In any case, I am going to investigate the Go based approaches first.

@myitcv
Copy link
Member Author

myitcv commented Jul 5, 2018

how about https://github.com/tebeka/selenium ? Have you had any experience with it ?

Unfortunately not.

@bradfitz
Copy link
Contributor

Presumably a browser-based trybot would let us test the net/http Fetch code and catch regressions like: https://go-review.googlesource.com/c/go/+/123537

@agnivade
Copy link
Contributor

I will look into a pure Go solution this weekend and update here.

From the looks of it, we need to serve the index.html, wasm.exec.js and the target wasm file by spinning up a web server and point the Chrome/Gecko Driver to it. And just capture the console logs.

@myitcv
Copy link
Member Author

myitcv commented Jul 13, 2018

From the looks of it, we need to serve the index.html, wasm.exec.js and the target wasm file by spinning up a web server and point the Chrome/Gecko Driver to it. And just capture the console logs.

If agouti (with Chrome) is anything to go by (used in myitcv.io/gjbt as I mentioned) then you won't need a web server, you can simply "inject" a JavaScript wrapper in a Page that does what you want.

@agnivade
Copy link
Contributor

Yes, but then the the WebAssembly.insantiateStreaming call has a fetch call which cannot read the wasm file from local filesystem from inside the browser. :)

Using puppeteer, I had to stub out fetch with fs.ReadFile. But I dont see that level of flexibility here. Figured that its simple to just spin up a web server.

Open to any other suggestions that you have.

@myitcv
Copy link
Member Author

myitcv commented Jul 13, 2018

Open to any other suggestions that you have.

You're more than likely miles ahead of me here so I'll defer to you 😄

@agnivade
Copy link
Contributor

agnivade commented Jul 14, 2018

I have managed to get a beta version here - https://github.com/agnivade/wasmbrowsertest.

Unfortunately, there is no FF support because the log API is not supported by geckodriver. I was very surprised this wasn't properly documented anywhere.

Overall, the process wasn't as trivial as I thought it would be. Since the driving process is now in Go, the interop between Go and JS is now much harder, than Node and JS. 2 things were particularly tricky -

  1. Passing the process.argv[1:] from the Go process to inside the browser.
  2. Capturing the go.exit() function and responding to that in the Go process.
    (EDIT: I had to add a new variable go.exitCode in wasm_exec.js to be able to check that in the Go process.
this.exitCode = 0;
this.exit = (code) => {
	if (code !== 0) {
		console.warn("exit code:", code);
		this.exitCode = code;
	}
};

)

These are simply replaced directly in the node-based go_js_wasm_exec. Hence there was no issue there.

Anyways, tests are running now.

The only blocking issue is that some tests are trying to open files. (like archive/tar, text/tabwriter). Those tests fail with
error parsing files: open testdata/file1.tmpl: not implemented on js",
"panic: syscall/js: Value.Call: property lstatSync is not a function, got undefined [recovered]".

But those pass in nodeJS because obviously readfile is supported from within node. Any idea on how do we tackle this @neelance ?

Please feel free to clone the repo and try it out. I apologize for the messy code. I initially thought of using rakyll/statik to bundle everything inside the binary to avoid passing the asset_folder, but then realized I have to templatize the wasm file name inside the html. Probably there are better solutions, but its late and my brain will shut down soon. 😩

@myitcv
Copy link
Member Author

myitcv commented Aug 1, 2018

Would also help to diagnose/repro #26748

@agnivade
Copy link
Contributor

agnivade commented Aug 4, 2018

Looked into this a bit more. There is a non-standard filesystem API which is implemented only by Chrome. https://developer.mozilla.org/en-US/docs/Web/API/Window/requestFileSystem. Using this, we can read/write files from the browser. So, if we implement the global.fs object inside wasm_exec.js to fill out all the filesystem calls, then it can work.

But unfortunately, the sync versions of those APIs, which is what we need to use to call from Go space, do not work. The page itself says This specification is more or less abandonned, failing to get significant traction. and the w3c page mentions Work on this document has been discontinued and it should not be referenced or used as a basis for implementation..

So unless we can use the async APIs, it does not seem possible to read/write files from browser. The syscall/fs_js.go file uses sync APIs to interact with nodeJS. We would need to change that to be async, which seems pretty cumbersome to me, because there will be intricate callbacks to respond to browser functions.

So as it stands -

  • Go uses sync fs APIs.
  • Browser allows only async fs APIs.
  • Change Go to use async APIs ? Hard.
  • Somehow ignore filesystem errors when running tests in browser ? Not sure how. But that is the closest we can get to a working solution.

@neelance

@agnivade
Copy link
Contributor

ping @neelance - Any thoughts on this ?

@gopherbot
Copy link

Change https://golang.org/cl/137236 mentions this issue: syscall: use asynchronous operations on js/wasm

@neelance
Copy link
Member

@agnivade The above CL makes it use the async APIs. Hope this helps.

@agnivade
Copy link
Contributor

Awesome, Thank you ! I will check with this and update.

gopherbot pushed a commit that referenced this issue Oct 4, 2018
This commit makes syscall on js/wasm use the asynchronous variants
of functions in Node.js' fs module. This enables concurrency
and allows the API of the fs module to be implemented with an
alternative backend that only supports asynchronous operations.

Updates #26051.

Change-Id: Ibe1dcc988469fc11c3b8d8d49de439c12ddaafce
Reviewed-on: https://go-review.googlesource.com/c/137236
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@agnivade
Copy link
Contributor

agnivade commented Nov 7, 2018

The FS APIs turned out to be much more unstable than I thought. Even a simple file read is throwing a Error: NotFoundError: A requested file or directory could not be found at the time an operation was processed.

If I try to write to a new file using {create: true}, it doesn't throw an error but I cannot see any new file created.

This is the read file code that I am trying. I have tried with keeping index.html in the webserver root as well as /, to no avail.

window.requestFileSystem  = window.requestFileSystem || window.webkitRequestFileSystem;
function errorCb(e) {
	var msg = '';
	console.error('Error: ' + e);
}
function successCb(fs) {
fs.root.getFile('index.html', {}, function(fileEntry) {
		console.log(fileEntry)
  }, errorCb);
}
navigator.webkitPersistentStorage.requestQuota(10*1024*1024, function(grantedBytes) {
	window.requestFileSystem(window.PERSISTENT, grantedBytes, successCb, errorCb);
}, function(e) {
  console.log('Error', e);
});

My Chrome version is 69 along with (--unlimited-quota-for-files --allow-file-access-from-files). Anybody is welcome to investigate.

Further notes: even if this works, I don't see any API to retrieve/write to a fd from a file, which is required for file operations to work.

@agnivade
Copy link
Contributor

Native filesystem APIs are coming back. https://blog.chromium.org/2019/09/chrome-78-beta-new-houdini-api-native.html. All hope is not lost.

@neelance
Copy link
Member

Any progress on this issue?

@agnivade
Copy link
Contributor

Actually, we don't need the filesystem APIs at all for this. It should be possible to achieve this by extending wasmbrowsertest and a little bit of extra customization to wasm_exec, to behave like a pass-through layer which forwards all file-system calls to the webserver, which then makes the actual calls and relay it back to the browser. That should make for more robust code and avoid some puppeteer edge-cases which I have been following up with the Chrome team without much success.

I don't have enough free time these days to work on wasm stuff. But it is not that complicated if someone wants to send a PR. Otherwise I'll get to it whenever I have some time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-wasm WebAssembly issues Builders x/build issues (builders, bots, dashboards) FeatureRequest NeedsFix The path to resolution is known, but the work has not been done. new-builder
Projects
None yet
Development

No branches or pull requests

5 participants