-
Notifications
You must be signed in to change notification settings - Fork 3.7k
GH-46338: [C++] Add compile step for Meson in cpp_build.sh #46339
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
base: main
Are you sure you want to change the base?
Conversation
|
@raulcd if I understand correctly, the CMake configuration can read the |
@github-actions crossbow submit *meson |
Revision: 6b788ef Submitted crossbow builds: ursacomputing/crossbow @ actions-a671c6cb33
|
6b788ef
to
ef09ed6
Compare
@github-actions crossbow submit *meson |
Revision: ef09ed6 Submitted crossbow builds: ursacomputing/crossbow @ actions-098f7bf390
|
ci/scripts/cpp_build.sh
Outdated
time { | ||
meson compile -j $[${n_jobs} + 1]; | ||
meson install; | ||
} |
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.
We need only compile time because install time will not be slow:
time { | |
meson compile -j $[${n_jobs} + 1]; | |
meson install; | |
} | |
time meson compile -j $[${n_jobs} + 1] | |
meson install |
ci/scripts/cpp_build.sh
Outdated
time { | ||
meson compile -j $[${n_jobs} + 1]; | ||
meson install; | ||
} | ||
else | ||
export CMAKE_BUILD_PARALLEL_LEVEL=${CMAKE_BUILD_PARALLEL_LEVEL:-$[${n_jobs} + 1]} |
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.
How about defining ARROW_BUILD_PARALLEL
or something and use it as the default for Meson and CMake?
: ${ARROW_BUILD_PARALLEL:=$[${n_jobs} + 1]}
if [ "${ARROW_USE_MESON:-OFF}" = "ON" ]; then
meson compile -j ${ARROW_BUILD_PARALLEL}
else
export CMAKE_BUILD_PARALLEL_LEVEL=${ARROW_BUILD_PARALLEL}
fi
@github-actions crossbow submit *meson |
Revision: f471fe1 Submitted crossbow builds: ursacomputing/crossbow @ actions-db63e10f79
|
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.
Thanks for working on this @WillAyd
ci/scripts/cpp_build.sh
Outdated
else | ||
export CMAKE_BUILD_PARALLEL_LEVEL=${CMAKE_BUILD_PARALLEL_LEVEL:-$[${n_jobs} + 1]} | ||
export ARROW_BUILD_PARALLEL=${ARROW_BUILD_PARALLEL:-$[${n_jobs} + 1]} |
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.
CMake doesn't know what ARROW_BUILD_PARALLEL
is, we should still use CMAKE_BUILD_PARALLEL_LEVEL
which we can define outside of the if : ${ARROW_BUILD_PARALLEL:=$[${n_jobs} + 1]}
export ARROW_BUILD_PARALLEL=${ARROW_BUILD_PARALLEL:-$[${n_jobs} + 1]} | |
export CMAKE_BUILD_PARALLEL_LEVEL=${ARROW_BUILD_PARALLEL} |
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.
Nice catch - done!
f471fe1
to
180ad7a
Compare
@github-actions crossbow submit *meson |
Revision: 180ad7a Submitted crossbow builds: ursacomputing/crossbow @ actions-d3f9e22713
|
ci/scripts/cpp_build.sh
Outdated
@@ -283,9 +283,10 @@ else | |||
fi | |||
|
|||
if [ "${ARROW_USE_MESON:-OFF}" = "ON" ]; then | |||
time meson install | |||
time meson compile -j ${ARROW_BUILD_PARALLEL:-$[${n_jobs} + 1]} |
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.
Could you define ARROW_BUILD_PARALLEL
out of this if
to avoid the default value duplication?
export CMAKE_BUILD_PARALLEL_LEVEL=${CMAKE_BUILD_PARALLEL_LEVEL:-${NPROC}} | ||
export CMAKE_BUILD_PARALLEL_LEVEL=${ARROW_BUILD_PARALLEL:-${NPROC}} |
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.
Hmm. I think that we don't need this because we always use CMake in this script.
If we want this, could you use ARROW_BUILD_PARALLEL
only if CMAKE_BUILD_PARALLEL
isn't defined?
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.
Good point - for now I have just reverted this
ci/scripts/cpp_build.sh
Outdated
else | ||
export CMAKE_BUILD_PARALLEL_LEVEL=${CMAKE_BUILD_PARALLEL_LEVEL:-$[${n_jobs} + 1]} | ||
export CMAKE_BUILD_PARALLEL_LEVEL=${ARROW_BUILD_PARALLEL:-$[${n_jobs} + 1]} |
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.
Could you use ARROW_BUILD_PARALLEL
only if CMAKE_BUILD_PARALLEL_LEVEL
? (If both of CMAKE_BUILD_PARALLEL_LEVEL
and ARROW_BUILD_PARALLEL
, CMAKE_BUILD_PARALLEL_LEVEL
should be used.)
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.
It looks a bit more complicated but let me know if the current iteration is now inline with your expectations or not
180ad7a
to
80d8d0c
Compare
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.
👍 LGTM now
ci/scripts/cpp_build.sh
Outdated
@@ -282,10 +282,12 @@ else | |||
${source_dir} | |||
fi | |||
|
|||
ARROW_BUILD_PARALLEL=${CMAKE_BUILD_PARALLEL_LEVEL:-${ARROW_BUILD_PARALLEL:-$[${n_jobs} + 1]}} |
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.
ARROW_BUILD_PARALLEL=${CMAKE_BUILD_PARALLEL_LEVEL:-${ARROW_BUILD_PARALLEL:-$[${n_jobs} + 1]}} | |
: ${ARROW_BUILD_PARALLEL:=$[${n_jobs} + 1]} |
ci/scripts/cpp_build.sh
Outdated
else | ||
export CMAKE_BUILD_PARALLEL_LEVEL=${CMAKE_BUILD_PARALLEL_LEVEL:-$[${n_jobs} + 1]} | ||
export CMAKE_BUILD_PARALLEL_LEVEL=${ARROW_BUILD_PARALLEL} |
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.
export CMAKE_BUILD_PARALLEL_LEVEL=${ARROW_BUILD_PARALLEL} | |
: ${CMAKE_BUILD_PARALLEL_LEVEL:=${ARROW_BUILD_PARALLEL}} | |
export CMAKE_BUILD_PARALLEL_LEVEL |
80d8d0c
to
fb1d52b
Compare
@github-actions crossbow submit *meson |
Revision: fb1d52b Submitted crossbow builds: ursacomputing/crossbow @ actions-770b1544a4
|
Rationale for this change
This makes it possible to control the number of jobs used with Meson in the cpp_build.sh script
What changes are included in this PR?
Updates to cpp_build.sh
Are these changes tested?
Yes
Are there any user-facing changes?
No