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.

Sign up for GitHub

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

Jump to bottom

cmake: extend zlib's AUTO option to brotli, zstd and enable if found #15431

Closed
wants to merge 4 commits into from
Closed

cmake: extend zlib's AUTO option to brotli, zstd and enable if found #15431

wants to merge 4 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Oct 28, 2024 •
edited
Loading

  • make curl_dependency_option() more generic.

  • extend CURL_BROTLI and CURL_ZSTD options to accept
    AUTO in addition to existing ON and OFF.

  • change CURL_BROTLI and CURL_ZSTD option default
    to AUTO. Was: OFF.
    It brings cmake behavior closer to ./configure.
    Still different:

    • ./configure defaults to off which means to check default
      locations. cmake checks more locations by default.
      (Also tried NO_CMAKE_PATH, but then it checked less locations.)
    • cmake returns both brotlicommon and brotlidec libs,
      while ./configure only returns the latter.

  • ci: drop explicit cmake options, that are now unnecessary.

  • GHA/configure-vs-cmake: make adjustments to make tests pass.

@vszakats vszakats added cmake feature-window A merge of this requires an open feature window labels Oct 28, 2024
@github-actions github-actions bot added the CI Continuous Integration label Oct 28, 2024
@vszakats vszakats force-pushed the cm-extend-AUTO-option-1 branch 3 times, most recently from fbdc10e to 403c331 Compare October 28, 2024 15:46
vszakats added a commit to vszakats/curl that referenced this pull request Oct 31, 2024
@vszakats
cmake: drop redundant FOUND checks
50a6ba4 
With `find_package(... REQUIRED)` the configuration fails and exits
if the package is not found. The `..._FOUND` check afterwards always
evaluates true and safe to delete.

Also true for brotli and zstd, but those are addressed differently
via curl#15431.
@vszakats vszakats mentioned this pull request Oct 31, 2024
Closed
@vszakats vszakats force-pushed the cm-extend-AUTO-option-1 branch 3 times, most recently from a332aab to 2c8ade3 Compare November 28, 2024 12:01
vszakats added a commit that referenced this pull request Dec 16, 2024
@vszakats
cmake: drop redundant FOUND checks (libgsasl, libssh, libuv)
05ba353 
With `find_package(... REQUIRED)` the configuration fails and exits
if the package is not found. The `..._FOUND` check afterwards always
evaluates true and safe to delete.

Also true for brotli and zstd, but those are addressed differently
via #15431.

Closes #15465
@vszakats vszakats changed the title cmake: extend AUTO option to brotli, zstd cmake: extend zlib's AUTO option to brotli, zstd Dec 16, 2024
@vszakats vszakats force-pushed the cm-extend-AUTO-option-1 branch from 2c8ade3 to 751e4a4 Compare December 17, 2024 01:38
@vszakats vszakats mentioned this pull request Dec 17, 2024
Closed
@vszakats
cmake: extend AUTO option to brotli, zstd
9300ed9 
Probably requires curl#15408
@vszakats
ci: drop unnecessary options enabling brotli/zstd
d934932
@vszakats
ci: drop unnecessary options enabling brotli/zstd 2
e20892c
@vszakats
try re-syncing configure-vs-cmake
69cda1b 
try re-syncing configure-vs-cmake disable brotli

cmake detects brotlicommon and brotlidec (which is a case of overlinking,
to avoid breaking builds where both static and shared libs are offered and
reading the shared libset from pkg-config, but ending up linking the static
libs by default.)
@vszakats vszakats force-pushed the cm-extend-AUTO-option-1 branch from 751e4a4 to 69cda1b Compare December 17, 2024 02:25
@vszakats vszakats changed the title cmake: extend zlib's AUTO option to brotli, zstd cmake: extend zlib's AUTO option to brotli, zstd and enable if found Dec 17, 2024
@vszakats
Copy link
Member Author

This triggered the revert of #15005, which enabled pkg-config detection for cmake MinGW cross-builds by default.

It's possible to fix without reverting, but it was a signal how fragile really pkg-config is in cases, MinGW cross-builds being one of the common one. Thus I chose to revert and avoid this class of problems.

