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

Roll back to Emscripten 1.38.48 #550

Merged
merged 1 commit into from
Oct 29, 2019

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Oct 29, 2019

Recently released 1.39.0 seems to have a regression than breaks our build for WasmThemis. Let's roll back to the previous version until the issue is examined and resolved.

Recently released 1.39.0 seems to have a regression than breaks our
build. Let's roll back to the previous version until the issue is
examined and resolved.
@ilammy ilammy added infrastructure Automated building and packaging W-WasmThemis 🌐 Wrapper: WasmThemis, JavaScript API, npm packages labels Oct 29, 2019
Copy link
Contributor

@vixentael vixentael left a comment

Choose a reason for hiding this comment

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

I actually like it more when we link to specific version rather then see building errors unexpectedly

@ilammy
Copy link
Collaborator Author

ilammy commented Oct 29, 2019

Well, arguably, breaking early is the point of having a CI build so it works as intended. We definitely can support building only with a particular version of the toolchain, but that needs to be spelled out for the users. Currently we recommend the defaults, and Emscripten recommends using the latest stable version by default (that’s what gets installed when using latest).

I don’t really know now who’s at fault for breaking the build. It could well be that our code triggers some undefined behavior which just happened to work up until now, because Hello world works for me with the 1.39.0 toolchain, it’s only Soter tests that are breaking.

@Lagovas
Copy link
Collaborator

Lagovas commented Oct 29, 2019

we can try to test with the latest version of dependency which will only warn about problems and don't block CI plus with specific version which will be required to successful pass

@ilammy
Copy link
Collaborator Author

ilammy commented Oct 29, 2019

We don’t explicitly pin any other toolchain and I don’t think we should (CircleCI has limited build capabilities and we can’t realistically test everything).

I’d propose to leave it like this at least until the next release of Emscripten. If resolving the original issue is going to take a while then we should probably track both versions. But I don’t really want to have a CI build that’s going to be a source of warnings for half a year. We can track the Emscripten issue to see whether it’s resolved, we don’t need to check that in every our build.

@ilammy ilammy merged commit c2fcdc5 into cossacklabs:master Oct 29, 2019
@ilammy ilammy deleted the rollback-emscripten branch October 29, 2019 15:02
@ilammy
Copy link
Collaborator Author

ilammy commented Oct 29, 2019

After fiddling with our code it seems that the issue is with Emscripten, so I’m merging this PR and we’ll be using 1.38.48 until the regression goes away. The issue has been reported on the Emscripten mailing list, and if that’s confirmed I’ll report it to their bug tracker as well.

ilammy added a commit that referenced this pull request Dec 16, 2019
Recently released 1.39.0 seems to have a regression than breaks our
build. Let's roll back to the previous version until the issue is
examined and resolved.

(cherry picked from commit c2fcdc5)

A-a-and we also need this so that CircleCI uses correct Emscripten
flavor for running tests.
@ilammy ilammy mentioned this pull request Dec 16, 2019
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Automated building and packaging W-WasmThemis 🌐 Wrapper: WasmThemis, JavaScript API, npm packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants