curl-config in C supporting CMake builds

[phetdam]

Motivation

To support a project on Windows I built libcurl from the Git repo checked out to 8.11.2-DEV with CMake as follows:

cmake -S . -B build_windows_x64 -A x64 -DCURL_USE_LIBPSL=OFF -DCURL_USE_SCHANNEL=ON ^
    -DCMAKE_INSTALL_PREFIX=install_win
cmake --build build_windows_x64 --config Debug -j
cmake --build build_windows_x64 --config Release -j

After setting CURL_ROOT in the environment to allow CMake to locate the libcurl installation, I then ran my CMake build for the other project, and noticed that find_package(CURL 7.68 REQUIRED COMPONENTS HTTPS) would get me this:

CMake Error at C:/Program Files/Microsoft Visual Studio/2022/Community/Common7/IDE/CommonExtensions/Microsoft/CMake/CMake/share/cmake-3.26/Modules/FindCURL.cmake:169 (message):
  CURL: Required protocol HTTPS is not found

Digging into the CMake FindCURL module I noticed that if config mode search fails (which it is failing), curl-config would be then used to determine the available features and protocols. Unfortunately, curl-config is a shell script, and thus unusable natively on Windows. I figured, however, that it should be possible to write curl-config as a (portable!) C program, in which case curl-config can be used on both Windows or POSIX-like systems equally.

PR summary

This PR therefore adds a curl-config.c.in file that will be run through CMake's configure_file using the same variables that are used to configure curl-config.in, although some variables that use relative variable expansions (e.g. execprefix, libdir, and includedir) are supplanted with full-path-expanded lit_-prefixed variables. For Windows, we also normalize the LIBCURL_PC_LIBS_PRIVATE list of libraries into Windows-format CMake list LIBCURL_WIN32_PC_LIBS_PRIVATE.

The curl-config.c.in gets converted into curl-config.c by configure_file, with all the @ patterns fully expanded into absolute paths, and is then compiled and installable with cmake --install in place of the curl-config shell script.

To not break existing behavior, I have kept the curl-config behavior identical where possible, only hardening --checkfor to report if input is invalid, i.e. not in major[.minor[.patch[-info]]] format, or if the expected argument is missing. However, the help output will report that curl-config was configured using a particular CMake version.

Testing results

Currently, using a local 8.11.2-DEV installation of libcurl built from this branch, the find_package call is working correctly, and CMake is able to correctly invoke the curl-config C program to determine what components are available. In particular, the following outputs seem to be "as expected" on Windows (build + install same as in the motivation section):

> .\install_win\bin\curl-config --version
libcurl 8.11.2-DEV
> .\install_win\bin\curl-config --ssl-backends
Schannel
> .\install_win\bin\curl-config --protocols | find "HTTPS"
HTTPS
> .\install_win\bin\curl-config --checkfor bad_version
error: provided version bad_version is invalid
> .\install_win\bin\curl-config --checkfor 8.11.3
requested version 8.11.3 is newer than existing 8.11.2-DEV

Addendum

Since this my first PR I would appreciate any suggestions for improvements. I wasn't able to find any other reports on using find_package(CURL) on Windows when I searched the mailing list archive so I read the contribution guidelines and decided to put out this PR as in my mind, it would be better if curl-config could be portable to Windows too, In the past I've successfully built libcurl with zlib support and Schannel as the TLS implementation and used it in Visual Studio projects (via hardcoding include/library paths and names), so I was a bit surprised to find CMake choking.

[phetdam]

I kept the variable names similar for the most part to make it easier to compare against curl-config.in.

[phetdam]

This still prints nothing (see CMakeLists.txt TODO for CONFIGURE_OPTIONS).

It could be changed to some CMake configure options, e.g. -DCURL_USE_SCHANNEL=ON -DCURL_USE_LIBPSL=OFF but how exactly this is done probably will require some CMake scripting to get a consistent format.

[phetdam]

Ok, so across the failing CI runs it's the curl-config tests that are failing:

FAIL 1013: 'Compare curl --version with curl-config --protocols' curl-config
FAIL 1014: 'Compare curl --version with curl-config --features' curl-config
FAIL 1022: 'Compare curl --version with curl-config --version' curl-config
FAIL 1023: 'Compare curl --version with curl-config --vernum' curl-config

