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.

Sign up for GitHub

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

Jump to bottom

GH-37266: [CI][C++] Avoid clobbering build type with CMAKE_ARGS #37269

Closed
wants to merge 1 commit into from
Closed

GH-37266: [CI][C++] Avoid clobbering build type with CMAKE_ARGS #37269

wants to merge 1 commit into from

Conversation

pitrou
Copy link
Member

@pitrou pitrou commented Aug 21, 2023 •
edited by github-actions bot
Loading

Since CMAKE_ARGS could in some circumstances (conda-forge) override CMAKE_BUILD_TYPE, pass it first and let our own choice override it.

@pitrou
apacheGH-37266: [CI][C++] Avoid clobbering build type with CMAKE_ARGS
f883af5 
Since CMAKE_ARGS could in some circumstances (conda-forge) override CMAKE_BUILD_TYPE, pass it first and let our own choice override it.
@github-actions
Copy link

⚠️ GitHub issue #37266 has been automatically assigned in GitHub to PR creator.

@github-actions github-actions bot added the awaiting review Awaiting review label Aug 21, 2023
@pitrou
Copy link
Member Author

pitrou commented Aug 21, 2023

@github-actions crossbow submit -g cpp

@github-actions
Copy link

Revision: f883af5

Submitted crossbow builds: ursacomputing/crossbow @ actions-2927577a55

Task Status
test-alpine-linux-cpp Github Actions
test-build-cpp-fuzz Github Actions
test-conda-cpp Github Actions
test-conda-cpp-valgrind Azure
test-cuda-cpp Github Actions
test-debian-11-cpp-amd64 Github Actions
test-debian-11-cpp-i386 Github Actions
test-fedora-35-cpp Github Actions
test-ubuntu-20.04-cpp Github Actions
test-ubuntu-20.04-cpp-bundled Github Actions
test-ubuntu-20.04-cpp-minimal-with-formats Github Actions
test-ubuntu-20.04-cpp-thread-sanitizer Github Actions
test-ubuntu-22.04-cpp Github Actions
test-ubuntu-22.04-cpp-20 Github Actions
test-ubuntu-22.04-cpp-no-threading Github Actions

@pitrou
Copy link
Member Author

pitrou commented Aug 21, 2023

@github-actions crossbow submit conda

@github-actions
Copy link

Revision: f883af5

Submitted crossbow builds: ursacomputing/crossbow @ actions-fcac70e6a6

Task Status
conda-clean Azure
conda-linux-aarch64-cpu-py3 Azure
conda-linux-aarch64-cpu-r41 Azure
conda-linux-aarch64-cpu-r42 Azure
conda-linux-aarch64-cuda-py3 Azure
conda-linux-ppc64le-cpu-py3 Azure
conda-linux-ppc64le-cuda-py3 Azure
conda-linux-x64-cpu-py3 Azure
conda-linux-x64-cpu-r41 Azure
conda-linux-x64-cpu-r42 Azure
conda-linux-x64-cuda-py3 Azure
conda-osx-arm64-cpu-py3 Azure
conda-osx-arm64-cpu-r41 Azure
conda-osx-arm64-cpu-r42 Azure
conda-osx-x64-cpu-py3 Azure
conda-osx-x64-cpu-r41 Azure
conda-osx-x64-cpu-r42 Azure
conda-win-x64-cpu-py3 Azure
conda-win-x64-cpu-r41 Azure
conda-win-x64-cuda-py3 Azure
example-python-minimal-build-fedora-conda Github Actions
test-conda-cpp Github Actions
test-conda-cpp-valgrind Azure
test-conda-python-3.10 Github Actions
test-conda-python-3.10-hdfs-2.9.2 Github Actions
test-conda-python-3.10-hdfs-3.2.1 Github Actions
test-conda-python-3.10-pandas-latest Github Actions
test-conda-python-3.10-pandas-nightly Github Actions
test-conda-python-3.10-spark-v3.4.1 Github Actions
test-conda-python-3.10-substrait Github Actions
test-conda-python-3.11 Github Actions
test-conda-python-3.11-dask-latest Github Actions
test-conda-python-3.11-dask-upstream_devel Github Actions
test-conda-python-3.11-hypothesis Github Actions
test-conda-python-3.11-pandas-upstream_devel Github Actions
test-conda-python-3.11-spark-master Github Actions
test-conda-python-3.8 Github Actions
test-conda-python-3.8-pandas-1.0 Github Actions
test-conda-python-3.8-spark-v3.4.1 Github Actions
test-conda-python-3.9 Github Actions
test-conda-python-3.9-pandas-latest Github Actions
verify-rc-binaries-jars-linux-conda-latest-amd64 Github Actions
verify-rc-binaries-wheels-linux-conda-latest-amd64 Github Actions
verify-rc-source-cpp-linux-conda-latest-amd64 Github Actions
verify-rc-source-cpp-macos-conda-amd64 Github Actions
verify-rc-source-csharp-linux-conda-latest-amd64 Github Actions
verify-rc-source-go-linux-conda-latest-amd64 Github Actions
verify-rc-source-integration-linux-conda-latest-amd64 Github Actions
verify-rc-source-integration-macos-conda-amd64 Github Actions
verify-rc-source-java-linux-conda-latest-amd64 Github Actions
verify-rc-source-js-linux-conda-latest-amd64 Github Actions
verify-rc-source-python-linux-conda-latest-amd64 Github Actions
verify-rc-source-python-macos-conda-amd64 Github Actions
verify-rc-source-ruby-linux-conda-latest-amd64 Github Actions

@pitrou
Copy link
Member Author

pitrou commented Aug 21, 2023

@kou Does this look ok?

@kou
Copy link
Member

kou commented Aug 21, 2023

Can we use our environment variable such as ARROW_CMAKE_ARGS instead of CMAKE_ARGS?
If we keep using CMAKE_ARGS, we may have a similar problem later again.
(In general, conda users should use CMAKE_ARGS to build their CMake based projects (not conda packages)?)

@pitrou
Copy link
Member Author

pitrou commented Aug 21, 2023

(In general, conda users should use CMAKE_ARGS to build their CMake based projects (not conda packages)?)

I suspect it might be necessary to set up the correct toolchain and options (such as ar and friends)?
@xhochy @h-vetinari Could you shed some light on this?

@pitrou
Copy link
Member Author

pitrou commented Aug 21, 2023

I've tried your solution in #37272

@pitrou
Copy link
Member Author

pitrou commented Aug 21, 2023

It seems like the test-conda-cpp-valgrind failure could be fixed by bumping xxhash (see #37273). At least locally that works.

@kou
Copy link
Member

kou commented Aug 22, 2023

If conda recommends using CMAKE_ARGS, I'm OK with this approach.
Otherwise I like #37272.

@h-vetinari
Copy link
Contributor

I suspect it might be necessary to set up the correct toolchain and options (such as ar and friends)?
@xhochy @h-vetinari Could you shed some light on this?

Not sure where best to answer across 3 different threads. Yes, we set a bunch of things in CMAKE_ARGS, but that's mostly internal to our setup. I guess we should rename that eventually to something less prone to conflicts (but that would mean changing a lot of feedstocks ATM).

I don't understand why the CI for conda builds here ever would need to build non-Release builds, but either ordering CMAKE_ARGS to the beginning (as this PR does) or renaming it should be fine.

Does that answer the questions?

@pitrou
Copy link
Member Author

pitrou commented Aug 22, 2023

I don't understand why the CI for conda builds here ever would need to build non-Release builds, but either ordering CMAKE_ARGS to the beginning (as this PR does) or renaming it should be fine.

Does that answer the questions?

It does, but what should be the preffered solution? Should we use conda's CMAKE_ARGS?

@h-vetinari
Copy link
Contributor

h-vetinari commented Aug 22, 2023 •
edited
Loading

Should we use conda's CMAKE_ARGS?

For conda builds (using conda compilers, with those activation scripts), generally yes, unless you do the (minimum) equivalent bits of the activation scripts yourself (e.g. setting certain paths to binaries). But from my POV, you're not obligated to do it in any particular way in your own CI, and should go with whatever is most understandable / maintainable.

I also still don't understand why you're doing debug builds with conda, but assuming that as a given, I'd say it's probably less surprising to rename the respective variable, i.e. follow #37272.

@pitrou
Copy link
Member Author

pitrou commented Aug 22, 2023

I also still don't understand why you're doing debug builds with conda

Why not? We're using conda as a package manager, we don't generate conda packages.

@h-vetinari
Copy link
Contributor

Sure. As you found out, the compiler stack that comes with that is more geared towards the needs of conda-forge. You can also strip the release flag out of CMAKE_ARGS with a simple sed, if you want.

I'm happy to help here, though in this case I don't see an approach that's obviously the best. It's a kind of "off-label" usage, and no variant is perfect.

@pitrou
Copy link
Member Author

pitrou commented Aug 22, 2023

Ok, so let's use ARROW_CMAKE_ARGS to decouple ourselves from conda-forge, then.

@pitrou pitrou closed this Aug 22, 2023
@pitrou pitrou deleted the gh37266-cmake-args branch August 22, 2023 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@assignUser assignUser Awaiting requested review from assignUser assignUser is a code owner

@kou kou Awaiting requested review from kou kou is a code owner

@raulcd raulcd Awaiting requested review from raulcd raulcd is a code owner

Assignees
No one assigned
Labels
awaiting review Awaiting review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[C++] conda-forge sets build type in CMAKE_ARGS
3 participants
@pitrou @kou @h-vetinari