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

Prepare for creating wasm builds #32

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

Senyoret1
Copy link
Contributor

Changes:

  • Now it is possible to compile a wasm version of the library. The code was created for Go v1.12.x. it does not work with Go v1.11.x and will probably not work with Go v1.13.x, as the wasm compiler is still getting breaking changes.

  • A version of the cipher test suite for the wasm version was added.

  • The file wasm_exec.js is https://github.com/golang/go/blob/master/misc/wasm/wasm_exec.js for Go v1.12.

  • The possibility of using GopherJS is still active, just in case. However, the wasm version is much faster, so the GopherJS version should be removed if we do not find problems.

I will check this PR again latter, just in case.

@Senyoret1
Copy link
Contributor Author

I checked the PR again and found nothing relevant, I just changed parts of the documentation and added comments to the Go file. I also made changes to the Travis config, and deactivated the checks for the GopherJS version in Travis (as those need Go v1.10 and the wasm build needs Go v1.12). However, it is still necessary to make changes to the Go linters config.

@Senyoret1
Copy link
Contributor Author

With the lastest commit the wasm test does not only includes the cipher test suite (which was translated from Go to TypeScript), but also compiles the test files found in src/cipher/sec256k1-go/ and src/cipher/sec256k1-go/secp256k1-go2/ to wasm files and runs them (this time directly compiled from Go to wasm, not translated to TypeScript).

Currently the Go wasm builds don’t return the exit code to the calling js code without making modifications, but as a warning is shown in the console, the tests code detect that warning for failing.

@gz-c
Copy link
Member

gz-c commented Jul 12, 2019

Merge conflicts

@Senyoret1
Copy link
Contributor Author

In the last 2 commits, due to #34, the following changes were made:

  • The conflects were solved.

  • The code related to the wasm version was updated.

  • The code for running a wasm version of the test on /vendor/github.com/skycoin/skycoin/src/cipher/secp256k1-go and /vendor/github.com/skycoin/skycoin/src/cipher/secp256k1-go/secp256k1-go2 was rewritten, as after updating the Skycoin cipher library the previous method stopped working due to timeouts.

  • The tests on the vendor folder were updated.

  • Small changes to the readme.

@Senyoret1
Copy link
Contributor Author

@gz-c The last 2 commits solve the remaining problems with the linter and the tests. However, for some reason Travis made this build: https://travis-ci.com/skycoin/skycoin-lite/builds/119038575 but did not report the result here.

@Senyoret1
Copy link
Contributor Author

Nevermind, Travis just taked a lot of time before reporting the result on Github

@@ -31,5 +31,4 @@ required = ["github.com/gopherjs/gopherjs"]
name = "github.com/skycoin/skycoin"

[prune]
go-tests = true
Copy link
Member

Choose a reason for hiding this comment

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

I believe this defaults to true, you may want to be explicit go-tests = false and leave a comment why (for the wasm tests)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding go-tests = false makes dep ensure say root prune options must be omitted instead of being set to false

cd js && npm run test-extensive

test-suite-ts-wasm: ## Run the ts version of the cipher test suite for wasm and additional tests
cd vendor/github.com/skycoin/skycoin/src/cipher/secp256k1-go && GOOS=js GOARCH=wasm go test -c -o test.wasm
Copy link
Member

Choose a reason for hiding this comment

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

Can we run these tests for cipher/ as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, no problem. Done in the lastest commit

@Senyoret1
Copy link
Contributor Author

The last commit includes the following changes:

  • Now make test-suite-ts-wasm also runs the test found inside /github.com/skycoin/skycoin/src/cipher.

  • The code on cipher-wasm-internal.js was modified to make it easier to run additional Go tests.

  • The package github.com/stretchr/testify/require was added to the required list in Gopkg.toml, as it is needed for the new tests, and the vendor folder was updated.

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

Successfully merging this pull request may close these issues.

2 participants