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

Improvements to CMake when LibSSH is not found #6

Closed
wants to merge 2 commits into from

Conversation

jktjkt
Copy link
Contributor

@jktjkt jktjkt commented Jul 26, 2016

...by switching from a handcrafted module to pkg-config.

With the original cmake 2.8.11 on CentOS 7.2.1511, a missing LibSSH was
not reported through a usual "required package XYZ not found" message,
but it generated a bit unintuitive error:

  CMake Error: The following variables are used in this project, but they are set to NOTFOUND.
  Please set them or make sure they are set and tested correctly in the CMake files:
  LIBSSH_INCLUDE_DIR
     used as include directory in directory /home/centos/libnetconf2
 (...)
     used as include directory in directory /home/centos/libnetconf2/tests

I suspect that this happens because the custom FindLibSSH.cmake doesn't
set LIBSSH_FOUND to TRUE on success (on my system).

According to rpmfind.net [1], the required pkg-config files have been
shipping since at least the release which is available in EL6's EPEL.

[1] https://www.rpmfind.net/linux/rpm2html/search.php?query=pkgconfig(libssh)

...by switching from a handcrafted module to pkg-config.

With the original cmake 2.8.11 on CentOS 7.2.1511, a missing LibSSH was
not reported through a usual "required package XYZ not found" message,
but it generated a bit unintuitive error:

  CMake Error: The following variables are used in this project, but they are set to NOTFOUND.
  Please set them or make sure they are set and tested correctly in the CMake files:
  LIBSSH_INCLUDE_DIR
     used as include directory in directory /home/centos/libnetconf2
 (...)
     used as include directory in directory /home/centos/libnetconf2/tests

I suspect that this happens because the custom FindLibSSH.cmake doesn't
set LIBSSH_FOUND to TRUE on success (on my system).

According to rpmfind.net [1], the required pkg-config files have been
shipping since at least the release which is available in EL6's EPEL.

[1] https://www.rpmfind.net/linux/rpm2html/search.php?query=pkgconfig(libssh)
This is what some random googling returned as a preferred variant. I
looked into this when investigating a missing find_package call before I
realized that this actually comes from a standard libc.
@michalvasko
Copy link
Member

Hi,
if you have the correct libssh package installed then the FindLibSSH.cmake module should definitely find it. The error may not be perfect, but it should be obvious what the problem is. The CMake module is written by the libssh team and I don't really see a reason why not to use it.

Regarding skipping -l in the additional linked libraries, are you saying it did not work for you the way it is?

Regards,
Michal

@jktjkt
Copy link
Contributor Author

jktjkt commented Jul 26, 2016

if you have the correct libssh package installed then the FindLibSSH.cmake module should definitely find it

Correct; there's no problem if that library is already installed.

The CMake module is written by the libssh team and I don't really see a reason why not to use it.

It doesn't conform to CMake's conventions. A FindFoo.cmake should always set Foo_FOUND to TRUE if that library is available. This bundled file does not set that variable on success -- it only sets it upon failure, which is why the find_package(... REQUIRED) doesn't error out early, and why the cmake process only gets interrupted by trying to use an undefined variable. As you said, a file coming form libssh means that this is a libssh bug. As this case demonstrates, a pkg-config approach would eliminate this error. The decision is yours for sure.

Regarding skipping -l in the additional linked libraries, are you saying it did not work for you the way it is?

Nope, it works already. I changed it because I was investigating another issue, it's just a followup cleanup that I split from my original commit (unpushed) for clarity. Feel free to ignore that one if you don't like it -- I was removing that hardcoded and redundant -lssh_threads so I cleaned up the rest as well.

@michalvasko
Copy link
Member

Hi,
it seems to me all this just polishes the build script, but does not fix any actual errors. So thanks for the effort, I will consider all this in the future when I have time for such minor fixes of the whole project, but I will not merge it now.

Regards,
Michal

