-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
GH-39517: [C++] Disable parallelism for jemalloc external project #39522
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -446,8 +446,8 @@ endmacro() | |
file(STRINGS "${THIRDPARTY_DIR}/versions.txt" TOOLCHAIN_VERSIONS_TXT) | ||
foreach(_VERSION_ENTRY ${TOOLCHAIN_VERSIONS_TXT}) | ||
# Exclude comments | ||
if(NOT ((_VERSION_ENTRY MATCHES "^[^#][A-Za-z0-9-_]+_VERSION=") | ||
OR (_VERSION_ENTRY MATCHES "^[^#][A-Za-z0-9-_]+_CHECKSUM="))) | ||
if(NOT ((_VERSION_ENTRY MATCHES "^[^ #][A-Za-z0-9-_]+_VERSION=") | ||
OR (_VERSION_ENTRY MATCHES "^[^ #][A-Za-z0-9-_]+_CHECKSUM="))) | ||
continue() | ||
endif() | ||
|
||
|
@@ -2040,10 +2040,17 @@ macro(build_jemalloc) | |
# Enable jemalloc debug checks when Arrow itself has debugging enabled | ||
list(APPEND JEMALLOC_CONFIGURE_COMMAND "--enable-debug") | ||
endif() | ||
|
||
set(JEMALLOC_BUILD_COMMAND ${MAKE} ${MAKE_BUILD_ARGS}) | ||
# Paralleism for Make fails with CMake > 3.28 see #39517 | ||
if(${CMAKE_GENERATOR} MATCHES "Makefiles") | ||
list(APPEND JEMALLOC_BUILD_COMMAND "-j1") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we want to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem has not occurred with any other dependency neither on CI nor in my own testing (or after the fix). For the smaller dependencies it might not make a difference but with the number of larger dependencies we have forcing the all to use -j1 if there is no actual issue seems like it would slow build times with no gain? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not a strong opinion but I think that we should choose safer option (disable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We only have 2 cores available on cran (policy), no ninja (afaik) and our build times are already being criticized so.... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we have only 2 cores, we should not use parallel build for dependencies. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI: We use |
||
endif() | ||
|
||
if(CMAKE_OSX_SYSROOT) | ||
list(APPEND JEMALLOC_BUILD_COMMAND "SDKROOT=${CMAKE_OSX_SYSROOT}") | ||
endif() | ||
|
||
externalproject_add(jemalloc_ep | ||
${EP_COMMON_OPTIONS} | ||
URL ${JEMALLOC_SOURCE_URL} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are they needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were made by the cmake linter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow. What CMake linter did you use? And what the error message from the CMake linter?
It seems that this diff changes matched pattern. (I think that it doesn't have any problem in our use case.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
archery lint --fix --cmake-format
^^ but I am happy to remove the change?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm weird, it doesn't produce output if I leave the --fix flag off. I will revert the change to be safe.