facebook / hhvm

A virtual machine for executing programs written in Hack.
https://hhvm.com
Other
18.19k stars 3k forks source link

update hack cmake to resolve build breakage on certain cmake versions #9303

Closed ssandler closed 1 year ago

ssandler commented 1 year ago

This resolves a build failure which occurs on Ubuntu 18.04 with cmake 3.10. It may occur on other distros and with other cmake versions as well. The error is:

16:25:25 [ 25%] Building CXX object hphp/hack/CMakeFiles/compiler_ffi.dir/src/hackc/ffi_bridge/external_decl_provider.cpp.o
16:25:25 In file included from /build/hhvm/hphp/hack/src/hackc/ffi_bridge/external_decl_provider.cpp:7:0:
16:25:25 /build/hhvm/hphp/hack/src/hackc/ffi_bridge/decl_provider.h:9:10: fatal error: hphp/hack/src/hackc/ffi_bridge/compiler_ffi.rs.h: No such file or directory
16:25:25  #include "hphp/hack/src/hackc/ffi_bridge/compiler_ffi.rs.h"

Using CMAKE_VERBOSE_MAKEFILE, the compilation command for external_decl_provider.cpp ends in:

-iquote /build/hhvm/hphp/hack/_build/rust_ffi/hphp/hack/src/hackc/ffi_bridge/cxxbridge /build/hhvm/hphp/hack/_build/rust_ffi -o CMakeFiles/compiler_ffi.dir/src/hackc/ffi_bridge/external_decl_provider.cpp.o -c /build/hhvm/hphp/hack/src/hackc/ffi_bridge/external_decl_provider.cpp

The -iquote option is deduplicated, but this deduplication breaks the compilation and the necessary directory is not included.

However, my understanding is that this -iquote was put in place from a time when HHVM was importing .rs files directly, and after this commit we shouldn't need to use target_compile_options() like this anymore and can use target_include_directories() instead.

I confirmed that this commit resolves that error.

facebook-github-bot commented 1 year ago

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

fredemmott commented 1 year ago

Test failure seems unrelated:

-iquote

To expand on this a little, -iquote means "consider these paths for #include "foo", not just #include <foo>"; it was used here because it has the side effect of making the include paths higher priority. In particular, it made BUILD_DIR/hphp/hack/src/.../foo.rs (a generated C++ header file) higher priority than SOURCE_DIR/hphp/hack/src/.../foo.rs (a rust source file). Now that HHVM's consistently using .rs.h includes instead of .rs, this prioritization is not needed and CMake's standard include directory management can be used instead.

facebook-github-bot commented 1 year ago

@Wilfred merged this pull request in facebook/hhvm@1512f9098c977aa5d6b5f24f80220cd8d111c318.