jktjkt added a commit to jktjkt/libnetconf2 that referenced this pull request Nov 21, 2016
On my version of OpenSSL (openssl-1.0.2j on Gentoo Linux), the original
version of this code resulted in a small memleak reoprted by ASAN:

  Direct leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0x4d5460 in malloc /var/tmp/portage/sys-devel/llvm-3.9.0/work/llvm-3.9.0.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64
    CESNET#1 0x7f8fff9cb7fc in CRYPTO_malloc /var/tmp/portage/dev-libs/openssl-1.0.2j/work/openssl-1.0.2j-abi_x86_64.amd64/crypto/mem.c:346
    CESNET#2 0x7f8fffcd372b in load_builtin_compressions /var/tmp/portage/dev-libs/openssl-1.0.2j/work/openssl-1.0.2j-abi_x86_64.amd64/ssl/ssl_ciph.c:487
    CESNET#3 0x7f8fffcd5b5a in SSL_COMP_get_compression_methods /var/tmp/portage/dev-libs/openssl-1.0.2j/work/openssl-1.0.2j-abi_x86_64.amd64/ssl/ssl_ciph.c:1962
    CESNET#4 0x7f8fffcdc6b1 in SSL_library_init /var/tmp/portage/dev-libs/openssl-1.0.2j/work/openssl-1.0.2j-abi_x86_64.amd64/ssl/ssl_algs.c:150
    CESNET#5 0x7f90005ea466 in nc_ssh_tls_init /home/jkt/work/prog/libnetconf2/build/../src/session.c:1165:5
    CESNET#6 0x7f90005ea448 in nc_init /home/jkt/work/prog/libnetconf2/build/../src/session.c:1209:5
    CESNET#7 0x7f90005fe7c5 in nc_server_init /home/jkt/work/prog/libnetconf2/build/../src/session_server.c:430:5
    CESNET#8 0x50ffd8 in main /home/jkt/work/prog/libnetconf2/build/../tests/test_fd_comm.c:335:5
    CESNET#9 0x7f8ffe93e733 in __libc_start_main /var/tmp/portage/sys-libs/glibc-2.22-r4/work/glibc-2.22/csu/libc-start.c:289
    CESNET#10 0x419e18 in _start (/home/jkt/work/prog/libnetconf2/build/tests/test_fd_comm+0x419e18)

Upstream OpenSSL's issue tracker suggests [1] to use
SSL_COMP_add_compression_method [2]. That particular overload was only
added in 1.0.2, and since 1.1.0, it's deprecated and documented [3] to
be a no-op. RHEL6 comes with 1.0.1.

TL;DR: it's an API-compatibility mess, but it fixes a leak. It shouldn't
break the build.

[1] https://rt.openssl.org/Ticket/Display.html?id=2561&user=guest&pass=guest
[2] https://www.openssl.org/docs/man1.0.2/ssl/SSL_COMP_add_compression_method.html
[3] https://www.openssl.org/docs/man1.1.0/ssl/SSL_COMP_add_compression_method.html
jktjkt added a commit to jktjkt/libnetconf2 that referenced this pull request Nov 21, 2016
On my version of OpenSSL (openssl-1.0.2j on Gentoo Linux), the original
version of this code resulted in a small memleak reoprted by ASAN:

  Direct leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0x4d5460 in malloc /var/tmp/portage/sys-devel/llvm-3.9.0/work/llvm-3.9.0.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64
    CESNET#1 0x7f8fff9cb7fc in CRYPTO_malloc /var/tmp/portage/dev-libs/openssl-1.0.2j/work/openssl-1.0.2j-abi_x86_64.amd64/crypto/mem.c:346
    CESNET#2 0x7f8fffcd372b in load_builtin_compressions /var/tmp/portage/dev-libs/openssl-1.0.2j/work/openssl-1.0.2j-abi_x86_64.amd64/ssl/ssl_ciph.c:487
    CESNET#3 0x7f8fffcd5b5a in SSL_COMP_get_compression_methods /var/tmp/portage/dev-libs/openssl-1.0.2j/work/openssl-1.0.2j-abi_x86_64.amd64/ssl/ssl_ciph.c:1962
    CESNET#4 0x7f8fffcdc6b1 in SSL_library_init /var/tmp/portage/dev-libs/openssl-1.0.2j/work/openssl-1.0.2j-abi_x86_64.amd64/ssl/ssl_algs.c:150
    CESNET#5 0x7f90005ea466 in nc_ssh_tls_init /home/jkt/work/prog/libnetconf2/build/../src/session.c:1165:5
    CESNET#6 0x7f90005ea448 in nc_init /home/jkt/work/prog/libnetconf2/build/../src/session.c:1209:5
    CESNET#7 0x7f90005fe7c5 in nc_server_init /home/jkt/work/prog/libnetconf2/build/../src/session_server.c:430:5
    CESNET#8 0x50ffd8 in main /home/jkt/work/prog/libnetconf2/build/../tests/test_fd_comm.c:335:5
    CESNET#9 0x7f8ffe93e733 in __libc_start_main /var/tmp/portage/sys-libs/glibc-2.22-r4/work/glibc-2.22/csu/libc-start.c:289
    CESNET#10 0x419e18 in _start (/home/jkt/work/prog/libnetconf2/build/tests/test_fd_comm+0x419e18)

Upstream OpenSSL's issue tracker suggests [1] to use
SSL_COMP_add_compression_method [2]. That particular overload was only
added in 1.0.2, and since 1.1.0, it's deprecated and documented [3] to
be a no-op. RHEL6 comes with 1.0.1.

TL;DR: it's an API-compatibility mess, but it fixes a leak. It shouldn't
break the build.

[1] https://rt.openssl.org/Ticket/Display.html?id=2561&user=guest&pass=guest
[2] https://www.openssl.org/docs/man1.0.2/ssl/SSL_COMP_add_compression_method.html
[3] https://www.openssl.org/docs/man1.1.0/ssl/SSL_COMP_add_compression_method.html
@tcnyambira tcnyambira mentioned this pull request Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants