Skip to content

WebSockets: make support official (non-experimental) - #14936

Closed
bagder wants to merge 5 commits into
masterfrom
bagder/welcome-ws
Closed

WebSockets: make support official (non-experimental)#14936
bagder wants to merge 5 commits into
masterfrom
bagder/welcome-ws

Conversation

@bagder

@bagder bagder commented Sep 16, 2024

Copy link
Copy Markdown
Member

Inverts the configure/cmake options to instead provide options that disable WebSockets and have them (ws + wss) enabled by default.

@bagder bagder added the feature-window A merge of this requires an open feature window label Sep 16, 2024
@github-actions github-actions Bot added the tests label Sep 16, 2024
@dfandrich

This comment was marked as outdated.

@bagder
bagder force-pushed the bagder/welcome-ws branch 2 times, most recently from 44bb28a to 8f78651 Compare September 22, 2024 19:31
bagder added a commit that referenced this pull request Sep 25, 2024
Inverts the configure/cmake options to instead provide options that
disable WebSockets and have them (ws + wss) enabled by default.

Closes #14936
@bagder bagder closed this in d78e129 Sep 27, 2024
@bagder
bagder deleted the bagder/welcome-ws branch September 27, 2024 11:23
vszakats added a commit that referenced this pull request Sep 29, 2024
- restore change lost after websockets-default update.
  Ref: 6a1dcdc #14998
- delete unused line after websockets is on by default.
  Follow-up to d78e129 #14936

Closes #15078
@dg0yt

dg0yt commented Nov 6, 2024

Copy link
Copy Markdown
Contributor

Inverts the configure/cmake options to instead provide options that disable WebSockets

Not in the release notes. 😞
Also not reported as unused by CMake due to

curl/CMakeLists.txt

Lines 49 to 64 in b1ef0e1

# Collect command-line arguments for buildinfo.txt.
# Must reside at the top of the script to work as expected.
get_cmake_property(_cache_vars CACHE_VARIABLES)
unset(_cmake_args)
foreach(_cache_var ${_cache_vars})
get_property(_cache_var_helpstring CACHE ${_cache_var} PROPERTY HELPSTRING)
if(_cache_var_helpstring STREQUAL "No help, variable specified on the command line.")
get_property(_cache_var_type CACHE ${_cache_var} PROPERTY TYPE)
if(_cache_var_type STREQUAL "UNINITIALIZED")
set(_cache_var_type)
else()
set(_cache_var_type ":${_cache_var_type}")
endif()
set(_cmake_args "${_cmake_args} -D${_cache_var}${_cache_var_type}=\"${${_cache_var}}\"")
endif()
endforeach()

Another thing to patch out?

@vszakats

vszakats commented Nov 6, 2024

Copy link
Copy Markdown
Member

Another thing to patch out?

It's useful to know how curl was built when debugging, though not supercritical to have this in buildinfo.txt (we can see the command-line now in both GHA and AppVeyor). I'll make a PR to delete it and avoid this side-effect.

Is there a better method to read the cmake command-line?

vszakats added a commit to vszakats/curl that referenced this pull request Nov 6, 2024
It has the side-effect of silencing CMake warnings about unused
variables passed via the command-line.

Drop it till a better method is found to retrieve this.

Reported-by: Kai Pastor
Ref: curl#14936 (comment)
Follow-up to 1fdea16 curl#14802
@dg0yt

dg0yt commented Nov 7, 2024

Copy link
Copy Markdown
Contributor

Is there a better method to read the cmake command-line?

It doesn't matter. You cannot assume to see the complete actual configuration in the command line of a single invocation. Input variables = cache variables. I can re-run cmake with no parameters, or I can change a single parameter. And then there are presets etc.

What you have is CMakeCache.txt ;-)

@vszakats

vszakats commented Nov 7, 2024

Copy link
Copy Markdown
Member

Is there a better method to read the cmake command-line?

It doesn't matter. You cannot assume to see the complete actual configuration in the command line of a single invocation. Input variables = cache variables. I can re-run cmake with no parameters, or I can change a single parameter. And then there are presets etc.

In many cases the command-line tells enough to be useful.

What you have is CMakeCache.txt ;-)

Yes, there is also curl_dumpvars(), but for buildinfo.txt they're overkill. It was just meant to match ./configure's $ac_configure_args.

@dg0yt

dg0yt commented Nov 7, 2024

Copy link
Copy Markdown
Contributor

In many cases the command-line tells enough to be useful.

... assuming a single invocation of CMake for configuration purposes.

vszakats added a commit that referenced this pull request Nov 8, 2024
Collecting the args list has the undesired side-effect of silencing
CMake warnings about unused variables passed via the command-line.

Drop it till a better method is found to retrieve them.

Reported-by: Kai Pastor
Ref: #14936 (comment)
Follow-up to 1fdea16 #14802
Closes #15501
pps83 pushed a commit to pps83/curl that referenced this pull request Apr 26, 2025
Inverts the configure/cmake options to instead provide options that
disable WebSockets and have them (ws + wss) enabled by default.

Closes curl#14936
pps83 pushed a commit to pps83/curl that referenced this pull request Apr 26, 2025
- restore change lost after websockets-default update.
  Ref: 6a1dcdc curl#14998
- delete unused line after websockets is on by default.
  Follow-up to d78e129 curl#14936

Closes curl#15078
pps83 pushed a commit to pps83/curl that referenced this pull request Apr 26, 2025
Collecting the args list has the undesired side-effect of silencing
CMake warnings about unused variables passed via the command-line.

Drop it till a better method is found to retrieve them.

Reported-by: Kai Pastor
Ref: curl#14936 (comment)
Follow-up to 1fdea16 curl#14802
Closes curl#15501
vszakats added a commit that referenced this pull request Jan 18, 2026
By using weird indentation in the autoconf source.

Fixing:
```
  --enable-sspi           Enable SSPI
    --disable-sspi          Disable SSPI
[...]
  --enable-websockets     Enable WebSockets support
    --disable-websockets    Disable WebSockets support
```

Follow-up to 923db35 #18116
Follow-up to d78e129 #14936

Closes #20342
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature-window A merge of this requires an open feature window tests WebSocket

Development

Successfully merging this pull request may close these issues.

4 participants