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
Contributor

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

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 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))
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Contributor

dfandrich commented May 13, 2020 via email

@Lekensteyn
Copy link
Contributor Author

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!

@Lekensteyn
Copy link
Contributor Author

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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants