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

dcodeIO · 2020-01-19T21:43:10Z

Omits the -fPIC option when building with Emscripten and updates the Docker image.

The buildbot is currently failing with latest-fastcomp Emscripten due to

shared:WARNING: ignoring -fPIC flag when not building with SIDE_MODULE or MAIN_MODULE
shared:ERROR: treating warnings as errors (-Werror)

so this PR excludes the flag when building the JS or Wasm versions. I guess CI didn't catch this because it uses an outdated docker image.

seen this happen with upstream. to be sure.

kripken

PIC part lgtm, but some questions about the rest.

kripken · 2020-01-21T19:23:12Z

.travis.yml

        - docker exec -it emscripten bash ./travis-emcc-tests.sh
+        - kill $PID
+        # printing something every 5 minutes prevents Travis from timing out.


have you seen such timeouts?

Yes, compiling Binaryen.js used to time out when I first tried the emscripten-upstream instead of the emscripten-fastcomp image (not here, but in my branch while investigating why upstream builds are broken), because compilation didn't print to console for more than 10 minutes, which Travis considers a stalled build, cancelling it. My expectation is that we will run into this sooner or later, so I thought why not add it.

kripken · 2020-01-21T19:24:23Z

.travis.yml

@@ -107,11 +107,15 @@ 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 :(


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?

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.

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)

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.

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.

Oh I see sdk-incoming is also fastcomp?

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

sbc100 · 2020-01-21T19:53:43Z

CMakeLists.txt

@@ -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")


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

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.

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.

sbc100 · 2020-01-21T19:54:55Z

Agree the timeout bash timeout thing doesn't belong as part of this change. Hopefully we can find a solution to that issue which isn't quite so hacky.. doesn't travis maybe have way to extend the timeout?

dcodeIO · 2020-01-21T21:29:15Z

Removed the timeout thing for now but tried to elaborate a bit why I considered it a good idea in the comments above. If you like, I can make it a separate PR.

dcodeIO · 2020-01-24T14:16:32Z

Friendly ping :)

sbc100

In that case LGTM

sbc100 · 2020-01-24T20:13:52Z

.travis.yml

@@ -107,11 +107,15 @@ 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 :(


Oh I see sdk-incoming is also fastcomp?

dcodeIO added 2 commits January 19, 2020 22:41

Fix warning (treated as error) when building with Emscripten

0ad1ab3

try latest-upstream

0d1e58f

dcodeIO mentioned this pull request Jan 19, 2020

Strip -fPIC compiler flag during compilation emscripten-core/emscripten#10209

Merged

dcodeIO added 2 commits January 20, 2020 00:38

use latest-fastcomp

7663100

avoid timing out

c1a1609

seen this happen with upstream. to be sure.

kripken reviewed Jan 21, 2020

View reviewed changes

sbc100 reviewed Jan 21, 2020

View reviewed changes

undo the timeout thing

c228ba8

sbc100 approved these changes Jan 24, 2020

View reviewed changes

sbc100 merged commit 090a405 into WebAssembly:master Jan 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dcodeIO commented Jan 19, 2020 •

edited

Loading

kripken left a comment

kripken Jan 21, 2020

dcodeIO Jan 21, 2020

kripken Jan 21, 2020

dcodeIO Jan 21, 2020 •

edited

Loading

dcodeIO Jan 21, 2020 •

edited

Loading

sbc100 Jan 24, 2020

dcodeIO Jan 24, 2020

sbc100 Jan 24, 2020

dcodeIO Jan 24, 2020

sbc100 Jan 21, 2020

dcodeIO Jan 21, 2020

sbc100 Jan 21, 2020

sbc100 commented Jan 21, 2020

dcodeIO commented Jan 21, 2020 •

edited

Loading

dcodeIO commented Jan 24, 2020

sbc100 left a comment

sbc100 Jan 24, 2020

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

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

Conversation

dcodeIO commented Jan 19, 2020 • edited Loading

kripken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcodeIO Jan 21, 2020 • edited Loading

Choose a reason for hiding this comment

dcodeIO Jan 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 commented Jan 21, 2020

dcodeIO commented Jan 21, 2020 • edited Loading

dcodeIO commented Jan 24, 2020

sbc100 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcodeIO commented Jan 19, 2020 •

edited

Loading

dcodeIO Jan 21, 2020 •

edited

Loading

dcodeIO Jan 21, 2020 •

edited

Loading

dcodeIO commented Jan 21, 2020 •

edited

Loading