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 compilation two issues #194

Closed
wants to merge 4 commits into from
Closed

fix compilation two issues #194

wants to merge 4 commits into from

Conversation

1a1a11a
Copy link

@1a1a11a 1a1a11a commented Feb 18, 2023

  1. remove cmake from Ubuntu apt installation because the cmake installed from apt (both Ubuntu18 and Ubuntu20) is too old to be used, and the new cmake from apt shadows existing cmake binary.
  2. current compilation cannot use multiple cores even though -j is specified due to a bug in using nproc.

1a1a11a and others added 3 commits February 18, 2023 19:31
`CheckLinkerFlag` requires CMake 3.18, but the default cmake in Ubuntu20 is 3.16, installing the default often shadow the customized cmake
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 18, 2023
@therealgymmy
Copy link
Contributor

if we remove cmake from dependency, do we expect users to have installed cmake from a newer source?

@wonglkd
Copy link
Contributor

wonglkd commented Feb 22, 2023

FYI @1a1a11a , re cmake being too old: I'm not sure if your cmake issue was the same as mine, but I did face a compilation issue on Ubuntu20 due to zstd 1.5.4 (facebook/zstd#3500) using a feature only present in cmake >= 3.18. It seems like they have resolved this issue upstream by eliminating the use of that feature, with the fix committed and expected in the next release.

(For myself, I resolved my issue by going back to Ubuntu18.)

@1a1a11a
Copy link
Author

1a1a11a commented Feb 22, 2023

Oh, this is helpful! In that case, then we can keep cmake

@1a1a11a
Copy link
Author

1a1a11a commented Feb 22, 2023

updated

wonglkd added a commit to wonglkd/CacheLib-1 that referenced this pull request Feb 22, 2023
Change build script to pin fmt version at same version that folly uses to minimize future breaks.

_Context:_ OSS build broke between 3-5 Jan 2023, likely due to changes in folly. While switching to v9.1.0 or 9.0.0 fixes the issue at hand, it seems sensible to match folly, which specifies fmt v8.0.1: https://github.com/facebook/folly/blob/main/build/fbcode_builder/manifests/fmt

Changed it on a fresh clone of CacheLib and got it to build. (Also had to change `external_git_branch=dev` for zstd to deal with the cmake/zstd issue in facebook#194, but that should resolve when gets merged into release)

> facebook#62 @agordon: For the other packages, you'll notice we do use a specific git tag or branch… I notice `fmt` is an exception - not pinned to a specific git tag or revision - likely an omission that can be fixed.

Related CacheLib issues: facebook#186, facebook#189, facebook#107, facebook#97, facebook#62
Related CacheLib commit: 67cc11a

Last working (Jan 3): https://github.com/facebook/CacheLib/actions/runs/3826992478
First failed (Jan 5): https://github.com/facebook/CacheLib/actions/runs/3844002307/jobs/6546742348
```error: static assertion failed: Cannot format an argument. To make type T formattable provide a formatter<T> specialization: https://fmt.dev/latest/api.html#udt```
@wonglkd wonglkd mentioned this pull request Feb 22, 2023
facebook-github-bot pushed a commit that referenced this pull request Feb 24, 2023
Summary:
Change build script to pin fmt version to same as folly's to minimize future breaks.

Pull Request resolved: #196

Test Plan:
Built successfully on a fresh clone of CacheLib. (Also had to change `external_git_branch=dev` for zstd to deal with the cmake/zstd issue in #194, but that should resolve when gets merged into release)

**Context:** OSS build broke between 3-5 Jan 2023, likely due to changes in folly. While switching to v9.1.0 or 9.0.0 fixes the issue at hand, it seems sensible to match folly, which specifies fmt v8.0.1: https://github.com/facebook/folly/blob/main/build/fbcode_builder/manifests/fmt

> #62 agordon: For the other packages, you'll notice we do use a specific git tag or branch… I notice `fmt` is an exception - not pinned to a specific git tag or revision - likely an omission that can be fixed.

Related CacheLib issues: #186, #189, #107, #97, #62
Possibly related CacheLib commit: 67cc11a

Last working (Jan 3): https://github.com/facebook/CacheLib/actions/runs/3826992478
First failed (Jan 5): https://github.com/facebook/CacheLib/actions/runs/3844002307/jobs/6546742348
Error: `error: static assertion failed: Cannot format an argument. To make type T formattable provide a formatter<T> specialization: https://fmt.dev/latest/api.html#udt`

Reviewed By: therealgymmy

Differential Revision: D43517927

Pulled By: jiayuebao

fbshipit-source-id: 2d28791f7804d862b646263b96b10b835f843d8c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants