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: Update FBOS to v2024.07.15 to fix arm64 + Linux build #12451

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

zhztheplayer
Copy link
Contributor

@zhztheplayer zhztheplayer commented Feb 26, 2025

Fixes #12450

@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 26, 2025
Copy link

netlify bot commented Feb 26, 2025

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 3544513
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/67beec34862a7f0008bf5108

@zhztheplayer zhztheplayer changed the title Update FBOS to v2024.07.15 to fix arm64 build fix: Update FBOS to v2024.07.15 to fix arm64 build Feb 26, 2025
@zhztheplayer zhztheplayer changed the title fix: Update FBOS to v2024.07.15 to fix arm64 build fix: Update FBOS to v2024.07.15 to fix arm64 + Linux build Feb 26, 2025
@zhztheplayer zhztheplayer marked this pull request as ready for review February 26, 2025 05:02
@zhztheplayer
Copy link
Contributor Author

Confirmed to fix #12450.

@zhztheplayer zhztheplayer marked this pull request as draft February 26, 2025 05:36
@zhztheplayer
Copy link
Contributor Author

CI Error seems related to facebook/folly@581e09a

@zhztheplayer zhztheplayer force-pushed the wip-update-folly branch 2 times, most recently from 04bfcf7 to e33b2d3 Compare February 26, 2025 10:23
@zhztheplayer
Copy link
Contributor Author

The folly patch will no longer be needed after bumping to version >= v2024.08.19.00 in future.

@zhztheplayer zhztheplayer marked this pull request as ready for review February 26, 2025 12:48
@czentgr
Copy link
Collaborator

czentgr commented Feb 26, 2025

The version of folly was picked to support both C++17 and C++20 standard. At the time, using a newer folly caused compatibility issues where you couldn't switch between c++17 and c++20 standard and have all tests pass.
I'm working on the c++20 support but got bogged down with compilation issues on Linux with missing symbols in the iceberg code.

@zhztheplayer
Copy link
Contributor Author

The version of folly was picked to support both C++17 and C++20 standard. At the time, using a newer folly caused compatibility issues where you couldn't switch between c++17 and c++20 standard and have all tests pass. I'm working on the c++20 support but got bogged down with compilation issues on Linux with missing symbols in the iceberg code.

@czentgr Thank you for the inputs. I could expect that there was good reason to stay in v2024.07.01 so the PR is totally optional for the community.

The version of folly was picked to support both C++17 and C++20 standard.

Given this is a small bump (v2024.07.01 -> v2024.07.15), I assume it's not complicated to have the both standards supported in 07.15? But perhaps it's not worth it since I already added a folly patch in this PR.

Another option is to add the patch to current version of folly code directly, without version bumping. Would you think that's feasible?

@czentgr
Copy link
Collaborator

czentgr commented Feb 27, 2025

@zhztheplayer Thanks. The problem with a patch is that it applies only to the bundled version of folly. But most deployments will use the system version (aka the one from the setup scripts). Unless the fix-thread-local.patch patch is applied at the setup script level as well after folly has been downloaded.

The thing about the folly versions was that I tried to find a version that worked for both at the same time. The proposed version also had the odd issue. I tried to find the most recent version that worked for both and this was it.
I would hope new versions of folly actually work with both c++ versions too.

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.

Build on arm64 + Linux: undefined reference to `__folly_memcpy_aarch64'
3 participants