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

@bagder
Copy link
Member

@bagder 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 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 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 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 added a commit to curl/curl-for-win that referenced this issue 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 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

@vszakats vszakats commented Jul 13, 2022

Rebased onto master.

@vszakats
Copy link
Member Author

@vszakats 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?

bagder
bagder approved these changes Jul 19, 2022
@vszakats vszakats closed this in 4d73854 Jul 19, 2022
@vszakats vszakats deleted the delunused branch Jul 19, 2022
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.

None yet

3 participants