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

tidy-up: delete unused build configuration settings #9044

Closed
wants to merge 76 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Jun 24, 2022

Delete unused macros (most of them feature guards):

  • CURL_INCLUDES_SYS_UIO [1]
  • HAVE_ALLOCA_H [2]
  • HAVE_CRYPTO_CLEANUP_ALL_EX_DATA (unused since de71e68)
  • HAVE_DLFCN_H
  • HAVE_DLOPEN
  • HAVE_DOPRNT
  • HAVE_FCNTL
  • HAVE_GETHOSTBYNAME [3]
  • HAVE_GETOPT_H
  • HAVE_GETPASS
  • HAVE_GETPROTOBYNAME
  • HAVE_GETSERVBYNAME
  • HAVE_IDN_FREE*
  • HAVE_INET_ADDR
  • HAVE_IOCTL
  • HAVE_KRB4
  • HAVE_KRB_GET_OUR_IP_FOR_REALM
  • HAVE_KRB_H
  • HAVE_LDAPSSL_H
  • HAVE_LDAP_INIT_FD
  • HAVE_LIBDL
  • HAVE_LIBNSL
  • HAVE_LIBRESOLV*
  • HAVE_LIBUCB
  • HAVE_LL
  • HAVE_LOCALTIME_R
  • HAVE_MALLOC_H
  • HAVE_MEMCPY
  • HAVE_MEMORY_H
  • HAVE_NETINET_IF_ETHER_H
  • HAVE_NI_WITHSCOPEID
  • HAVE_OPENSSL_CRYPTO_H
  • HAVE_OPENSSL_ERR_H
  • HAVE_OPENSSL_PEM_H
  • HAVE_OPENSSL_PKCS12_H
  • HAVE_OPENSSL_RAND_H
  • HAVE_OPENSSL_RSA_H
  • HAVE_OPENSSL_SSL_H
  • HAVE_OPENSSL_X509_H
  • HAVE_PEM_H
  • HAVE_POLL
  • HAVE_RAND_SCREEN
  • HAVE_RAND_STATUS
  • HAVE_RECVFROM
  • HAVE_SETSOCKOPT
  • HAVE_SETVBUF
  • HAVE_SIZEOF_LONG_DOUBLE
  • HAVE_SOCKIO_H
  • HAVE_SOCK_OPTS
  • HAVE_STDIO_H
  • HAVE_STRCASESTR
  • HAVE_STRFTIME
  • HAVE_STRLCAT
  • HAVE_STRNCMPI
  • HAVE_STRNICMP
  • HAVE_STRSTR
  • HAVE_STRUCT_IN6_ADDR
  • HAVE_TLD_H
  • HAVE_TLD_STRERROR
  • HAVE_UNAME
  • HAVE_USLEEP
  • HAVE_WINBER_H
  • HAVE_WRITEV
  • HAVE_X509_H
  • LT_OBJDIR
  • NEED_BASENAME_PROTO
  • NOT_NEED_LIBNSL
  • OPENSSL_NO_KRB5
  • RECVFROM_TYPE*
  • SIZEOF_LONG_DOUBLE
  • STRERROR_R_TYPE_ARG3
  • USE_YASSLEMUL
  • _USRDLL (from CMake) [4]

[1] Maybe some related parts in m4/curl-functions.m4 and
configure.ac could also be deleted?

[2] Maybe the related comment can also be deleted in
packages/vms/generate_config_vms_h_curl.com

[3] There are more instances of this in autotools, but I did not dare to
touch those. Looked like it is used to detect socket support.

[4] This is necessary for MFC (Microsoft Foundation Class) DLLs to
force linking MFC components statically to the DLL. libcurl.dll
does not use MFC, so this define can be deleted.
Ref: https://docs.microsoft.com/cpp/build/regular-dlls-statically-linked-to-mfc

Closes #xxxx

@vszakats vszakats added build feature-window A merge of this requires an open feature window labels Jun 24, 2022
@bagder
Copy link
Member

