Skip to content

Fix warning (treated as error) when building with Emscripten #2605

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jan 28, 2020
Merged

Fix warning (treated as error) when building with Emscripten #2605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0ad1ab3
Fix warning (treated as error) when building with Emscripten
dcodeIO Jan 19, 2020
0d1e58f
try latest-upstream
dcodeIO Jan 19, 2020
7663100
use latest-fastcomp
dcodeIO Jan 19, 2020
c1a1609
avoid timing out
dcodeIO Jan 20, 2020
c228ba8
undo the timeout thing
dcodeIO Jan 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,8 @@ jobs:
services:
- docker
before_install:
- docker run -dit --name emscripten -v $(pwd):/src trzeci/emscripten:sdk-incoming-64bit bash
# TODO: Emscripten upstream produces a broken build in CI environments only :(
Copy link
Member

Choose a reason for hiding this comment

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

Is this fixing an actual issue on this CI? (If so why don't tests fail?)

If it's trying to prevent an issue we only see on other CI, is that necessary?

Copy link
Contributor Author

@dcodeIO dcodeIO Jan 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Edit: See comment below first.

This is a common thing on Travis, which cancels jobs when it doesn't see console output for 10 minutes, usually affecting long-running build tasks like this one (also see comment above). They have a bash function named travis_wait for this, but the last time I checked it suppressed output while the task executes so one can't watch the build. Adding this doesn't seem strictly necessary based on the latest test runs, but is very likely to prevent issues in the future.

Copy link
Contributor Author

@dcodeIO dcodeIO Jan 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Just noticed that your question was about something different. Tests do indeed fail when using the emscripten-upstream image, but it works when using the legacy emscripten-fastcomp image (emscripten:sdk-incoming-64bit is an outdated variant of fastcomp not updated anymore). This is happening since the first day Emscripten switched to upstream by default and I started testing it, so I thought a TODO would be appropriate to raise awareness. This is a very odd problem in that

  • Affects only upstream, not fastcomp
  • It compiles fine, but the output is broken (hits assertions)
  • It only happens on CI (both Travis and GitHub Actions)
  • I can't reproduce it locally
  • It does not affect debug builds
  • It affects any release build (without closure etc. etc.)
  • Apparently isolated to building Binaryen.js
  • Happens with both the Docker image and just Emsdk

(At least the last time I checked)

Copy link
Member

Choose a reason for hiding this comment

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

This sounds worth investigating. The fact that you can't repro it locally is very strange, especially since using the docker image should produce identical results.

I guess I'm OK landing this if its urgent but I'd rather try to fix instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR is unrelated to the upstream breakage, though, it only updates the outdated fastcomp image to a recent fastcomp image (both work) and adds the TODO comment.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see sdk-incoming is also fastcomp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I believe that's Emscripten before upstream became the default, and they then switched to the other naming.

- docker run -dit --name emscripten -v $(pwd):/src trzeci/emscripten-fastcomp bash
script:
# run binaryen.js tests before and after building, so we see if the bundled
# version is good too
Expand Down
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ else()
if(WIN32)
add_compile_flag("-D_GNU_SOURCE")
add_link_flag("-Wl,--stack,8388608")
else()
elseif(NOT EMSCRIPTEN)
add_compile_flag("-fPIC")
Copy link
Member

Choose a reason for hiding this comment

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

What is this flag even doing here in the first place? CMake should be adding this automatically when it knows its building a DLL, no? I'm OK with landing this part of the change for now, but would like to look into removing this line completely

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might this have something to do with this warning?

CMake Warning (dev) at CMakeLists.txt:272 (add_library):
  ADD_LIBRARY called with SHARED option but the target platform does not
  support dynamic linking.  Building a STATIC library instead.  This may lead
  to problems.

Copy link
Member

Choose a reason for hiding this comment

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

I think we not be trying to build with SHARED using emscripten, as the warning says.

Other platforms can use SHARED but the emscripten build should only use STATIC.

endif()
add_debug_compile_flag("-O0")
Expand Down