TESTFAIL: These test cases failed: 1013 1014 1022 1023

test1013.pl and test1022.pl in tests/libtest seem to be failing because curl-config is now an executable. I considered adding the following instead to handle the case when curl-config is a C program, not the shell script:

# curl-config as C program (from CMake)
if (-B $ARGV[0]) {
    open(CURLCONFIG, "$ARGV[0] --$what|") ||
    die "Can't get curl-config C program $what list\n";
}
# curl-config as shell script (autotools)
else {
    open(CURLCONFIG, "sh $ARGV[0] --$what|") ||
    die "Can't get curl-config shell script $what list\n";
}

This seems to work locally after I finally figured out the second argument to test1013.pl and test1022.pl should be a file holding the output of ./build/src/curl --version. I think CI should pass now.

Edit: Was being overly optimistic. Some CI jobs don't seem like they allow curl-config (the program) to execute.

[testclutch]

Analysis of PR #15854 at 6caf23f4:

Test http/test_14_auth.py::TestAuth::test_14_03_digest_put_auth[0-h3] failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Test 1014 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 8 different CI jobs (the link just goes to one of them).

Test 1013 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 8 different CI jobs (the link just goes to one of them).

Test 1022 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 8 different CI jobs (the link just goes to one of them).

Test 1023 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 8 different CI jobs (the link just goes to one of them).

Generated by Testclutch

[vszakats]

Curl provides a curl-config.cmake for CMake config mode detection also on Windows.

Is there a reaon why it did not work with find_package(CURL)?

[phetdam]

I'm not entirely sure why the CMake config mode search failed, but it definitely is failing. E.g. consider:

find_package(CURL 7.68 CONFIG REQUIRED COMPONENTS HTTPS)

Here find module mode is disabled, so only CURLConfig.cmake is getting run, not CMake's own find module (which since 3.17 searchs with config mode first; I am using CMake 3.26.0). Unfortunately this yields a less-than-helpful message:

CMake Error at CMakeLists.txt:181 (find_package):
  Found package configuration file:

    C:/path/to/curl-8.11.1/install_win/lib/cmake/CURL/CURLConfig.cmake

  but it set CURL_FOUND to FALSE so package "CURL" is considered to be NOT
  FOUND.

As an addendum, the provided CURLConfig.cmake doesn't seem to allow identification of particular features or protocols that the installed libcurl supports; the CMake FindCURL module does its first config mode search like this:

find_package(CURL QUIET NO_MODULE)

[vszakats]

I propose #15858 which adds feature/protocol info to CURLConfig.cmake.
It allows FindCURL to get this information natively.

The REQUIRED COMPONENTS error seems like a CMake FindCURL issue to me.
It probably shouldn't fail on Windows like it does. It could also run
the curl executable to extract feature/protocol information.

A workaround may be to use libcurl.pc via pkg-config, which FindCURL
tries before falling back to curl-config.

Other methods is to not rely on this FindCURL feature but implement it
manually either by calling curl -V or using pkg-config to extract it
from libcurl.pc. Or just read CURL_SUPPORTED_PROTOCOLS after #15858
landed. It works with find_package(CURL QUIET NO_MODULE).

# method 1
find_package(PkgConfig QUIET)
pkg_get_variable(_proto_str libcurl supported_protocols)
message(STATUS "protocols method 1|${_proto_str}|")

# method 2
find_program(CURL_EXECUTABLE NAMES curl)
execute_process(COMMAND ${CURL_EXECUTABLE} --disable --version OUTPUT_VARIABLE _curl_V
  ERROR_QUIET OUTPUT_STRIP_TRAILING_WHITESPACE)
set(_proto_regex "Protocols: (.+)")
string(REGEX MATCH "${_proto_regex}" _proto_str ${_curl_V})
string(REGEX REPLACE "${_proto_regex}" "\\1" _proto_str "${_proto_str}")
message(STATUS "protocols method 2|${_proto_str}|")

# method 3 requires #15858
find_package(CURL)
message(STATUS "protocols method 3|${CURL_SUPPORTED_PROTOCOLS}|")
message(STATUS "HTTPS supported|${CURL_SUPPORTED_HTTPS_FOUND}|")

