Skip to content

Enable real dynamic linking with -shared by default#25930

Open
sbc100 wants to merge 3 commits into
emscripten-core:mainfrom
sbc100:shared_flag_part2
Open

Enable real dynamic linking with -shared by default#25930
sbc100 wants to merge 3 commits into
emscripten-core:mainfrom
sbc100:shared_flag_part2

Conversation

@sbc100
Copy link
Copy Markdown
Collaborator

@sbc100 sbc100 commented Dec 11, 2025

This change essentially disables FAKE_DYLIBS by default, which in
turn means -shared will now produce dynamic libraries by default.

If you want the old behaviour you now need -sFAKE_DYLIBS.

Comment thread cmake/Modules/Platform/Emscripten.cmake Outdated
@sbc100 sbc100 force-pushed the shared_flag_part2 branch 2 times, most recently from 6e4f78f to 3b0addc Compare December 11, 2025 00:49
@sbc100 sbc100 requested review from dschuff and kripken December 11, 2025 00:50
Comment thread site/source/docs/tools_reference/settings_reference.rst Outdated
Comment thread tools/building.py
Copy link
Copy Markdown
Member

@dschuff dschuff left a comment

Choose a reason for hiding this comment

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

I wonder what would happen if we tested this with Binaryen. it doesn't seem prepared to force the Emscripten build to be static (assuming that's the behavior we actually want).

Comment thread test/test_core.py Outdated
@sbc100 sbc100 force-pushed the shared_flag_part2 branch from 3b0addc to 2a98976 Compare May 12, 2026 01:15
@sbc100 sbc100 changed the title Disable FAKE_DYLIBS by default Enable real dynamic linking by default May 12, 2026
@sbc100 sbc100 force-pushed the shared_flag_part2 branch 3 times, most recently from 3b9f050 to d2bc4e4 Compare May 12, 2026 04:13
Comment thread cmake/Modules/Platform/Emscripten.cmake
@sbc100 sbc100 force-pushed the shared_flag_part2 branch 2 times, most recently from dbf81cc to 2a061bd Compare May 14, 2026 01:07
sbc100 added a commit that referenced this pull request May 14, 2026
This test was first added in c735bb6. Hopefully these changes make it
more obvious what is being tested, and also test more commonly used
flags (`-shared` vs `-r`).

Split out from #25930.
@sbc100 sbc100 force-pushed the shared_flag_part2 branch 4 times, most recently from d43b884 to 19b4b57 Compare May 15, 2026 19:07
@sbc100
Copy link
Copy Markdown
Collaborator Author

sbc100 commented May 15, 2026

I wonder what would happen if we tested this with Binaryen. it doesn't seem prepared to force the Emscripten build to be static (assuming that's the behavior we actually want).

I actually ended up leaving the cmake TARGET_SUPPORTS_SHARED_LIBS setting as False until we resolve https://gitlab.kitware.com/cmake/cmake/-/work_items/27240

@sbc100
Copy link
Copy Markdown
Collaborator Author

sbc100 commented May 15, 2026

OK, I think this change is in pretty good shape now

@sbc100 sbc100 force-pushed the shared_flag_part2 branch from 19b4b57 to 28ec96d Compare May 15, 2026 19:31
@sbc100 sbc100 changed the title Enable real dynamic linking by default Enable real dynamic linking with -shared by default May 15, 2026
@sbc100 sbc100 force-pushed the shared_flag_part2 branch from 28ec96d to 6b35408 Compare May 15, 2026 21:40
@sbc100
Copy link
Copy Markdown
Collaborator Author

sbc100 commented May 16, 2026

@kripken @dschuff this change is pretty small now. No codesize changes anymore.

@sbc100 sbc100 requested review from dschuff and kripken May 16, 2026 00:48
sbc100 added a commit to sbc100/emscripten that referenced this pull request May 18, 2026
In particular, mention the `FAKE_DYLIBS` setting.

This is important especially since we are about the change the default
behavior.  See emscripten-core#25930
@sbc100
Copy link
Copy Markdown
Collaborator Author

sbc100 commented May 18, 2026

