Skip to content

cmake: fix to use variable for the curl namespace#11629

Closed
vszakats wants to merge 1 commit intocurl:masterfrom
vszakats:cmake-dual-fixup
Closed

cmake: fix to use variable for the curl namespace#11629
vszakats wants to merge 1 commit intocurl:masterfrom
vszakats:cmake-dual-fixup

Conversation

@vszakats
Copy link
Member

@vszakats vszakats commented Aug 8, 2023

Replace (wrong) literal with a variable to specify the curl
namespace.

Follow-up to 1199308 #11505

Reported-by: balikalina on Github
Fixes 1199308#r123923098
Closes #11629

Follow-up to 1199308 curl#11505

Reported-by: balikalina on Github
Fixes curl@1199308#r123923098
Closes #xxxxx
@vszakats vszakats added the cmake label Aug 8, 2023
@github-actions github-actions bot added the build label Aug 8, 2023
vszakats referenced this pull request Aug 8, 2023
This patch adds the ability to build a static and shared libcurl library
in a single build session. It also adds an option to select which one to
use when building the curl executable.

New build options:
- `BUILD_STATIC_LIBS`. Default: `OFF`.
  Enabled automatically if `BUILD_SHARED_LIBS` is `OFF`.
- `BUILD_STATIC_CURL`. Default: `OFF`.
  Requires `BUILD_STATIC_LIBS` enabled.
  Enabled automatically if building static libcurl only.
- `STATIC_LIB_SUFFIX`. Default: empty.
- `IMPORT_LIB_SUFFIX`. Default: `_imp` if implib filename would collide
  with static lib name (typically with MSVC) in Windows builds.
  Otherwise empty.

Also:

- Stop setting the `CURL_STATICLIB` macro via `curl_config.h`, and pass
  it directly to the compiler. This also allows to delete a condition
  from `tests/server/CMakeLists.txt`.

- Complete a TODO by following the logic used in autotools (also for
  `LIBCURL_NO_SHARED`), and set `-DCURL_STATICLIB` in `Cflags:` of
  `libcurl.pc` for _static-only_ curl builds.

- Convert an existing CI test to build both shared and static libcurl.

Closes #11505
@vszakats vszakats changed the title cmake: use variable to set the namespace cmake: fix to use variable for the curl namespace Aug 9, 2023
@vszakats vszakats closed this in 121e60b Aug 9, 2023
@vszakats vszakats deleted the cmake-dual-fixup branch August 9, 2023 12:04
@balikalina
Copy link

balikalina commented Aug 9, 2023

Some projects need to export alias
add_library(ALIAS) in config file in some cases cause an error

CMake Error at ***/build/parts/curl/install/lib64/cmake/CURL/CURLConfig.cmake:62 (add_library):
[25/26]   add_library cannot create ALIAS target "CURL::libcurl" because target
[25/26]   "CURL::libcurl_static" is imported but not globally visible.
[25/26] Call Stack (most recent call first):
[25/26]   /usr/share/cmake3/Modules/FindCURL.cmake:58 (find_package)
[25/26]   pull/tests/integration/CMakeLists.txt:29 (find_package)
[25/26] -- Found CURL: ***/build/parts/curl/install/lib64/cmake/CURL/CURLConfig.cmake (found version "8.3.0-DEV")  
[25/26] CMake Error at ***/build/parts/curl/install/lib64/cmake/CURL/CURLConfig.cmake:62 (add_library):
[25/26]   add_library cannot create ALIAS target "CURL::libcurl" because target
[25/26]   "CURL::libcurl_static" is imported but not globally visible.
[25/26] Call Stack (most recent call first):
[25/26]   /usr/share/cmake3/Modules/FindCURL.cmake:58 (find_package)
[25/26]   push/CMakeLists.txt:2 (find_package)

there is an issue about exporting the alias
https://gitlab.kitware.com/cmake/cmake/-/issues/18996

so could you consider please set property EXPORT_NAME instead of add_library(ALIAS)?

line 222 lib/CMakeLists.txt

add_library(${LIB_NAME} ALIAS ${LIB_SELECTED})
add_library(${PROJECT_NAME}::${LIB_NAME} ALIAS ${LIB_SELECTED})

#export libcurl alias as target name
set_property(TARGET ${LIB_SELECTED}
  PROPERTY
  EXPORT_NAME ${LIB_NAME})

if(CURL_ENABLE_EXPORT_TARGET)

if added, must remove the

add_library(@PROJECT_NAME@::libcurl ALIAS @PROJECT_NAME@::@LIB_SELECTED@)

from CMake/curl-config.cmake.in

@vszakats
Copy link
Member Author

vszakats commented Aug 9, 2023

Thank you, is this what you had in mind?: #11646

vszakats added a commit to vszakats/libssh2 that referenced this pull request Aug 9, 2023
Fixes:
```
[build] Make Error at /usr/local/lib/cmake/libssh2/libssh2-config.cmake:7 (add library):
[build]   add library cannot create ALIAS target "libssh2::libssh2" because target
[build]   "libssh2: :libssh2 static" is imported but not globally visible.
```

Reported-by: bilal614 on Github
Suggested-by: balikalina on Github
Ref: curl/curl#11629 (comment)
Ref: curl/curl#11646
Fixes: libssh2#1150
Closes #xxxx
@balikalina
Copy link

balikalina commented Aug 11, 2023

Thank you, is this what you had in mind?: #11646

yes, exactly
if it doesn't crash any other builds for other build systems/os
may be should take some pause while waiting for other opinions

@vszakats
Copy link
Member Author

There was an issue reported with this solution over at libssh2: libssh2/libssh2#1154 (comment)

With a difference suggestion for a fix.

@balikalina
Copy link

There was an issue reported with this solution over at libssh2: libssh2/libssh2#1154 (comment)

With a difference suggestion for a fix.

if I move setting property EXPORT_NAME from lib/CMakeLists.txt to CMake/curl-config.cmake.in in a such way

include("${CMAKE_CURRENT_LIST_DIR}/@TARGETS_EXPORT_NAME@.cmake")
check_required_components("@PROJECT_NAME@")

# Alias for either shared or static library
#add_library(@PROJECT_NAME@::libcurl ALIAS @PROJECT_NAME@::@LIB_SELECTED@)
set_property(TARGET @PROJECT_NAME@::@LIB_SELECTED@ PROPERTY EXPORT_NAME @PROJECT_NAME@::libcurl)

receive error:

[25/26] CMake Error at ***/build/parts/curl/install/lib64/cmake/CURL/CURLConfig.cmake:63 (set_property):
[25/26]   EXPORT_NAME property can't be set on imported targets
[25/26]   ("CURL::libcurl_static")

@balikalina
Copy link

balikalina commented Aug 11, 2023

both CMakefiles.txt add target library (example for static)

add_library(${LIB_STATIC} STATIC ${SOURCES})

then libcurl declares aliases

add_library(${LIB_NAME} ALIAS ${LIB_SELECTED})
add_library(${PROJECT_NAME}::${LIB_NAME} ALIAS ${LIB_SELECTED})

when ssh2 doesn't do this

and the last they both install target

so the only difference is the absence of ALIAS add_library() call in CMakeLists.txt

vszakats added a commit to vszakats/curl that referenced this pull request Aug 11, 2023
Fixes:
```
[25/26] -- Found CURL: ***/build/parts/curl/install/lib64/cmake/CURL/CURLConfig.cmake (found version "8.3.0-DEV")
[25/26] CMake Error at ***/build/parts/curl/install/lib64/cmake/CURL/CURLConfig.cmake:62 (add_library):
[25/26]   add_library cannot create ALIAS target "CURL::libcurl" because target
[25/26]   "CURL::libcurl_static" is imported but not globally visible.
```

Follow-up to 1199308 curl#11505

Reported-by: balikalina on Github
Suggested-by: balikalina on Github
Ref: curl#11629 (comment)
Closes #xxxxx
@vszakats
Copy link
Member Author

Thanks for investigating @balikalina and pointing to these bits.

I read docs and came up with a minimal test, but the bad news is that I couldn't reproduce, and all patch suggestions break the test, while the master build works OK. I tested this with libssh2 only, because based on the CMake error message, this looks like the same issue in both libcurl and libssh2 (despite minor differences in their CMakeLists.txts).

I closed the related PR, but feel free to discuss / investigate further in #11646.

ptitSeb pushed a commit to wasix-org/curl that referenced this pull request Sep 25, 2023
Replace (wrong) literal with a variable to specify the curl
namespace.

Follow-up to 1199308 curl#11505

Reported-by: balikalina on Github
Fixes curl@1199308#r123923098
Closes curl#11629
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.

2 participants