@vszakats vszakats closed this in f2adb3b Dec 17, 2024
@vszakats vszakats deleted the cm-extend-AUTO-option-1 branch December 17, 2024 03:07
@vszakats vszakats mentioned this pull request Jan 19, 2025
Closed
5 tasks
vszakats added a commit that referenced this pull request Jan 20, 2025
@vszakats
GHA: add iOS jobs with LibreSSL, enable dependencies for Android via …
12a6de2 
…vcpkg

iOS:

- add jobs with autotools, CMake, CMake Xcode generator.
  The Xcode generator is >10x slower than Unix Makefiles. Keep it
  because it's the one recommended by CMake and for having its own
  quirks we may want to know about.
- build, cache and use LibreSSL for these jobs.
  With workaround for an iOS build issue fixed in master.
- make Xcode generator work by explicitly disabling code signing.
- make tests and examples build with the Xcode generator by setting
  `-DMACOSX_BUNDLE_GUI_IDENTIFIER=se.curl`, to avoid
  "Bundle identifier is missing" errors.
- cmake: disable `CURL_USE_PKGCONFIG` by default for Apple device.
- cmake: add `stdc++` library for BoringSSL and AWS-LC, with
  `OPENSSL_USE_STATIC_LIBS=ON` set.
- cmake: add workaround for Xcode generator issue, where it cannot
  handle two targets depending on one custom command. A better fix may
  be dropping `tool_hugehelp.c` and `tool_ca_embed.c` from curltool
  library. For a future PR.

Android:

- add vcpkg to Android jobs, enable dependencies.
  Assisted-by: Tal Regev via #16045
- make vcpkg work with autotools.
- pass `--with-brotli` to autotools to detect the vcpkg-supplied brotli.
- enable BoringSSL for Android and add a job with it.
- silence 457 CMake configure warnings about the Android NDK CMake
  scripts targeting freshly deprecated CMake versions.

These were much more involved than imagined. Basically nothing works out
of the box, and when combined, everything becomes a unique edge case.
autotools builds were a much easier to make work than CMake ones.

Also:

- GHA/non-native: re-sync names to be shorter and more aligned with
  other workflows.
- GHA: add `persist-credentials: false` where missing.

Unresolved issues:

- `OPENSSL_ROOT_DIR` ignored/mis-used when pointing it to LibreSSL.
  CMake seems to prepend the sysroot to the passed absolute directory.
  Found no workaround.
- CMake when combined with Android, both the Google-recommended method
  and the built-in CMake method fail to provide a way to avoid
  `pkg-config` packages at system directories. Failed to find a knob
  that can remove `/usr/include` from the search path. The workaround is
  to disable zstd. (I enabled it by default in this release, maybe
  premature?: f2adb3b #15431)
  Disabling `pkg-config` doesn't work because vcpkg dependencies do not
  link without it.
- CMake's Xcode generator is slow because each `try_compile()` feature
  check springs a new CMake + Xcode project taking a long time to run,
  just to compile single-liner C files. A known issue, with no solution.
  `-DCMAKE_MACOSX_BUNDLE=OFF` did not help, limiting build types to
  a single one (e.g. `Debug`) also had no effect.
   make | Xcode | GHA run
  :---- | :---- | :--------------------------------------------------------------------
    16s | 2m57s | https://github.com/curl/curl/actions/runs/12866334102/job/35868712426
    23s | 4m13s | https://github.com/curl/curl/actions/runs/12868128013/job/35874212461
    16s | 3m39s | https://github.com/curl/curl/actions/runs/12859073531/job/35849041880
    14s | 2m23s | https://github.com/curl/curl/actions/runs/12858298423/job/35847201313
    15s | 2m36s | https://github.com/curl/curl/actions/runs/12858058492/job/35846669761
    19s | 3m19s | https://github.com/curl/curl/actions/runs/12868919430/job/35876601168

Closes #16043
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CI Continuous Integration cmake feature-window A merge of this requires an open feature window
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@vszakats