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

Use fmt_8 from nixpkgs instead of building it from source #9131

Closed
wants to merge 3 commits into from

Conversation

Atry
Copy link
Contributor

@Atry Atry commented Jul 21, 2022

Currently we build fmt from source unconditionally, and our CMake file to build fmt is not compatible with nix's clang environment. An error about cannot find -lc++: No such file or directory would be reported when trying to build the bundled fmt with clang in #9129. See https://github.com/facebook/hhvm/actions/runs/3095588732/jobs/5017341496 for the build log.

This diff modifies third-party/CMakeLists.txt, using find_package to detect fmt from the build environment, or building the bundled fmt from source if no fmt from the build environment is found.

This PR also specifies fmt_8 nix package, given that fmt in nixpkgs unstable is version 7 while we previously build HHVM with a bundled version 8 of fmt.

Test Plan:

Rebase #9129 onto this diff, and there will be no error about cannot find -lc++: No such file or directory

@facebook-github-bot
Copy link
Contributor

@Atry has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@Atry has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@Atry has updated the pull request. You must reimport the pull request before landing.

@Atry Atry force-pushed the fmt_8-from-nixpkgs branch from 2a5f957 to 6a8658c Compare September 21, 2022 16:57
@facebook-github-bot
Copy link
Contributor

@Atry has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@Atry has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

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

@Atry has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@Atry has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@Atry has updated the pull request. You must reimport the pull request before landing.

@Atry Atry force-pushed the fmt_8-from-nixpkgs branch from bab4978 to 1f147f7 Compare September 21, 2022 17:43
@facebook-github-bot
Copy link
Contributor

@Atry has updated the pull request. You must reimport the pull request before landing.

@Atry Atry force-pushed the fmt_8-from-nixpkgs branch from 1f147f7 to 1ff8f2a Compare September 21, 2022 17:51
@facebook-github-bot
Copy link
Contributor

@Atry has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@Atry has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@Atry Atry marked this pull request as ready for review September 21, 2022 18:19
facebook-github-bot pushed a commit that referenced this pull request Sep 23, 2022
Summary:
We tried to use fmt_8 from nix package in #9131, however it did not take effect because the target created by find_package is `fmt::fmt`, not `fmt`.

This PR properly detect `fmt::fmt`, and create a `fmt` as a proxy to `fmt::fmt`.

Pull Request resolved: #9223

Test Plan:
Run GitHub Action. The following line should be included in the build log:
```
hhvm> -- Using system fmt: /nix/store/qcgg8plb11j9dcz4r22hsaps2qfq8bnm-fmt-8.1.1-dev/lib/cmake/fmt
```

See https://github.com/facebook/hhvm/actions/runs/3110469220/jobs/5041685489

Reviewed By: Wilfred

Differential Revision: D39760260

Pulled By: Atry

fbshipit-source-id: 5fa9b7b50560499c6162b06720ba63e33460f9f9
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.

2 participants