As for converting curl-config to C and a binary, I'm not in favour. It
feels overkill with long-term implications to fix an issue like this. If
we opt doing it anyway, autotools build also needs to be converted, with
more complexity. This effectively replicates curl -V via curl-config,
but has to deal with also making curl-config compatible with MSVC.
Tests would also need to be rewritten.

[phetdam]

I propose #15858 which adds feature/protocol info to CURLConfig.cmake. It allows FindCURL to get this information natively.

I think this is good. However, maybe some extra work needs to be done for find_package(CURL COMPONENTS ...) to work correctly; according to CMake documentation it seems like the config file itself needs to have some logic to determine which requested components match and which don't. The built-in <PackageName>_FIND_COMPONENTS and related variables might be helpful to check if items in those lists are also in CURL_SUPPORTED_PROTOCOLS and/or CURL_SUPPORTED_FEATURES.

On a related note, I think CURL_FOUND may have to be set by CURLConfig.cmake. As I commented previously using config mode search seems to show that CURL_FOUND is false (or not set, possibly). It's possible this issue is masked by the CMake FindCURL module, since if config search fails, it will use pkg-config (which works for system libcurl installations).

The REQUIRED COMPONENTS error seems like a CMake FindCURL issue to me. It probably shouldn't fail on Windows like it does. It could also run the curl executable to extract feature/protocol information.

A workaround may be to use libcurl.pc via pkg-config, which FindCURL tries before falling back to curl-config.

If config search mode is failing, however, then something must not be right with the CURLConfig.cmake that ends up being generated. On Windows I can't use pkg-config here, but as you suggested, I did consider essentially writing my own CURL-locating CMake function that would use CMake's FindCURL on POSIX-like systems, but for Windows, do this:

1. Look for curl/curl.h and curl/curlver.h, maybe get version info from curl/curlver.h
2. Look for curl.exe and invoke it to get features + components info
3. Check against the extracted features + components

As for converting curl-config to C and a binary, I'm not in favour. It feels overkill with long-term implications to fix an issue like this. If we opt doing it anyway, autotools build also needs to be converted, with more complexity. This effectively replicates curl -V via curl-config, but has to deal with also making curl-config compatible with MSVC. Tests would also need to be rewritten.

Yes, I 100% agree. Rewriting curl-config in C should ideally mean both autotools and CMake builds need to be involved. And as you can tell from the CI failures for this PR, even though the Perl tests might be easily changed, the CI jobs that ensure curl-config is generated the same from autotools and CMake would have to be redone.

Anyways, I think for now I'll use a CMake workaround, and if my opinion matters at all, I am definitely in favor of having some changes done to CURL's CMake config file as a more targeted solution. Thanks for the discusssion!

[phetdam]

Ah, interesting find after I decided to directly edit the installed CURLConfig.cmake as an experiment.

After manually inserting set(CURL_FOUND TRUE) into the file, config mode search is succeeding.

Furthermore, confirming what I read in the find_package docs, we can use CURL_FIND_COMPONENTS to get the list of components requested in find_package, so these could be checked against the new features + protocols lists added in #15858. So it seems that perhaps the minimal steps to get the CURL CMake config working for config mode search is to at least set CURL_FOUND to TRUE and CURL_VERSION_STRING to the value of CURL_VERSION (for consistency with the FindCURL find module).

I suppose more work is required for full integration with CMake's own FindCURL module, as after the config search it does check whether or not all requested components were found. So CURL_HTTPS_FOUND would have to be set, etc.

[vszakats]

I'm still trying to figure out where CURL_FOUND (and possible variations) are supposed
to be set and when. It's set by some projects, not set by most (as per quick local search).
For our Find modules it's set by find_package_handle_standard_args() if the passed,
required variables are set. So far it seems it's supposed to be set by CMake.

In my non-Windows env find_package(CURL CONFIG) sets CURL_FOUND to 1
with CURL_VERSION_STRING missing. find_package(CURL) sets CURL_FOUND
to TRUE and fills CURL_VERSION_STRING. Is there any reason why we want this
synonym when CURL_VERISION is set consistently?

