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

workflow to compare configure vs cmake outputs #11964

Closed
wants to merge 2 commits into from
Closed

Conversation

bagder
Copy link
Member

@bagder bagder commented Sep 27, 2023

Details pending to fix/address:

@bagder bagder added build CI Continuous Integration labels Sep 27, 2023
@vszakats

This comment was marked as resolved.

vszakats added a commit to vszakats/curl that referenced this pull request Sep 27, 2023
With new option `CURL_DISABLE_SRP=ON` to force-disable it.

Also:
- fix detecting GnuTLS.
- comment typos, whitespace.

Ref: curl#11964

Closes #xxxxx
vszakats added a commit to vszakats/curl that referenced this pull request Sep 27, 2023
With new option `CURL_DISABLE_SRP=ON` to force-disable it.
To match existing similar option and detection logic in autotools.

Also:
- fix detecting GnuTLS.
- comment typos, whitespace.

Ref: curl#11964

Closes #xxxxx
@vszakats
Copy link
Member

vszakats commented Sep 27, 2023

Attempt to address TLS-SRP: #11967 [MERGED]

@bagder bagder added the cmake label Sep 27, 2023
vszakats added a commit to vszakats/curl that referenced this pull request Sep 27, 2023
With new option `CURL_DISABLE_SRP=ON` to force-disable it.
To match existing similar option and detection logic in autotools.

Also:
- fix detecting GnuTLS.
- comment typos, whitespace.

Ref: curl#11964

Closes #xxxxx
vszakats added a commit that referenced this pull request Sep 28, 2023
With new option `CURL_DISABLE_SRP=ON` to force-disable it.
To match existing option and detection logic in autotools.

Also:
- fix detecting GnuTLS.
  We assume `nettle` as a GnuTLS dependency.
- add CMake GnuTLS CI job.
- bump AppVeyor CMake OpenSSL MSVC job to OpenSSL 1.1.1 (from 1.0.2)
  TLS-SRP fails to detect with 1.0.2 due to an OpenSSL header bug.
- fix compiler warning when building with GnuTLS and disabled TLS-SRP.
- fix comment typos, whitespace.

Ref: #11964

Closes #11967
bagder added a commit that referenced this pull request Sep 28, 2023
- for sys/uio.h
- for fork
- for connect

Ref: #11964
bagder added a commit that referenced this pull request Sep 28, 2023
- check for arc4random. To make rand.c use it accordingly.
- check for fcntl
- fix fseek detection
- add SIZEOF_CURL_SOCKET_T
- fix USE_UNIX_SOCKETS
- define HAVE_SNPRINTF to 1
- check for fnmatch
- check for sched_yield
- remove HAVE_GETPPID duplicate from curl_config.h
- add HAVE_SENDMSG

Ref: #11964
@bagder

This comment was marked as resolved.

bagder added a commit that referenced this pull request Sep 28, 2023
It is not used in any code anywhere.

Ref: #11964
bagder added a commit that referenced this pull request Sep 28, 2023
And fix the HAVE_LONGLONG define

Ref: #11964
bagder added a commit that referenced this pull request Sep 28, 2023
And fix the HAVE_LONGLONG define

Ref: #11964
Closes #11977
bagder added a commit that referenced this pull request Sep 28, 2023
It is not used in any code anywhere.

Ref: #11964
Closes #11975
bagder added a commit that referenced this pull request Sep 28, 2023
- for sys/uio.h
- for fork
- for connect

Ref: #11964

Closes #11973
bagder added a commit that referenced this pull request Sep 28, 2023
- check for arc4random. To make rand.c use it accordingly.
- check for fcntl
- fix fseek detection
- add SIZEOF_CURL_SOCKET_T
- fix USE_UNIX_SOCKETS
- define HAVE_SNPRINTF to 1
- check for fnmatch
- check for sched_yield
- remove HAVE_GETPPID duplicate from curl_config.h
- add HAVE_SENDMSG

Ref: #11964

