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 OSS build #9564

Draft
wants to merge 62 commits into
base: master
Choose a base branch
from
Draft

Conversation

mszabo-wikia
Copy link
Contributor

@mszabo-wikia mszabo-wikia commented Jan 6, 2025

The HHVM OSS build has become nonfunctional over the past year or so (#9506, #9389). This patchset aims to get it working again.

Due to the number of changes required, I don't think this PR is feasible to land as-is—I'm opening it to provide an overview of the changes made and also to have a working OSS branch that's up to date with the current master. I'll be making individual PRs for the patches within this patchset if there's maintainer capacity to accommodate them.

This patchset does not yet fix every extension—notably, squangle and mcrouter still need fixes to properly compile for OSS, hash has added the blake3 hash which will require pulling in the corresponding library dependency and so on. This can be done as a followup.

It's been a tad finicky to get the build to work in GitHub Actions because the build as-is requires 30+ GiB of disk space, which is more than what the default runners have available. I'll see if the Meta-specific 8-core-ubuntu runner will be able to handle this PR, and if not, I'll change the CI to remove assorted garbage from the default runner prior to commencing the build.

I'd like to thank @kmapb and @Atry for their OSS build fixes that proved to be very helpful.

Below is a non-exhaustive list of the most important changes:

Changes made

  • C++20 is required due to the use of coroutines, ranges and assorted C++20 paraphernalia. (cb168b5)
  • As alluded to in the OSS support change blog post, HHVM is now built using clang internally. I've been unable to get it to build with either GCC 14 + libstdc++ 14 / clang 18 + libstdc++ 14, but a complete LLVM toolchain of clang 18 + libc++ 18 did the trick. In either case, at least libc++ 17 is required due to the dependency on std::lexicographical_compare_three_way introduced in ba8cf61.
  • Using libc++ also requires providing a way to force the compiler to use libc++ over the system default, which is likely to be libstdc++ on the majority of Linux systems. (8ce6dcf)
  • Building with libc++ also implies that any C++ library dependency whose ABI exposes C++ types must itself have been built against libc++, otherwise they won't link correctly. In practice, this means building C++ dependencies that could otherwise have been provided via system libraries (e.g. re2, boost, glog) from source, and ensuring that compiler flags are correctly forwarded to such subprojects. (d7982c7, 4374e14, bad51c2, def968d, 7058d8f)
  • ext_gd depends on libheif >= 1.16.0 since 80f15c3, which implies building on Ubuntu Noble or newer unless we want to vendor libheif as well.
  • Install the nightly build from 2024-11-26 which corresponds to the current stable version 1.83.0, because some Rust dependencies now require at least Rust 1.73.0 later. (984acef)
  • Update patches for other internal FB dependencies (e.g. folly and fizz) that have become outdated due to changes to those libraries. (6cb6189, c2fbba9)
  • Install new required dependencies for FB-internal library dependencies. Proxygen and thrift now require mvfst, so provide it as a submodule (presumably this can be converted to use the same mechanism used to keep folly etc. in sync with this repo). Fizz optionally depends on liboqs (with corresponding linking implications), while folly has a hard dependency on fast_float and xxhash. (b03fd39, 24b3e00, ad852cc)
  • Fix compatibility with fmtlib 11. (0ffb19a, 1115143).
  • Update outdated OCaml stubs. (be39b5a)
  • Update various listfiles in the core runtime and extensions that have become out of sync with the code.(235d811, 2fba118, 98d947b)
  • Add various missing #include directives that were causing compile errors. (01975f1, 34eefc6,14edad8afcaff5b4ffd78dd096ffc3da670e13c7, a19eee5, 3f07dc4).
  • Always build Intel® Xed because it is now a required dependency, and update its mbuild submodule to work with Python 3.8+. (fc5a561, 8122e8f)
  • Run Rust build steps after setting up OCaml and provide them with OPAM environment variables because the ocamlrep crate used in some places requires a working OPAM install. (4a72abb, 0e3ca63)
  • Update code generation logic for hhbc-unit.h. (ae086c0)
  • Compile Rust FFI libraries into a single Rust staticlib and link that into the rest of HHVM instead of the current approach of linking individual Rust staticlibs that does not work with cxx. (5639957)
  • Exclude code from ext/fb that depended on the Meta-specific FBSerialize header, because it got moved elsewhere—it's unlikely that this Meta-specific serialization implementation was being used by any third parties to begin with. (c6750ed)
  • Provide compatibility logic for post-2021 libdwarf versions that are packaged by some distributions like Fedora (but not Debian/Ubuntu derivatives as of Bookworm/Oracular): conditionally use the new replacement functions and include the libdwarfp header if so. (97ce3e3)
  • Support the new Rust code generator that turns configs.specification into configuration headers. (22dd7ac)
  • Update systemlib generation to work with systemlib precompilation introduced in 20118c8, and account for the core systemlib having been turned into an extension in 62bed4d. Raise a more helpful error for the likely case (in OSS) that a new extension's systemlib hasn't been wired up with the build system. (fd6fe22, 5c45ff9).
  • Fix tc-print directory inclusion order due to dependency on generated systemlibs (89264df).
  • Fix the libxml and soap extensions to work with libxml >= 2.12.0 that changed several functions to take a const xmlError* instead of xmlError*. (f0dfb05)
  • Apply @Atry 's unmerged patch from Fix compilation error with OpenSSL 3 #9288 to fix a compile error with OpenSSL 3.
  • Use a known custom runner label. (bca2570)

@facebook-github-bot
Copy link
Contributor

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. (Because this pull request was imported automatically, there will not be any future comments.)

mszabo-wikia added a commit to mszabo-wikia/hhvm that referenced this pull request Jan 6, 2025
These will be helpful both in the future and when applying parts of facebook#9564.

* Set up .editorconfig to ensure consistent indents.
* Instruct CMake to export a clangd JSON compilation database that IDEs
  can integrate with.
@facebook-github-bot
Copy link
Contributor

@mszabo-wikia has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@mszabo-wikia has updated the pull request. You must reimport the pull request before landing.

2 similar comments
@facebook-github-bot
Copy link
Contributor

@mszabo-wikia has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@mszabo-wikia has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@mszabo-wikia has updated the pull request. You must reimport the pull request before landing.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@mszabo-wikia has updated the pull request. You must reimport the pull request before landing.

@mszabo-wikia
Copy link
Contributor Author

Sorry about the comment spam, some last-minute CI fixes were in order. This now builds fine.

Going forward, I'm unconvinced of the benefits of building packages for PRs. I think it might be better to have a scheduled (nightly?) workflow for doing that instead, and have a lightweight workflow for pushes and PRs that would just compile HHVM and run tests.

facebook-github-bot pushed a commit that referenced this pull request Jan 7, 2025
Summary:
If a new extension systemlib isn't wired up with the build system, its unit emitters and decls won't be embedded in the HHVM binary, causing a cryptic crash when trying to run Hack code ("Invalid varint value: too few bytes."). One then has to fire up a debugger to determine what systemlib is missing.

Instead, raise a more informative error if a specific systemlib was not found in the binary.

Split from #9564.

Pull Request resolved: #9566

Reviewed By: ricklavoie

Differential Revision: D67874442

fbshipit-source-id: 876fe61ff08fd05a311659a44c2b0a19ea878f94
mszabo-wikia added a commit to mszabo-wikia/hhvm that referenced this pull request Jan 7, 2025
The bundled Rust version is too old for several library dependencies.
Update to the nightly build from November 26th, 2024, which corresponds
to the 1.83.0 release.

Split from facebook#9564.
@muglug
Copy link
Contributor

muglug commented Jan 8, 2025

Thanks for this — at Slack we are also very interested in this PR, which contains a few changes we've already independently applied to our local patch (which is at the moment forked off a mid-2023 version of upstream)

@mszabo-wikia
Copy link
Contributor Author

@muglug Thanks, glad to hear this! Let me know if you have any feedback or questions.

I've already started to cherry-pick some of the individual changes into PRs but will try to keep this branch periodically updated until the fixes land.

@lexidor
Copy link
Collaborator

lexidor commented Jan 8, 2025

@mszabo-wikia I too am very interested in this branch. I have built it locally. I am able to run Hack code with hhvm after adding openssl 1.1.1 for the ext_watchman autoloader. I am looking at hh_client (which fails with EOPNOTSUPP). The typechecker itself is alive, since hh_client check . can correctly report errors. Have you been able to type check a project using hh_client at commit 0c55ab3?

I have created a Dockerfile which builds hhvm of your branch. It is nothing spectacular, but it would allow anyone interested to build outside of GitHub CI in a reproducible environment. I will need to add the OpenSSL fix, an 'add to path' fix, and an 'install watchman' fix. I will share tomorrow after work, or if the build takes more time than I have available on a weekday, this weekend.

@mszabo-wikia
Copy link
Contributor Author

@mszabo-wikia I too am very interested in this branch. I have built it locally. I am able to run Hack code with hhvm after adding openssl 1.1.1 for the ext_watchman autoloader. I am looking at hh_client (which fails with EOPNOTSUPP). The typechecker itself is alive, since hh_client check . can correctly report errors. Have you been able to type check a project using hh_client at commit 0c55ab3?

I have created a Dockerfile which builds hhvm of your branch. It is nothing spectacular, but it would allow anyone interested to build outside of GitHub CI in a reproducible environment. I will need to add the OpenSSL fix, an 'add to path' fix, and an 'install watchman' fix. I will share tomorrow after work, or if the build takes more time than I have available on a weekday, this weekend.

Hmm, this seems to work okay for me in a trivial container (podman run -it --rm -v $PWD:/app --security-opt label=disable ubuntu:noble bash, with a test project mounted and HHVM_DISABLE_PERSONALITY=true + HHVM_DISABLE_NUMA=true ).

Perhaps there are some additional restrictions imposed for your particular container?

mszabo-wikia added a commit to mszabo-wikia/hhvm that referenced this pull request Jan 9, 2025
The bundled Rust version is too old for several library dependencies.
Update to the nightly build from October 13th, 2024, which corresponds
to the 1.83.0 release.

Split from facebook#9564.
facebook-github-bot pushed a commit that referenced this pull request Jan 9, 2025
Summary:
These will be helpful both in the future and when applying parts of #9564.

* Set up .editorconfig to ensure consistent indents.
* Instruct CMake to export a clangd JSON compilation database that IDEs can integrate with.

Pull Request resolved: #9565

Reviewed By: Wilfred

Differential Revision: D67870812

fbshipit-source-id: 8d1db6b9002e65e8cf52d0a0d005ed4fe49259cf
facebook-github-bot pushed a commit that referenced this pull request Jan 9, 2025
Summary:
The bundled Rust version is too old for several library dependencies. Update to the nightly build from November 26th, 2024, which corresponds to the 1.83.0 release.

Split from #9564.

Pull Request resolved: #9571

Reviewed By: dtolnay

Differential Revision: D67921964

fbshipit-source-id: 6e7f24e2e00ec45303356daa64a7d9a1b53f40fb
facebook-github-bot pushed a commit that referenced this pull request Jan 9, 2025
Summary:
libxml 2.12.0 changes the API to return `const xmlError*` in several places.[1] As a fix, use auto variables for storing the output of xmlGetLastError() and make the signature of our error handler function dependent on the libxml version.

Split from #9564.

[1] https://gitlab.gnome.org/GNOME/libxml2/-/blob/86401cc3d293d6ea3c4552885e3cadcd952021d1/NEWS#L392

Pull Request resolved: #9572

Reviewed By: Wilfred

Differential Revision: D67922353

fbshipit-source-id: 36eded2295fdd60d15ff559d4d79eb5732c5598f
mszabo-wikia and others added 28 commits January 16, 2025 02:24
Summary:
In OpenSSL 3, `EVP_PKEY_get0_*` functions now return `const` pointers, breaking existing usage depending on mutable pointers returned by OpenSSL 1.x. This diff fix the errors

1. Use `auto` type for return type of `EVP_PKEY_get0_*` functions instead of mutable pointer types.
1. Some `EVP_PKEY_get0_RSA` are replaced with `EVP_PKEY_get1_RSA` in order to get a mutable pointer with the ownership.

See https://www.openssl.org/docs/man3.0/man7/crypto.html#LIBRARY-CONVENTIONS for the `get_0` conventions

Differential Revision: D40942193

fbshipit-source-id: b9a868aec3d332e9de323cb801af52e2692d4686
@facebook-github-bot
Copy link
Contributor

@mszabo-wikia has updated the pull request. You must reimport the pull request before landing.

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

Successfully merging this pull request may close these issues.

5 participants