Fix for `-lGL` + `AUTO_NATIVE_LIBRARIES=0` #14337

sbc100 · 2021-06-01T18:05:49Z

When calling map_to_js_libs('GL') we were mapping this to the JS
libraries, and not also including the native libgl library. Normally
this doesn't matter since libgl is included by default, but not
when AUTO_NATIVE_LIBRARIES=0 is used.

This also happens to fix an old bug (#10171) where we would look on the
file system for libGL.a before looking in map_to_js_libs.

Fixes: #10171
Fixes: #14335

kripken · 2021-06-01T20:56:55Z

tests/test_other.py

+
+  def test_explict_gl_linking(self):
+    # ensure libgl exists
+    self.run_process([EMBUILDER, 'build', 'libgl'])


what happens if embuilder isn't run here? wouldn't we build the library automatically?

No. As of today if you use NO_AUTO_NATIVE_LIBRARIES there is no way to trigger the auto-building of the affected libraries (libgl, libal, libhtml5). I think we can fix that as part of #14341

(this PR doesn't change that)

Force pushes make it hard to see, but is the addition of NO_AUTO_NATIVE_LIBRARIES new? Or did I just miss it before?

Regardless, why is it here? Wouldn't the test be shorter without it, as we could delete the above line?

If the reasoning is that NO_AUTO_NATIVE_LIBRARIES gives us an error if we try to use a missing library, and the point of the test is to check for that error, how does this avoid the library already existing because it was built by the test harness (or another test) before?

NO_AUTO_NATIVE_LIBRARIES was always there.

And yes I can not remove the line above your are right. Sorry about that.

The reason that NO_AUTO_NATIVE_LIBRARIES is needed is because -lGL was previously failing in that mode (NO_AUTO_NATIVE_LIBRARIES is enabled in strict mode which is more common than the almost unknown/unused NO_AUTO_NATIVE_LIBRARIES).

Before:

-sSTRING -lGL -> link failure because to interpret -lGL as being just about JS libraries

After:

-sSTRICT -lGL -> link success because we find and use the native library.

tools/building.py

sbc100 · 2021-06-03T01:45:54Z

Removed hacky comment and workaround. This should be good to go now.

kripken · 2021-06-03T16:19:22Z

tools/building.py

@@ -1339,7 +1339,6 @@ def is_wasm_dylib(filename):
 def map_to_js_libs(library_name):
  # Some native libraries are implemented in Emscripten as system side JS libraries
  library_map = {
-    'c': [],


why is it safe to remove this?

Its not just safe, but essential if we want to continue to allow folks to do -lc on the command line.

The reason is that previously we would search for libc.a on the filesystem before looking in this map. With this change we call map_to_js_libs first, so this would cause -lc to be ignored, which we don't want.

In practice anyone who passes -lc today might not get the correct version of libc, but that was true both before and after this change. To fix that issue we have #14355, although that issue is completely separate to this change.

I am having a hard time understanding this. How is, say, libc (which is removed here) different from libdl (which is not)?

libdl and libm don't exist under musl, since they all get rolled into libc. So the libdl and libm cases here are different because we really do want them to be no-ops.

We are pretending to have -ldl support for compat with glibc

In that case, how about comments on each one explaining the issues? Sorry, but this is all quite unclear to me. Maybe this is just part of the codebase I am unfamiliar with.

I agree.. that would be more clear.

After rebasing on top of #14382 perhaps this is more clear now?

tools/building.py

Split out from #14337

kripken

Looks a lot better! But still, I don't get the test, see comment...

kripken · 2021-06-04T22:48:15Z

tests/test_other.py

+
+  def test_explict_gl_linking(self):
+    # ensure libgl exists
+    self.run_process([EMBUILDER, 'build', 'libgl'])


Force pushes make it hard to see, but is the addition of NO_AUTO_NATIVE_LIBRARIES new? Or did I just miss it before?

Regardless, why is it here? Wouldn't the test be shorter without it, as we could delete the above line?

If the reasoning is that NO_AUTO_NATIVE_LIBRARIES gives us an error if we try to use a missing library, and the point of the test is to check for that error, how does this avoid the library already existing because it was built by the test harness (or another test) before?

Preparation for #14337.

sbc100 · 2021-06-04T23:27:25Z

Rebased onto refactor #14384

tests/test_other.py

kripken · 2021-06-04T23:32:16Z

Side note, the github UI seems to be hitting a bug here... you responded in detail to my previous comment (thanks!) but it does not show up on this page, nor on the file contents page. It makes sense not to be there, since the file was updated around the comment. But it's like your response is gone, with even the "conversations" tab not allowing access to it... I had to find the email to me to re-read it.

Preparation for #14337.

When calling `map_to_js_libs('GL')` we were mapping this to the JS libraries, and not also including the native `libgl` library. Normally this doesn't matter since `libgl` is included by default, but not when `AUTO_NATIVE_LIBRARIES=0` is used. This also happens to fix an old bug (#10171) where we would look on the file system for `libGL.a` before looking in `map_to_js_libs`. Fixes: #10171 Fixes: #14335

sbc100 mentioned this pull request Jun 1, 2021

undefined symbol: eglGetProcAddress even with -lEGL flag #14335

Closed

sbc100 requested a review from kripken June 1, 2021 18:07

sbc100 force-pushed the fix_libgl branch 4 times, most recently from a608686 to b89154e Compare June 2, 2021 01:03

kripken reviewed Jun 2, 2021

View reviewed changes

sbc100 force-pushed the fix_libgl branch 5 times, most recently from a91b043 to c662994 Compare June 3, 2021 01:45

sbc100 requested a review from kripken June 3, 2021 01:49

sbc100 force-pushed the fix_libgl branch from c662994 to 6726910 Compare June 3, 2021 01:51

sbc100 enabled auto-merge (squash) June 3, 2021 03:19

kripken reviewed Jun 3, 2021

View reviewed changes

sbc100 force-pushed the fix_libgl branch 2 times, most recently from 982bc5e to 483091e Compare June 4, 2021 03:19

sbc100 added a commit that referenced this pull request Jun 4, 2021

Seperate out handling of dummy glibc libraries. NFC

4c65d91

Split out from #14337

sbc100 mentioned this pull request Jun 4, 2021

Separate out handling of dummy glibc libraries. NFC #14380

Closed

sbc100 added a commit that referenced this pull request Jun 4, 2021

Seperate out handling of dummy glibc libraries. NFC

bc2f472

Split out from #14337

sbc100 force-pushed the fix_libgl branch 2 times, most recently from 690236f to 7b3716a Compare June 4, 2021 21:09

sbc100 disabled auto-merge June 4, 2021 21:09

sbc100 force-pushed the fix_libgl branch 2 times, most recently from 17240e5 to c77b6c6 Compare June 4, 2021 22:41

kripken reviewed Jun 4, 2021

View reviewed changes

sbc100 added a commit that referenced this pull request Jun 4, 2021

Minor refactor of process_libraires. NFC

916a7a6

Preparation for #14337.

sbc100 mentioned this pull request Jun 4, 2021

Minor refactor of process_libraries. NFC #14384

Merged

sbc100 force-pushed the fix_libgl branch from c77b6c6 to c104836 Compare June 4, 2021 23:27

sbc100 changed the base branch from main to refactor_process_libraries June 4, 2021 23:27

kripken approved these changes Jun 4, 2021

View reviewed changes

tests/test_other.py Show resolved Hide resolved

Base automatically changed from refactor_process_libraries to main June 5, 2021 00:25

sbc100 added a commit that referenced this pull request Jun 5, 2021

Minor refactor of process_libraries. NFC (#14384)

c0e1a98

Preparation for #14337.

sbc100 force-pushed the fix_libgl branch from c104836 to c9b18c7 Compare June 5, 2021 00:27

sbc100 enabled auto-merge (squash) June 5, 2021 00:27

sbc100 merged commit 0ca7b3a into main Jun 7, 2021

sbc100 deleted the fix_libgl branch June 7, 2021 00:49

sbc100 mentioned this pull request Jun 9, 2021

Linking issues when using Emscripten + Rust + pthread #14419

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for `-lGL` + `AUTO_NATIVE_LIBRARIES=0` #14337

Fix for `-lGL` + `AUTO_NATIVE_LIBRARIES=0` #14337

sbc100 commented Jun 1, 2021 •

edited

Loading

kripken Jun 1, 2021

sbc100 Jun 2, 2021

sbc100 Jun 2, 2021

kripken Jun 4, 2021

sbc100 Jun 4, 2021

sbc100 commented Jun 3, 2021

kripken Jun 3, 2021

sbc100 Jun 3, 2021

kripken Jun 3, 2021

sbc100 Jun 3, 2021

sbc100 Jun 3, 2021

kripken Jun 4, 2021

sbc100 Jun 4, 2021

sbc100 Jun 4, 2021

kripken left a comment

kripken Jun 4, 2021

sbc100 commented Jun 4, 2021

kripken commented Jun 4, 2021

Fix for -lGL + AUTO_NATIVE_LIBRARIES=0 #14337

Fix for -lGL + AUTO_NATIVE_LIBRARIES=0 #14337

Conversation

sbc100 commented Jun 1, 2021 • 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

sbc100 commented Jun 3, 2021

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

kripken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 commented Jun 4, 2021

kripken commented Jun 4, 2021

Fix for `-lGL` + `AUTO_NATIVE_LIBRARIES=0` #14337

Fix for `-lGL` + `AUTO_NATIVE_LIBRARIES=0` #14337

sbc100 commented Jun 1, 2021 •

edited

Loading