build: Minor cleanup in CMakeLists #11453
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
|
@zuyu what is the motivation for this change? |
|
|
|
Folly seems to handle Linux arm which is aarch64 |
|
@zuyu The IS_AARCH64_ARCH seems to be enabling some optimizations for arm. It looks like we cannot remove this. |
zuyu
force-pushed
the
cmake-cleanup
branch
2 times, most recently
from
7f1d396 to
786b58a
Compare
| # folly will wrongly assume x86_64 if this is not set | ||
| set(CMAKE_LIBRARY_ARCHITECTURE aarch64) | ||
| if(${CMAKE_SYSTEM_PROCESSOR} MATCHES "arm64") | ||
| set(IS_AARCH64_ARCH TRUE) |
There was a problem hiding this comment.
@zuyu Are you able to confirm from the build that this change works? Can you share the log?
There was a problem hiding this comment.
@majetideepak W/ the following changes:
if(${CMAKE_SYSTEM_PROCESSOR} MATCHES "arm64")
set(IS_AARCH64_ARCH TRUE)
message(STATUS "set(IS_AARCH64_ARCH TRUE)")
endif()
make debug output on macOS:
-- Setting folly source to AUTO
CMake Warning at CMake/ResolveDependency.cmake:70 (find_package):
By not providing "Findfolly.cmake" in CMAKE_MODULE_PATH this project has
asked CMake to find a package configuration file provided by "folly", but
CMake did not find one.
Could not find a package configuration file provided by "folly" with any of
the following names:
follyConfig.cmake
folly-config.cmake
Add the installation prefix of "folly" to CMAKE_PREFIX_PATH or set
"folly_DIR" to a directory containing one of the above files. If "folly"
provides a separate development package or SDK, be sure it has been
installed.
Call Stack (most recent call first):
CMakeLists.txt:502 (velox_resolve_dependency)
-- Building Folly from source
-- set(IS_AARCH64_ARCH TRUE)
...
-- Setting FOLLY_USE_SYMBOLIZER: OFF
-- Setting FOLLY_HAVE_ELF:
-- Setting FOLLY_HAVE_DWARF: FALSE
...
-- Using BUNDLED folly
...
[3567/3568] Linking CXX executable velox/tool/trace/velox_query_replayer
ld: warning: ignoring duplicate libraries: ... '_deps/folly-build/libfolly.a' ...Note that
IS_AARCH64_ARCHis defined inv2024.07.15.00, so it has no effects for now (v2024.07.01.00).CMAKE_LIBRARY_ARCHITECTUREis only used inmessagecmds in folly CMake. In other words, it has no effects at all.
There was a problem hiding this comment.
Sorry to come in late, this whole if can be removed because I upstreamed the change to folly last year https://github.com/facebook/folly/blob/main/CMakeLists.txt#L82
There was a problem hiding this comment.
@assignUser I get your point, but on macOS ${CMAKE_SYSTEM_PROCESSOR} is arm64, NOT aarch64.
Or, we could update folly w/ ${CMAKE_SYSTEM_PROCESSOR} MATCHES "aarch64|arm64", and remove it here.
There was a problem hiding this comment.
Ok but are you running into the linking issues described here: facebook/folly@c30d49d ? From what I can tell this is about linux arm not apple?
Because the actual issue why this use of ON_M1 was added was fixed (it added sse flags on m1).
There was a problem hiding this comment.
No, I do not have any build / link issues.
This PR just to remove ON_APPLE_M1, which was set in root/CMake, but only used in folly/CMake.
Btw, facebook/folly@c30d49d is part of v2024.07.15.00, and Velox uses v2024.07.01.00 for now.
There was a problem hiding this comment.
Right, so we can just remove the entire thing :)
There was a problem hiding this comment.
@assignUser Plz also review facebook/folly#2341. Ty!
assignUser
added
the
ready-to-merge
|
@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
1 similar comment
|
@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@kagamiori merged this pull request in 63b4b08. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
No description provided.