Given that we have a sensible behavior for dynamic linking, I think that having the -shared flag enable that behavior is a more sensible default than having the -shared flag do nothing. (Note that we are not enabling dynamic linking itself by default the way it is on Linux). Speaking of what the behavior is, before we ship this in a release, we should update our documentation with a description of the expected way to use and enable dynamic linking.

Good idea. I started by updated the docs here: #26976.

I can then include another update with this PR when it lands.

sbc100 added a commit to sbc100/emscripten that referenced this pull request May 18, 2026
In particular, mention the `FAKE_DYLIBS` setting.

This is important especially since we are about the change the default
behavior.  See emscripten-core#25930
sbc100 added a commit to sbc100/emscripten that referenced this pull request May 19, 2026
In particular, mention the `FAKE_DYLIBS` setting.

This is important especially since we are about the change the default
behavior.  See emscripten-core#25930
sbc100 added a commit that referenced this pull request May 19, 2026
In particular, mention the `FAKE_DYLIBS` setting.

This is important especially since we are about the change the default
behavior. See #25930
@sbc100 sbc100 force-pushed the shared_flag_part2 branch from 2901ff3 to 524ceee Compare May 19, 2026 22:37
@sbc100
Copy link
Copy Markdown
Collaborator Author

sbc100 commented May 19, 2026

OK, this change now has corresponding update to Dynamic-Linking.rst.

@kripken was I able to persuade you this is a direction worth going in?

@kripken
Copy link
Copy Markdown
Member

kripken commented May 19, 2026

Yes, I'm not opposed to the direction. I do worry about some breakage, but you're probably right that there are few users of this feature, and eventually this might lead to simplifications.

@sbc100
Copy link
Copy Markdown
Collaborator Author

sbc100 commented May 19, 2026

Any last concerns before landing this?

sbc100 added a commit to sbc100/emscripten that referenced this pull request May 20, 2026
Comment thread site/source/docs/compiling/Dynamic-Linking.rst Outdated
Comment thread site/source/docs/compiling/Dynamic-Linking.rst Outdated
Comment thread test/common.py
@sbc100 sbc100 force-pushed the shared_flag_part2 branch from 524ceee to 8c5739b Compare May 20, 2026 17:19
@sbc100 sbc100 enabled auto-merge (squash) May 20, 2026 17:43
Comment thread test/test_core.py Outdated
Comment thread test/test_core.py Outdated
sbc100 added a commit that referenced this pull request May 20, 2026
@sbc100 sbc100 force-pushed the shared_flag_part2 branch 3 times, most recently from a673118 to 8d7ffbb Compare May 20, 2026 20:41
@sbc100 sbc100 requested a review from kripken May 20, 2026 20:42
Comment thread cmake/Modules/Platform/Emscripten.cmake Outdated
Comment thread test/test_core.py Outdated
@sbc100 sbc100 force-pushed the shared_flag_part2 branch from 8d7ffbb to b594e80 Compare May 20, 2026 20:49
This change essentially disables `FAKE_DYLIBS` by default, which in
turn means `-shared` will now produce dynamic libraries by default.

If you want the old behaviour you now need `-sFAKE_DYLIBS`.
@sbc100 sbc100 force-pushed the shared_flag_part2 branch from b594e80 to efe9645 Compare May 20, 2026 20:58
@sbc100
Copy link
Copy Markdown
Collaborator Author

sbc100 commented May 20, 2026

OK, cmake issue is now resolved I believe.

Comment thread test/test_core.py Outdated
Comment thread test/test_other.py Outdated
Comment thread test/test_other.py
Comment thread test/test_other.py
Comment thread test/test_other.py
self.run_process([EMCC, 'libdir/libfile.c', '-sFAKE_DYLIBS', '-shared', '-fPIC', '-o', 'libdir/libfile.so' + lib_suffix])
# Build libother and dynamically link it to libfile
self.run_process([EMCC, '-Llibdir', 'libdir/libother.c'] + link_flags + ['-shared', '-o', 'libdir/libother.so'])
self.run_process([EMCC, '-Llibdir', 'libdir/libother.c'] + link_flags + ['-sFAKE_DYLIBS', '-shared', '-fPIC', '-o', 'libdir/libother.so'])
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here I think we do need a comment on the new flag? I don't see one in this function.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I split out a change to remove this redundant test: #26986

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.

5 participants