Co-authored-by: Viktor Szakats
Closes #11973
vszakats added a commit to vszakats/curl that referenced this pull request Sep 28, 2023
Move detection before the creation of detection results
in `curl_config.h`.

Ref: curl#11964

Closes #xxxxx
vszakats added a commit to vszakats/curl that referenced this pull request Sep 28, 2023
Move detection before the creation of detection results
in `curl_config.h`.

Ref: curl#11964

Closes #xxxxx
vszakats added a commit that referenced this pull request Oct 2, 2023
This restores `CURL_CHECK_FUNC_IOCTL` detection. I deleted it in
4d73854 and
c345665 (2022-08), because the
`HAVE_IOCTL` result it generated was unused in the source. But,
I did miss the fact that this had two dependent checks:
`CURL_CHECK_FUNC_IOCTL_FIONBIO`,
`CURL_CHECK_FUNC_IOCTL_SIOCGIFADDR` that we do actually need:
`HAVE_IOCTL_FIONBIO`, `HAVE_IOCTL_SIOCGIFADDR`.

Regression from 4d73854

Ref: #11964 (effort to sync cmake detections with autotools)

Closes #12008
vszakats added a commit that referenced this pull request Oct 2, 2023
- set `HAVE_LDAP_URL_PARSE` if `ldap_url_parse` function exists.
  Before this patch we set it based it on the presence of `stricmp`,
  which correctly enabled it on e.g. Windows, but was inaccurate for
  other platforms.

- always set `HAVE_LDAP_SSL` if an LDAP backend is detected and
  LDAPS is not explicitly disabled. This mimics autotools behaviour.
  Previously we set it only for Windows LDAP. After this fix, LDAPS is
  correctly enabled in default macOS builds.

- enable LDAP[S] for a CMake macOS CI job. Target OS X 10.9 (Mavericks)
  to avoid deprecation warnings for LDAP API.

- always detect `HAVE_LDAP_SSL_H`, even with LDAPS explicitly disabled.
  This doesn't make much sense, but let's do it to sync behaviour with
  autotools.

- fix benign typo in variable name.

Ref: #11964 (effort to sync cmake detections with autotools)

Closes #12006
@bagder
Copy link
Member Author

bagder commented Oct 3, 2023

HAVE_SA_FAMILY_T missing from configure

This is currently sorted out in the header file so it does not actually need to be fixed.

@vszakats
Copy link
Member

vszakats commented Oct 3, 2023

Pushed #12015 that fixes HAVE_LDAP_URL_PARSE in my first LDAP patch. I forgot to delete the old logic.

There are more problems though: autotools detects LDAP and LDAPS (via libs?), but NOT the LDAP headers, which it checks separately. CMake only checks for headers and that fails. HAVE_LDAP_SSL is based on the non-header LDAPS detection in autotools, so it mismatches the simplistic method (= ! disabled + headers detected) CMake still uses.

I'm leaving this to somebody else, as the last weeks were exhausting and I'm starting to make mistakes. I'm also not familiar with LDAP and its history.

UPDATE: I tested this locally and turns out I misread the autotools output above. autotools is not detecting LDAP libs, only saying that LDAP/LDAPS is enabled as a feature. Later it reports that LDAPs was not found. Also in local tests the LDAP results match on a vanilla Debian (bullseye).

@bagder
Copy link
Member Author

bagder commented Oct 3, 2023

@vszakats I also think that detection of individual third party libs are less important than getting "the generic" stuff more aligned and I think we have taken a huge step in that direction now.

@vszakats
Copy link
Member

vszakats commented Oct 3, 2023

Agreed, we can tackle the LDAP details later. With some (wildcard?) allowlisting, we're almost there.

It'd even be useful to include this check and ignore the diff exit code for now.

Suggestion: I think results would be easier to read if named autotools / cmake instead of one and other.

vszakats added a commit that referenced this pull request Oct 3, 2023
Left there by accident after adding proper detection for this.

Follow-up to 772f0d8 #12006

Ref: #11964 (effort to sync cmake detections with autotools)

Closes #12015
Presumbly generated with configure and cmake.

