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

Fix build with pre-release libssh in Ubuntu 18.04, add libssh to CMake #5372

Closed
wants to merge 2 commits into from

Conversation

@Lekensteyn
Copy link
Member

Lekensteyn commented May 11, 2020

Ubuntu 18.04 is the only known distro version that ships with a pre-release version of libssh. Since they are unlikely to upgrade their libssh version, add a workaround to prevent build failures due to deprecation errors. This change is a dependency for #5370.

CMake support was added so I could easily test the libssh build.

cc @ansasaki

Lekensteyn added 2 commits May 10, 2020
Workaround to prevent -Werror=deprecated-declarations build failures.
@bagder
Copy link
Member

bagder commented May 12, 2020

@ansasaki can you give this libssh version check a glance and see if it makes sense to you?

@ansasaki
Copy link
Contributor

ansasaki commented May 12, 2020

@ansasaki can you give this libssh version check a glance and see if it makes sense to you?

Sure, I'll check

* libssh/sftp.h that was added after 0.7.0 to avoid deprecation warnings.
*/
#if (LIBSSH_VERSION_INT >= SSH_VERSION_INT(0, 8, 0)) || \
(LIBSSH_VERSION_INT == SSH_VERSION_INT(0, 7, 0) && defined(SSH_S_IFMT))

This comment has been minimized.

Copy link
@ansasaki

ansasaki May 12, 2020

Contributor

Wouldn't be better if the existence of the new symbol was checked during configuration and used if defined?

Maybe cmake's CheckSymbolExists or CheckFunctionExists could be used.

For autoconf it would require writing m4 macros to detect the presence of the new symbol.

This comment has been minimized.

Copy link
@bagder

bagder May 12, 2020

Member

Yes, except for builds using other methods than autotools and cmake but I'm pretty sure they're not using libssh to any large extent...

This comment has been minimized.

Copy link
@Lekensteyn

Lekensteyn May 12, 2020

Author Member

I'd prefer less feature detection checks in autotools/cmake rather than more since they slow down the configure/cmake step. Luckily this case can easily be detected by these preprocessor macros, so no symbol checks are needed.

This comment has been minimized.

Copy link
@Lekensteyn

Lekensteyn May 13, 2020

Author Member

Hi @ansasaki, do you have any objections on this patch? It is a blocker before #5370 can be merged.

This comment has been minimized.

Copy link
@ansasaki

ansasaki May 13, 2020

Contributor

Hi @Lekensteyn, I just think the workaround for the check is a bit "hacky", but if it works for you I won't object. Since it is checking for something that is not going to change, I believe it wouldn't create problems for the future. As you mentioned, the workaround does not increase the configuration time and avoids having to write checks for cmake, autotools, and a fall back when none of them are available, which can be considered arguments in favour of it.

This comment has been minimized.

Copy link
@Lekensteyn

Lekensteyn May 13, 2020

Author Member

I agree it is a bit hacky, but I thought it is the best solution available to ensure that CI does not fail.

Some of the options are:

  • Special-case a known bad version and detect it with macros. Unlikely to break older or newer versions since the version check is fixed. Implemented by this patch.
  • Set -Wno-error or -Wno-deprecation for the affected file. This may hide other errors in the same file.
  • Add even more lines of code to configure.ac/CMakeLists.txt to perform the symbol check. May be more complex than worth it.
  • Ask Ubuntu developers to update their packages. Since both functions are functionally equivalent and it is "just" a compiler warning, I don't see them pushing a "fix" anytime soon.
  • Accept that libssh on Ubuntu 18.04 is an uncommon configuration. libssh2 is available and so are newer Ubuntu versions, including 20.04. To fix CI, patch the header as workaround: sed '/ ssh_get_publickey(/s/^SSH_DEPRECATED //' /usr/include/libssh/libssh.h -i. Unfortunately, this patch would be needed for both Travis CI and Azure Pipelines as they only offer 18.04 as latest option (Travis CI - Linux, Azure ubuntu-latest).

Given these, it seems that the first option achieves the desired objective (no CI failures) with the least complexity.

@dfandrich
Copy link
Collaborator

dfandrich commented May 13, 2020

@Lekensteyn
Copy link
Member Author

Lekensteyn commented May 14, 2020

Travis now offers 20.04.

This is not documented anywhere (blog, changelog, docs), but indeed 20.04 support appears to be work in progress in the travis-build, docs-travis-ci-com, and travis-yml repos (search for "focal"). If that is available, I'll drop the version check here!

@bagder
bagder approved these changes May 14, 2020
@Lekensteyn
Copy link
Member Author

Lekensteyn commented May 15, 2020

I've applied the cmake change, but dropped the libssh.c change in favor of using focal with a newer libssh version in CI. Thanks for your feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.