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

cmake: add CURL_USE_GSASL option with detection + CI test #13948

Closed
wants to merge 9 commits into from

Conversation

talregev
Copy link
Contributor

@talregev talregev commented Jun 14, 2024

Add gsasl feature to cmake + ci test

Copy link
Member

@vszakats vszakats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be useful if passing -DUSE_GSASL=ON would look up the dependency and setup lib, header path and all. But for now there is no CMakeLists.txt logic at all using this variable. Just forwarding this CMake setting to the compiler would not add much value and has a risk of confusion.

Can you extend it to detect gsasl, and pass USE_GSASL=1 to the compiler when present? To match existing practice, I suggest using CURL_USE_GSASL as the CMake-level option name.

@talregev
Copy link
Contributor Author

This would be useful if passing -DUSE_GSASL=ON would look up the dependency and setup lib, header path and all. But for now there is no CMakeLists.txt logic at all using this variable. Just forwarding this CMake setting to the compiler would not add much value and has a risk of confusion.

Can you extend it to detect gsasl, and pass USE_GSASL=1 to the compiler when present? To match existing practice, I suggest using CURL_USE_GSASL as the CMake-level option name.

I am planning to add this functionality to the vcpkg. (I already have a draft branch for that) But in vcpkg it will be detected with pkg-config not like you do here on curl. I only verify it with vcpkg tool.
So in this PR i try to minimize the patches that I will need to do for vcpkg. Similar PR to what I did to librtmp.

@vszakats
Copy link
Member

I see. Do you need to patch curl to do this though? In curl-for-win builds the CMAKE_C_FLAGS method worked well for these cases, e.g. -DCMAKE_C_FLAGS=-DUSE_GSASL.

That said I think any kind of gsasl detection would be an improvement at this point. pkgconfig is also fine, there is nothing saying we cannot do that.

@talregev
Copy link
Contributor Author

That said I think any kind of gsasl detection would be an improvement at this point. pkgconfig is also fine, there is nothing saying we cannot do that.

I will add with pkg-config detection. Thank you!

@talregev
Copy link
Contributor Author

talregev commented Jun 14, 2024

I see. Do you need to patch curl to do this though? In curl-for-win builds the CMAKE_C_FLAGS method worked well for these cases, e.g. -DCMAKE_C_FLAGS=-DUSE_GSASL.

For other pckges detection vcpkg patch curl to detect via pkg-config. Do you want to know what patches are used?
In that said, I am not the maintainer of vcpkg. I am a user that contribute to vcpkg.

vcpkg is not just for windows. It for linux, osx, android, and optional like ios, wasm32, mingw and more.

@vszakats
Copy link
Member

vszakats commented Jun 14, 2024

I see. Do you need to patch curl to do this though? In curl-for-win builds the CMAKE_C_FLAGS method worked well for these cases, e.g. -DCMAKE_C_FLAGS=-DUSE_GSASL.

For other pckges detection vcpkg patch curl to detect via pkg-config. Do you want to know what patches are used? In that said, I am not the maintainer of vcpkg. I am a user that contribute to vcpkg.

vcpkg is not just for windows. It for linux, osx, android, and optional like ios, wasm32, mingw and more.

I can see them, yes.

To start with them, we'd need to identify what is non-vcpkg specific, then understand the context and the problems they fix (unless obvious). Then adapt them to work outside the vcpkg framework.

Some minor stuff can be cherry-picked as-is.
Some changes I don't readily grok, e.g. dropping the _imp suffix.
It also uses hacks like elseif(0).

It'd be nice to implement the generic parts here, but it's a non-trivial amount of work just to understand what's being done and why, then (re)apply those for all envs.

I can acknowledge that CMakes dependency detection has multiple issues (some mentioned here) and it's also out of sync with autotools (which also has issues FWIW).

@talregev
Copy link
Contributor Author

talregev commented Jun 14, 2024

I see. Do you need to patch curl to do this though? In curl-for-win builds the CMAKE_C_FLAGS method worked well for these cases, e.g. -DCMAKE_C_FLAGS=-DUSE_GSASL.

For other pckges detection vcpkg patch curl to detect via pkg-config. Do you want to know what patches are used? In that said, I am not the maintainer of vcpkg. I am a user that contribute to vcpkg.
vcpkg is not just for windows. It for linux, osx, android, and optional like ios, wasm32, mingw and more.

I can see them, yes.

To start with them, we'd need to identify what is non-vcpkg specific, then understand the context and the problems they fix (unless obvious). Then adapt them to work outside the vcpkg framework.

Some minor stuff can be cherry-picked as-is. Some changes I don't readily grok, e.g. dropping the _imp suffix. It also uses hacks like elseif(0).

It'd be nice to implement the generic parts here, but it's a non-trivial amount of work just to understand what's being done and why, then (re)apply those for all envs.

I can acknowledge that CMakes dependency detection has multiple issues (some mentioned here) and it's also out of sync with autotools (which also has issues FWIW).

You can start an issue or dissection on this topic in vcpkg and you can get the help for the understanding and even suggestions and maybe more.
I am also not understand everting that inside the patches.

@talregev talregev requested a review from vszakats June 15, 2024 15:52
Copy link
Member

@vszakats vszakats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you say of adding gsasl to the feature-list assembled in CMakeLists?:

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -1644,6 +1644,7 @@ if(NOT CURL_DISABLE_INSTALL)
   _add_if("UnixSockets"   USE_UNIX_SOCKETS)
   _add_if("libz"          HAVE_LIBZ)
   _add_if("brotli"        HAVE_BROTLI)
+  _add_if("gsasl"         USE_GSASL)
   _add_if("zstd"          HAVE_ZSTD)
   _add_if("AsynchDNS"     USE_ARES OR USE_THREADS_POSIX OR USE_THREADS_WIN32)
   _add_if("IDN"           HAVE_LIBIDN2 OR USE_WIN32_IDN OR USE_APPLE_IDN)

@talregev
Copy link
Contributor Author

What do you say of adding gsasl to the feature-list assembled in CMakeLists?:

That nice. How it effect the build?
I will add this.

@talregev talregev requested a review from vszakats June 15, 2024 16:10
@vszakats
Copy link
Member

What do you say of adding gsasl to the feature-list assembled in CMakeLists?:

That nice. How it effect the build? I will add this.

It includes the list of features in the generated curl-config. Then test1014 verifies if this list matches with curl -V output.

What do you say of enabling gsasl in one of the CMake CI jobs to put this (and a gsasl build) to the test?

@github-actions github-actions bot added the CI Continuous Integration label Jun 15, 2024
@talregev talregev force-pushed the TalR/use_gsasl branch 4 times, most recently from 11fc727 to 93afdd1 Compare June 15, 2024 17:43
@talregev
Copy link
Contributor Author

talregev commented Jun 15, 2024

What do you say of adding gsasl to the feature-list assembled in CMakeLists?:

That nice. How it effect the build? I will add this.

It includes the list of features in the generated curl-config. Then test1014 verifies if this list matches with curl -V output.

What do you say of enabling gsasl in one of the CMake CI jobs to put this (and a gsasl build) to the test?

I try to do this on the ci, but it not easy to do as you already know. that why I use vcpkg to validate stuff, because it build you the stuff to the right place, and help you to detected the packages on any os you use it.
It can done compile the dependencies throw vcpkg, then use curl cmake to build curl. But this for another topic.

I tried now to compile the same as old linux, but to do it on new linux (ubuntu 22.04) because there is the libgsasl-dev package.
I put -DCURL_USE_GSASL=OFF to test what happening, but it not detected the other packages. Do you know why?

I will try only with gsasl ...

@talregev
Copy link
Contributor Author

talregev commented Jun 15, 2024

Only gslasl successfully build on New ubuntu ci. I can add one by one the other packages to see who is doing the problem, but before I do that, let me know what do you think.

The New Linux ci compile and successfully pass the tests. It prove that the gsasl package is correctly detected and build.

@talregev talregev changed the title Add USE_GSASL 1 Add gsasl feature to cmake + ci test Jun 15, 2024
@vszakats
Copy link
Member

vszakats commented Jun 15, 2024

I agree it's usually quite a bit of trouble to add new CI jobs. It also adds to the load.
In this case, modifying an existing seems to be enough to add the necessary coverage for this case:

--- a/.github/workflows/macos.yml
+++ b/.github/workflows/macos.yml
@@ -220,8 +220,8 @@ jobs:
           - CC: gcc-12
         build:
           - name: OpenSSL
-            install: nghttp2 openssl
-            generate: -DOPENSSL_ROOT_DIR=$(brew --prefix)/opt/openssl -DCMAKE_OSX_DEPLOYMENT_TARGET=10.9
+            install: nghttp2 openssl gsasl
+            generate: -DOPENSSL_ROOT_DIR=$(brew --prefix)/opt/openssl -DCMAKE_OSX_DEPLOYMENT_TARGET=10.9 -DCMAKE_USE_GSASL=ON
           - name: LibreSSL
             install: nghttp2 libressl
             generate: -DOPENSSL_ROOT_DIR=$(brew --prefix)/opt/libressl -DCURL_DISABLE_LDAP=ON -DCURL_DISABLE_LDAPS=ON -DBUILD_EXAMPLES=ON

@talregev
Copy link
Contributor Author

talregev commented Jun 15, 2024

In this case, modifying an existing seems to enough to add the necessary coverage for this case:

Should I keep the new ci I created?
I was planned to add many features for cmake on the new ubuntu.

@vszakats
Copy link
Member

I think it'd be better to tackle new CI workflows/jobs separately. For this PR I suggest updating an existing one.

@talregev
Copy link
Contributor Author

I updated the osx ci as you requested,
Also I want to keep the new ci that I was created. The problem was with libssh1 that it not linkage correctly with cmake on the new ubuntu.

@vszakats
Copy link
Member

I updated the osx ci as you requested, Also I want to keep the new ci that I was created. The problem was with libssh1 that it not linkage correctly with cmake on the new ubuntu.

I still find it better to keep that separate. It has nothing to do with this PR. As a copy-paste of linux-old.yml, it has a bunch of logic specific to the old Linux. Is that needed for Ubuntu 22.04? ubuntu-latest is 22.04, why launch a container with the same ubuntu version? I think this would better be addressed inside the existing linux.yml. Either way we should not mix up those changes with this PR, it makes this thread confusing.

@talregev
Copy link
Contributor Author

I agree, you are right. I will remove the new ci. Thank you for your comments and review.

@talregev talregev requested a review from vszakats June 16, 2024 01:51
@talregev
Copy link
Contributor Author

I fix it.

@vszakats vszakats changed the title Add gsasl feature to cmake + ci test cmake: add CURL_USE_GSASL with detection + CI test Jun 16, 2024
@vszakats vszakats changed the title cmake: add CURL_USE_GSASL with detection + CI test cmake: add CURL_USE_GSASL option with detection + CI test Jun 16, 2024
@bagder bagder closed this in 66bf995 Jun 17, 2024
@bagder
Copy link
Member

bagder commented Jun 17, 2024

Thanks!

@talregev talregev deleted the TalR/use_gsasl branch June 17, 2024 21:24
@vszakats
Copy link
Member

Thank you @talregev.

BillyONeal pushed a commit to microsoft/vcpkg that referenced this pull request Jun 27, 2024
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

3 participants