Display the differences. Filter out a lot of known lines we ignore.
@bagder
Copy link
Member Author

bagder commented Oct 3, 2023

@vszakats I kept the exit code now but have 20-something lines filtered off and now it runs green. I figure we can go live with this and see how it runs over time and adjust it as we go forward. You good with that?

The script outputs the filter lines that are not actually filtered out, so we can figure out if we want to clean the filter going forward.

@vszakats
Copy link
Member

vszakats commented Oct 3, 2023

LGTM. Thanks for this!

@bagder bagder closed this in 2e0fa50 Oct 3, 2023
@bagder bagder deleted the bagder/cmp-config branch October 3, 2023 21:34
vszakats added a commit that referenced this pull request Oct 4, 2023
Follow-up to 2e0fa50 #11964
Follow-up to c39585d #12000

Closes #12023
vszakats added a commit that referenced this pull request Oct 4, 2023
- cmake: detect OpenLDAP based on function `ldap_init_fd`.
  autotools does this. autotools also publishes this detection result
  in `HAVE_LDAP_INIT_FD`. We don't mimic that with CMake as the source
  doesn't use this value. (it might need to be remove-listed in
  `scripts/cmp-config.pl` for future OpenLDAP test builds.)
  This also deletes existing self-declaration method via the
  CMake-specific `CURL_USE_OPENLDAP` configuration.

- cmake: define `LDAP_DEPRECATED=1` for OpenLDAP.
  Like autotools does. This fixes a long list of these warnings:
  ```
  /usr/local/opt/openldap/include/ldap.h:1049:5: warning: 'LDAP_DEPRECATED' is not defined, evaluates to 0 [-Wundef]
  ```

- cmake: delete LDAP TODO comment no longer relevant.

Also:

- autotools: replace domain name `dummy` with `0.0.0.0` in LDAP feature
  detection functions.

Ref: #11964 (effort to sync cmake detections with autotools)

Closes #12024
vszakats added a commit to vszakats/curl that referenced this pull request Oct 8, 2023
Syncing this up with CMake.

Source code uses the built-in `OPENSSL_IS_AWSLC` and
`OPENSSL_IS_BORINSSL` macros to detect BoringSSL and AWS-LC. No help
is necessary from the build tools.

autotools detects this anyway for display purposes.
CMake detects this to decide whether to use the BoringSSL-specific
crypto lib with ngtcp2. It detects AWS-LC, but doesn't use the
detection results just yet.

Ref: curl#11964

Closes #xxxxx
vszakats added a commit that referenced this pull request Oct 8, 2023
Syncing this up with CMake.

Source code uses the built-in `OPENSSL_IS_AWSLC` and
`OPENSSL_IS_BORINSSL` macros to detect BoringSSL and AWS-LC. No help is
necessary from the build tools.

The one use of `HAVE_BORINGSSL` in the source turned out to be no longer
necessary for warning-free BoringSSL + Schannel builds. Ref: #1610 #2634

autotools detects this anyway for display purposes.
CMake detects this to decide whether to use the BoringSSL-specific
crypto lib with ngtcp2. It detects AWS-LC, but doesn't use the detection
result just yet (planned in #12066).

Ref: #11964

Reviewed-by: Daniel Stenberg
Reviewed-by: Jay Satiro
Closes #12065
vszakats added a commit that referenced this pull request Oct 12, 2023
Fix `HAVE_H_ERRNO_ASSIGNABLE` to not run, only compile its test snippet,
aligning this with autotools. This fixes an error when doing
cross-builds and also actually detects this feature. It affected systems
not allowlisted into this, e.g. SerenityOS.

We used this detection result to enable `HAVE_GETADDRINFO_THREADSAFE`.

Follow-up to 04a3a37 #11979
Ref: #12095 (closed in favour of this patch)
Ref: #11964 (effort to sync cmake detections with autotools)

Reported-by: Kartatz on Github
Assisted-by: Kartatz on Github
Fixes #12093
Closes #12094
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build CI Continuous Integration cmake
Development

Successfully merging this pull request may close these issues.

None yet

2 participants