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

[Web] Disable UNIX sockets #98954

Merged
merged 2 commits into from
Nov 13, 2024
Merged

[Web] Disable UNIX sockets #98954

merged 2 commits into from
Nov 13, 2024

Conversation

Faless
Copy link
Collaborator

@Faless Faless commented Nov 8, 2024

They are not supported anyway, emscripten has an emulation layer that implements them over WebSocket/WebRTC, which is really surprising for users, and also not very useful since we have proper WebSocket and WebRTC support.

This can make the build smaller, if we also disable the UPNP module (which will otherwise include a third party library referencing "socket" thus forcing emscripten to include the compatibility layer)

Fixes #98389

Comment on lines 169 to 172
#ifndef UNIX_SOCKET_UNAVAILABLE
NetSocketPosix::make_default();
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Should we call this POSIX_SOCKET_UNAVAILABLE? Technically it's about the Unix implementation, but the class it disables is the Posix one and includes the Windows implementation too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe POSIX_SOCKET_UNAVAILABLE would indeed be a better name.

I'm just reusing UNIX_SOCKET_UNAVAILABLE which was added in #32454 to allow building on some console platform.

I've actually been thinking about splitting Windows sockets since they've diverged quite a bit from the posix one (and their "compatibility" layer is not compatible)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah splitting Windows sockets might make sense, I've thought about this too on and off.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've opened #98969 which does that.

@akien-mga
Copy link
Member

This should probably be documented in some places?

@dustdfg
Copy link
Contributor

dustdfg commented Nov 8, 2024

This can make the build smaller, if we also disable the UPNP module (which will otherwise include a third party library referencing "socket" thus forcing emscripten to include the compatibility layer)

What is the problem with disabling it? Something in web heavily depends on it? I don't know anything about network but from reading description on site of miniupnp I got a feeling it anyway won't work in web or not?

It looks like it can be easily disabled like already disabled raycast module

def get_flags():
return {
"arch": "wasm32",
"target": "template_debug",
"builtin_pcre2_with_jit": False,
"vulkan": False,
# Embree is heavy and requires too much memory (GH-70621).
"module_raycast_enabled": False,

@Faless
Copy link
Collaborator Author

Faless commented Nov 8, 2024

@dustdfg unlike the raycast module, UPNP adds exposed types. This means that if your GDScript includes references to that type, the script will fail parsing, even if you don't actually use it in web builds.

@AThousandShips AThousandShips added this to the 4.4 milestone Nov 9, 2024
@Repiteo
Copy link
Contributor

Repiteo commented Nov 12, 2024

Needs to be rebased now that #98969 is merged

They are not supported anyway, emscripten has an emulation layer that
implements them over WebSocket/WebRTC, which is really surprising for
users, and also not very useful since we have proper WebSocket and
WebRTC support.

This can make the build smaller, if we also disable the UPNP module
(which will otherwise include a third party library referencing "socket"
thus forcing emscripten to include the compatibility layer)
@Faless
Copy link
Collaborator Author

Faless commented Nov 13, 2024

@Repiteo I rebased this on top of #99167 (fixup on #98969)

@Repiteo Repiteo merged commit eb92f72 into godotengine:master Nov 13, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 13, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The StreamPeerTCP on the web sends a string of data when the connection is successful
6 participants