We can emulate some of the stuff done by FindCURL, but it looks kind of messy.
E.g. it uses "ICT" instead of "DICT", and it would only set CURL_<feat>_FOUND
for those features explicitly passed via COMPONENTS, but it also resets all
"known" features' CURL_<feat>_FOUND variables to FALSE. The list of "known"
features is a hard-coded string that's incomplete. Meaning the end result is not
usable to reliably tell which features are available. The definition of "component"
is also loose, it now means protocols or features, but it might as well mean a
curl dependency (e.g. GnuTLS), some of which are not reflected in the list of
features/protocols (see also: #14930). The idea behind COMPONENTS is
fuzzy, and one interpretation is to requrest certain features to be enabled.
But in curl, these features were already enabled at build time, so it only
makes sense to check if they are present, with nothing to actually enable.
The built-in components are always available without checking for them. It'd
make sense to set those to FOUND, always. I also wonder if it adds something
useful to explicitly set potential, but disabled features to FALSE? (as opposed to
just leaving the _FOUND variable unset) (Where "potential" is derived from the
outdated list mentioned above.)

Maybe curl should set its own variables names where things are in order?
and do a "best effort" to set things for FindCURL compatibility?

Here's a patch implementing some of these:

diff --git a/CMake/curl-config.cmake.in b/CMake/curl-config.cmake.in
index cce77d122..7cf798bfb 100644
--- a/CMake/curl-config.cmake.in
+++ b/CMake/curl-config.cmake.in
@@ -52,3 +52,21 @@ set_and_check(CURL_INCLUDE_DIRS "@PACKAGE_CMAKE_INSTALL_INCLUDEDIR@")
 
 string(REPLACE " " ";" CURL_SUPPORTED_PROTOCOLS "@SUPPORT_PROTOCOLS@")
 string(REPLACE " " ";" CURL_SUPPORTED_FEATURES "@SUPPORT_FEATURES@")
+
+foreach(_item IN LISTS CURL_SUPPORTED_PROTOCOLS)
+  set(CURL_PROTOCOL_${_item}_FOUND TRUE)
+endforeach()
+foreach(_item IN LISTS CURL_SUPPORTED_FEATURES)
+  set(CURL_FEATURE_${_item}_FOUND TRUE)
+endforeach()
+
+foreach(_item IN LISTS CURL_FIND_COMPONENTS)
+  if(NOT "${CURL_PROTOCOL_${_item}_FOUND}" AND
+     NOT "${CURL_FEATURE_${_item}_FOUND}")
+    if(CURL_FIND_REQUIRED)
+      message(FATAL_ERROR "CURL: Required protocol/feature '${_item}' is not found")
+    else()
+      message(WARNING "CURL: Protocol/feature '${_item}' is not found")
+    endif()
+  endif()
+endforeach()

[phetdam]

Apologies in advance for the long response--you've given me a lot of good discussion :)

I'm still trying to figure out where CURL_FOUND (and possible variations) are supposed to be set and when. It's set by some projects, not set by most (as per quick local search). For our Find modules it's set by find_package_handle_standard_args() if the passed, required variables are set. So far it seems it's supposed to be set by CMake.

Yes, on reflection this does not need to be explicitly set by CURLConfig.cmake.

In my non-Windows env find_package(CURL CONFIG) sets CURL_FOUND to 1 with CURL_VERSION_STRING missing. find_package(CURL) sets CURL_FOUND to TRUE and fills CURL_VERSION_STRING. Is there any reason why we want this synonym when CURL_VERISION is set consistently?

My mistake, CURL_VERSION is the "correct" variable to set and the FindCURL module will set CURL_VERSION_STRING. CURLConfig.cmake need not set CURL_VERSION_STRING by itself.

With regards to the config mode search issue, I think I've narrowed down why exactly find_package(CURL CONFIG REQUIRED) will succeed and set CURL_FOUND to true, but find_package(CURL CONFIG REQUIRED COMPONENTS HTTPS) will fail. This is because the curl-config.cmake.in has a check_required_components call after including CURLTargets.cmake:

curl/CMake/curl-config.cmake.in

Line 42 in 8a66c11

check_required_components("@PROJECT_NAME@")

Since CURLConfig.cmake is not setting any CURL_<component>_FOUND variables, check_required_components ends up setting CURL_FOUND to false. I confirmed this by commenting that line out and trying the following:

find_package(CURL 7.68 CONFIG REQUIRED COMPONENTS HTTPS)

We can emulate some of the stuff done by FindCURL, but it looks kind of messy. E.g. it uses "ICT" instead of "DICT", and it would only set CURL_<feat>_FOUND for those features explicitly passed via COMPONENTS, but it also resets all "known" features' CURL_<feat>_FOUND variables to FALSE. The list of "known" features is a hard-coded string that's incomplete. Meaning the end result is not usable to reliably tell which features are available... The idea behind COMPONENTS is fuzzy, and one interpretation is to requrest certain features to be enabled. But in curl, these features were already enabled at build time, so it only makes sense to check if they are present, with nothing to actually enable.

Yes, the FindCURL module is written from an "outsider's" ad-hoc point of view, hence the hardcoded features + protocols and the unfixed ICT typo. It was probably written for a time where CURL did not have CMake support, hence FindCURL first falling back to pkg-config and then to curl-config as appropriate (although everything is still quite heuristic).

In my mind COMPONENTS, with regards to libcurl, is probably better suited as a way to check that the detected libcurl has been built with the specific protocol/feature available. E.g. there are some cases where some components are definitely required, and if find_package(CURL CONFIG COMPONENTS ...) can be supported, that would allow an extra layer of correctness checking.

I've seen an issue before where libcurl was mistakenly rebuilt without libz support and that caused HTTPS GET requests to an in-house REST endpoint to fail because that particular endpoint always sends data in gzip compressed chunks.

It'd make sense to set those to FOUND, always. I also wonder if it adds something useful to explicitly set potential, but disabled features to FALSE? (as opposed to just leaving the _FOUND variable unset) (Where "potential" is derived from the outdated list mentioned above.)

Yes, as I mentioned the FindCURL module is written from an outsider's point of view, hence the hardcoded features + protocols. I actually think they don't need to set CURL_<component>_FOUND to false for all their hardcoded features + protocols because ostensibly, find_package_handle_standard_args should check just the required CURL_<component>_FOUND values.

Maybe curl should set its own variables names where things are in order?

Yes, that would be ideal. I looked at your patch and have made some changes to better conform to CMake conventions, e.g. CURL_<component>_FOUND should only be set to true if the user-requested <component> is a feature/protocol available in the detected libcurl, and only emitting a fatal error if both CURL_FIND_REQUIRED and CURL_FIND_REQUIRED_<component> are true, which allows OPTIONAL_COMPONENTS to be specified and for CMake users to check if CURL_libz_FOUND is true, for example.

Here's a sample curl-config.cmake.in patch to replace the check_required_components call:

diff --git a/CMake/curl-config.cmake.in b/CMake/curl-config.cmake.in
index aa9eb51ff..6b4ce7f57 100644
--- a/CMake/curl-config.cmake.in
+++ b/CMake/curl-config.cmake.in
@@ -39,7 +39,33 @@ if("@HAVE_LIBZ@")
 endif()
 
 include("${CMAKE_CURRENT_LIST_DIR}/@TARGETS_EXPORT_NAME@.cmake")
-check_required_components("@PROJECT_NAME@")
+
+# set located components if requested
+# for each user-requested component <comp>, CURL_<comp>_FOUND is set to TRUE
+foreach(_req_comp ${CURL_FIND_COMPONENTS})
+  # first check features
+  list(FIND CURL_SUPPORTED_FEATURES ${_req_comp} CURL_${_req_comp}_POS)
+  # if not found, try again
+  if(CURL_${_req_comp}_POS EQUAL -1)
+    list(FIND CURL_SUPPORTED_PROTOCOLS ${_req_comp} CURL_${_req_comp}_POS)
+  endif()
+  # failed
+  if(CURL_${_req_comp}_POS EQUAL -1)
+    # if REQUIRED was given + component is not optional, hard error
+    if(CURL_FIND_REQUIRED_${_req_comp} AND CURL_FIND_REQUIRED)
+      message(FATAL_ERROR "CURL: missing component: ${_req_comp}")
+    # otherwise, just notify
+    else()
+      message(STATUS "CURL: missing component: ${_req_comp}")
+    endif()
+  # success, so mark component as found. this is necessary for correct
+  # interop with find_package_handle_standard_args and optional components
+  else()
+    set(CURL_${_req_comp}_FOUND TRUE)
+  endif()
+  # unset unused variable
+  unset(CURL_${_req_comp}_POS)
+endforeach()
 
 # Alias for either shared or static library
 if(NOT TARGET @PROJECT_NAME@::libcurl)

Do note this is assuming CURL_SUPPORTED_FEATURES and CURL_SUPPORTED_PROTOCOLS are already available as lists.

I've tested this with the following set of find_package calls (both Windows and Ubuntu without CMake):

# CONFIG mode only
find_package(CURL 7.68 CONFIG REQUIRED COMPONENTS HTTPS)
# allow use of find module, nonexistent optional component
find_package(CURL 7.68 REQUIRED COMPONENTS HTTPS OPTIONAL_COMPONENTS dont_exist)
# allow use of find module but don't specify REQUIRED
find_package(CURL 7.68 COMPONENTS HTTPS OPTIONAL_COMPONENTS dont_exist)

This seems to work well, as the optional components are correctly marked as not found, and even if REQUIRED is specified, this still allows CURL_FOUND to be true since the required HTTPS component is found.

If this ok, hopefully this can be accepted--from a CMake standpoint this is ideal behavior.

[vszakats]

Thanks for your inputs and research! In the meantime I also made some changes to the patch proposal
(see it over #15858).

It drops check_required_components() and the REQUIRED/COMPONENTS logic tweaked to match it,
also to make it behave more like documented in find_package().

One difference is that it sets CURL_<feat>_FOUND for all features available. Is there any point in
hiding available features if the caller didn't explicitly ask for them? It would make sense if detecting
availability would require an elaborate/slow check, but in our case, it's just looping through a static
list. Perhaps for compatibility we could retain the old behaviour for these variables and set all
CURL_SUPPORTED_<feat>_FOUND to TRUE unconditionally, maybe the _FOUND suffix isn't
necessary for these.

Can you give it a test?

[phetdam]

Thanks for your inputs and research! In the meantime I also made some changes to the patch proposal (see it over #15858).

No problem, thank you for the great discussion. It's valuable to have your perspective as someone more actively involved in the project; I am an "outsider" so my knowledge on how proposed changes might interact with the CURL project is limited.

I will take a look at #15858 and I hope you won't mind if I leave some comments when appropriate.

One difference is that it sets CURL_<feat>_FOUND for all features available. Is there any point in
hiding available features if the caller didn't explicitly ask for them? It would make sense if detecting
availability would require an elaborate/slow check, but in our case, it's just looping through a static
list. Perhaps for compatibility we could retain the old behaviour for these variables and set all
CURL_SUPPORTED_<feat>_FOUND to TRUE unconditionally, maybe the _FOUND suffix isn't
necessary for these.

From what I understand from the CMake documentation, for any <component> requested by the user, if it is found, then CURL_<component>_FOUND should be set to true. These CURL_<component>_FOUND variables should only be set to true for those user-requested components that are found, otherwise they should be false. This would allow check_required_components("CURL") to successfully check whether each required component has been found, so the CURLConfig.cmake module need not set CURL_FOUND to false explicitly. I forgot to note this, so the diff I commented previously is missing a check_required_components("CURL") call after the component check looping. Sorry for the mistake! I will add context in PR comments.

With regards to indicating all the supported features + protocols, I think downstream users can use CURL_SUPPORTED_FEATURES and CURL_SUPPORTED_PROTOCOLS, and for individual testing, a CURL_<feature_or_protocol>_SUPPORTED variable can be set to true for each feature/protocol supported by the installed libcurl. I think the notion of "finding" a component is moreso the user checking if the installed libcurl supports a feature/protocol they are looking for, which is the purpose of CURL_<component>_FOUND.

Can you give it a test?

Of course, would be my pleasure. I'll comment later on my findings.

[phetdam]

Looks like the changes in #15858 are working nicely.

I tested multiple find_package calls across Windows and Ubuntu using a libcurl installation built from the PR branch (with the new CMake config), and used combinations of REQUIRED, CONFIG, OPTIONAL_COMPONENTS.

Left some comments on the PR; let me know your thoughts. And happy New Year's Eve :)

[phetdam]

@vszakats Since our discussion has moved to #15858 I will close this PR. Thanks for your patience!