WebAssembly / wabt

The WebAssembly Binary Toolkit
Apache License 2.0
6.9k stars 702 forks source link

CMake missing OpenSSL dependency #2446

Closed alexreinking closed 3 months ago

alexreinking commented 4 months ago

When wabt depends on OpenSSL, it fails to find_dependency() the package in its config files. This can be observed when installing wabt via Homebrew; we get the error:

CMake Error at /opt/homebrew/lib/cmake/wabt/wabt-targets.cmake:61 (set_target_properties):
  The link interface of target "wabt::wabt" contains:

    OpenSSL::Crypto

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

This could be fixed by, e.g. adding the following snippet to scripts/wabt-config.cmake.in, just after @PACKAGE_INIT@:

if ("@HAVE_OPENSSL_SHA_H@")
    include(CMakeFindDependencyMacro)
    find_dependency(OpenSSL)
endif ()

If you fix this, pretty please cut a new release 🙏

keithw commented 4 months ago

Hmm, I'm confused by this report and couldn't immediately replicate it. We don't see to use find_dependency() anywhere in all of wabt. We do have find_package(OpenSSL QUIET) in CMakeLists.txt, but it does seem to work for me on a GNU/Linux machine with OpenSSL installed:

$ cmake -S . -B build
-- The C compiler identification is GNU 13.2.0
-- The CXX compiler identification is GNU 13.2.0

[...]
-- Looking for openssl/sha.h
-- Looking for openssl/sha.h - found
-- Using OpenSSL libcrypto for SHA-256
[...]
-- Configuring done (2.5s)
-- Generating done (0.1s)
-- Build files have been written to: /home/keithw/stanford/wabt/build

It also seems to work fine in CI on Mac (and finds the OpenSSL there and uses it).

We might want to just move from the current SHA-256 setup (which uses either external OpenSSL or vendored PicoSHA2) to a world where it's always vendored BLAKE3. Because that will probably lead to fewer questions about external dependencies. But I can't immediately replicate this report... :-/

keithw commented 4 months ago

What is the failure mode you see here -- does it fail to build? Or does it successfully fall back to the vendored/internal PicoSHA2?

alexreinking commented 4 months ago

This happens in downstreams (e.g. Halide, where I observed the issue) when going through:

find_package(wabt REQUIRED)
target_link_libraries(mylib PRIVATE wabt::wabt)

This doesn't affect the main build, which uses _find_package_ instead.

There's no test for this in CI, unfortunately. When testing out of the build directory, the OpenSSL libraries are already linked into the test apps.

keithw commented 4 months ago

Are you able to contribute a CI test that exercises this configuration so we can make sure not to break it? Honestly I think we should probably just eliminate the OpenSSL dependency entirely, but whatever we do should not break users like you.

alexreinking commented 4 months ago

I will be able to... eventually. I'm waiting for my company to approve contributing under the W3 CLA that the README indicates is required.

keithw commented 4 months ago

AFAIK we don't actually require or verify CG membership before merging PRs on this repo (and certainly not for something like a CI config that helps somebody).

alexreinking commented 3 months ago

Fixed by #2447