bagder commented Jun 26, 2022

How did you find these? Can we create a script or test or something for this?

@vszakats
Copy link
Member Author

vszakats commented Jun 26, 2022

@bagder It was a byproduct of an effort in syncing autotools-cmake-Makefile.m32 builds to produce the exact same binaries 1. I dumped command-line -D + curl-config.h / config-win32.h options for all three into sorted lists and started comparing. Then I git grep for those that were only used by some of the build tools.

Some unused settings are still being generated by autotools, e.g. most HAVE_OPENSSL_*_H ones, as the strings seem generated and not visible as-is, by grep. (UPDATE: well, they are visible, but I didn't try updating autotools files to stop generating them. Probably something to fix before merging.)

It's probably possible to automate this to some extent: The source for most of these settings is lib/curl_config.h.cmake, lib/curl_config.h.in and lib/config-*.h. So parsing them and then looking the results up in the source tree can work.

Footnotes

  1. This was a success for the generated .o files which are now all identical. Besides the OS string, which is different for each build tool, so I synced those with patches for these tests.

@vszakats
Copy link
Member Author

vszakats commented Jun 26, 2022

Here's a rough attempt to automate it:

#!/bin/sh

autoheader configure.ac  # generate lib/curl_config.h.in

{
  grep -o -E    'set\([A-Z][A-Z0-9_]{3,}'          CMake/Platforms/WindowsCache.cmake | sed -E 's|set\(||g'
  grep -o -E -h '#define +[A-Z][A-Z0-9_]{3,}'      lib/config-*.h                     | sed -E 's|#define +||g'
  grep -o -E    '#cmakedefine +[A-Z][A-Z0-9_]{3,}' lib/curl_config.h.cmake            | sed -E 's|#cmakedefine +||g'
  grep -o -E    '#undef +[A-Z][A-Z0-9_]{3,}'       lib/curl_config.h.in               | sed -E 's|#undef +||g'
} | sort -u | grep -v -F 'HEADER_CURL_' | while read -r def; do
  c="$(git grep -w -F "${def}" | grep -v -E -c '(/libcurl\.tmpl|^lib/config-|^lib/curl_config\.h\.cmake|^CMakeLists\.txt|^CMake/Platforms/WindowsCache\.cmake|^packages/vms/config_h\.com|^m4/curl-functions\.m4|^acinclude\.m4|^configure\.ac)')"
  if [ "${c}" = '0' ]; then
    echo "${def}"
  fi
done

This misses some RECVFROM_* macros. These are present in curl_setup_once.h in a section that is #if 0-guarded since f9894f4.

After a few tweaks the results are not so bad. It missed these few unique cases: RECVFROM_* (see above), HAVE_GETHOSTBYNAME which has an #undef in curl_setup.h, HAVE_OPENSSL_RAND_H / _USRDLL are CMake-specific leftovers), and it catches a whole lot more:

HAVE_ALLOCA_H
HAVE_BROTLI_DECODE_H
HAVE_DECL_GETPWUID_R
HAVE_DLFCN_H
HAVE_DLOPEN
HAVE_DOPRNT
HAVE_FCNTL
HAVE_GETPASS
HAVE_GSSAPI_GSSAPI_KRB5_H
HAVE_GSSHEIMDAL
HAVE_GSSMIT
HAVE_HYPER_H
HAVE_IDN_FREE
HAVE_IDN_FREE_H
HAVE_INTTYPES_H
HAVE_IOCTL
HAVE_KRB4
HAVE_KRB_GET_OUR_IP_FOR_REALM
HAVE_KRB_H
HAVE_LDAPSSL_H
HAVE_LDAP_H
HAVE_LDAP_INIT_FD
HAVE_LIBBROTLIDEC
HAVE_LIBDL
HAVE_LIBNSL
HAVE_LIBPSL
HAVE_LIBPSL_H
HAVE_LIBRESOLV
HAVE_LIBRESOLVE
HAVE_LIBRTMP_RTMP_H
HAVE_LIBSOCKET
HAVE_LIBSSH
HAVE_LIBUCB
HAVE_LIBWOLFSSH
HAVE_LIBZSTD
HAVE_LOCALTIME_R
HAVE_MACRO_SIGSETJMP
HAVE_MALLOC_H
HAVE_MEMCPY
HAVE_MSH3_H
HAVE_NETINET_IF_ETHER_H
HAVE_NGHTTP2_NGHTTP2_H
HAVE_NGHTTP3_NGHTTP3_H
HAVE_NGTCP2_NGTCP2_CRYPTO_H
HAVE_NGTCP2_NGTCP2_H
HAVE_NI_WITHSCOPEID
HAVE_OPENSSL_PKCS12_H
HAVE_O_NONBLOCK
HAVE_PEM_H
HAVE_POLL
HAVE_QUICHE_H
HAVE_SIGNAL_FUNC
HAVE_SIGNAL_MACRO
HAVE_SOCKET_H
HAVE_SOCKIO_H
HAVE_SOCK_OPTS
HAVE_SSL_GET_ECH_STATUS
HAVE_SSL_H
HAVE_STDINT_H
HAVE_STDLIB_H
HAVE_STRCASESTR
HAVE_STRLCAT
HAVE_STRNCMPI
HAVE_STRSTR
HAVE_STRUCT_IN6_ADDR
HAVE_SYS_UIO_H
HAVE_TLD_H
HAVE_TLD_STRERROR
HAVE_UNAME
HAVE_USLEEP
HAVE_WINLDAP_H
HAVE_WOLFSSH_SSH_H
HAVE_WRITEV
HAVE_X509_H
HAVE_ZSTD_H
NEED_BASENAME_PROTO
NEED_LBER_H
NOT_NEED_LIBNSL
OPENSSL_NO_KRB5
PACKAGE_BUGREPORT
PACKAGE_NAME
PACKAGE_STRING
PACKAGE_TARNAME
PACKAGE_URL
PACKAGE_VERSION
SELECT_QUAL_ARG5
SELECT_TYPE_ARG1
SELECT_TYPE_ARG234
SELECT_TYPE_ARG5
SELECT_TYPE_RETV
STRERROR_R_TYPE_ARG3
USE_ECH
USE_NGTCP2_CRYPTO_GNUTLS
USE_NGTCP2_CRYPTO_OPENSSL
USE_YASSLEMUL

Some of these may be used by external headers (e.g. OPENSSL_NO_KRB5), USE_ECH belongs to a future feature, some are automatically added by autotools, but some could surely be deleted.

@vszakats vszakats changed the title [WIP] tidy-up: delete unused build configuration settings tidy-up: delete unused build configuration settings Jun 26, 2022
@jay
Copy link
Member

jay commented Jun 27, 2022

I think the HAVE_ ones at least should be ok. OPENSSL_NO_KRB5 is only defined by config-dos but I'm not sure why as I'd expect it to be defined by opensslconf.h.

@vszakats vszakats added tidy-up and removed feature-window A merge of this requires an open feature window labels Jun 27, 2022
vszakats added a commit to curl/curl-for-win that referenced this pull request Jul 4, 2022
…skip]

AppVeyor CI (Debian):
                            real
                          ------
Makefile.m32              40m53s (=2453s) 100%  https://ci.appveyor.com/project/curlorg/curl-for-win/builds/44066230
CMake                     50m11s (=3011s) 123%  https://ci.appveyor.com/project/curlorg/curl-for-win/builds/44062838

Same on local machine (macOS):
                            real                  user      sys       space
                          ------                ------   ------   ---------
Makefile.m32              29m40s (=1780s) 100%  34m50s    7m48s   909343242 bytes
CMake (with PR 9044)      36m59s (=2219s) 125%  41m58s    9m26s   973999614 bytes
CMake                     37m18s (=2238s) 126%  42m12s    9m25s   973829216 bytes
autotools (with PR 9044)  43m01s (=2581s) 145%  46m50s   11m18s   984571419 bytes
autotools                 44m06s (=2646s) 149%  48m11s   11m47s   984477038 bytes

Ref: curl/curl#9044
@bagder
Copy link
Member

bagder commented Jul 4, 2022

OPENSSL_NO_KRB5 is only defined by config-dos but I'm not sure why

Yeah, that feels like an overstep. We should leave openssl to define those things.

@vszakats
Copy link
Member Author

Rebased onto master.

@vszakats
Copy link
Member Author

vszakats commented Jul 15, 2022

@bagder: At this point this is committable. The few FIXMEs are rather questions, that are beyond my insight to touch. Do you think I shall continue with this PR?

@vszakats vszakats closed this in 4d73854 Jul 19, 2022
@vszakats vszakats deleted the delunused branch July 19, 2022 15:13
vszakats added a commit to curl/curl-for-win that referenced this pull request Oct 25, 2023
Switch from `curl-gnumake.sh` to `curl-cmake.sh` with upcoming curl
release v8.5.0.

cmake builds are now _faster_ for Windows builds than raw gnumake
(aka `Makefile.mk`). They also use 'unity' mode, which makes builds
run fast with the side-effect of also creating potentially more
efficient binaries by allowing better compiler optimizations.

This also makes curl-for-win use the same build system for all target
platforms (`Makefile.mk` is not available for *nix platforms).

Initially on 2022-07-04 cmake was 25% slower than gnumake. By
2022-09-26 this reduced to 20%, by 2023-07-29 to 18% and after the
latest round of updates gnumake came out 7% slower than cmake.
This is for Windows, for a triple x64, arm64 and x86 build. In
absolute times this is 27m22s for cmake and 29m11s for gnumake.

(FWIW autotools builds are 52% slower than cmake unity builds now.)

Making cmake builds fast was a multi-year effort with these major steps:

1. add support for cmake builds in curl-for-win.
   420e73c
2. bring it in-line with gnumake and autotools builds and tidy-up
   as much as possible. Scattered to a many commits.
3. delete a whole bunch of unused feature detections.
   curl/curl@4d73854
   curl/curl#9044
   (and a lot more smaller commits)
4. speed up picky warning option initialization by avoiding brute-force
   all options. (first in libssh2, then in curl, then applied also
   ngtcp2, nghttp3, nghttp2)
   curl/curl@9c543de
   curl/curl#10973
5. implement single build run for shared and static libcurl + tool
   (first in libssh2)
   curl/curl@1199308
   curl/curl#11505
   53dcd49
6. implement single build pass for shared and static libcurl
   (for Windows initially)
   curl/curl@2ebc74c
   curl/curl#11546
7. extend above to non-Windows (e.g. macOS)
   curl/curl@fc9bfb1
   curl/curl#11627
   bafa77d (mac)
   1b27304 (linux)
8. implement unity builds: single compiler invocation for libcurl + tool
   curl/curl@3f8fc25
   curl/curl#11095
   curl/curl@f42a279
   curl/curl#11928
   67d7fd3
9. speed up 4x the cmake initialization phase (for Windows)
   curl/curl@2100d9f
   curl/curl#12044
10. speed up cmake initialization even more by pre-filling
   detection results for our well-known mingw-w64 environments.
   74dd967
   5a43c61

+1: speed up autotools initialization by mitigating a brute-force
   feature detection on Windows. This reduced total build times
   by 5 minutes at the time, for the 3 Windows targets in total.
   curl/curl@6adcff6
   curl/curl#9591

Also update build times for cmake-unity and gnumake, based on runs:
cmake-unity: https://ci.appveyor.com/project/curlorg/curl-for-win/builds/48357875
gnumake: https://ci.appveyor.com/project/curlorg/curl-for-win/builds/48358005
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants