-
Notifications
You must be signed in to change notification settings - Fork 3.9k
GH-48074: [C++] Use FetchContent for bundled Abseil #48075
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
|
|
|
@github-actions crossbow submit -g cpp |
|
Revision: ffffeb4 Submitted crossbow builds: ursacomputing/crossbow @ actions-f1048dc859 |
|
@github-actions crossbow submit -g cpp |
|
Revision: ef326eb Submitted crossbow builds: ursacomputing/crossbow @ actions-b9f2ce761a |
|
@kou I'd like to discuss this with you once you have some time. There are some challenges mixing |
|
@github actions crossbow submit bundled |
|
@github-actions crossbow submit bundled |
|
Revision: b81600c Submitted crossbow builds: ursacomputing/crossbow @ actions-3868d3fe73
|
|
@kou @assignUser I would apprec iate if you could take a look here. I have several PRs aligned after this one, the current plan is to move: The main problem is that in the interim we have to live with both |
|
Sorry for missing this! (I saw your comment last week but I attended a conference. So I forgot to review this... Sorry...) I'll review this today. |
| # ---------------------------------------------------------------------- | ||
| # Dependencies for Arrow Flight RPC | ||
|
|
||
| macro(build_absl) |
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 function() not macro() to create a variable scope?
We can remove cleanup_fetchcontent() by this 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.
That was a good idea, moving to function help me clean a bunch of things that were not necessary! Thanks for the suggestion @kou
Co-authored-by: Sutou Kouhei <[email protected]>
…ve target from abls_ep to absl_fc
|
@github-actions crossbow submit bundled |
|
Revision: 541ca51 Submitted crossbow builds: ursacomputing/crossbow @ actions-46baf386d7
|
Rationale for this change
As a follow up of requiring a minimum CMake version >= 3.25 we discussed moving our dependencies from ExternalProject to FetchContent. This can heavily simplify our third party dependency management. Moving abseil is the first step to simplify some of them.
What changes are included in this PR?
The general change is moving from
ExternalProjecttoFetchContent. In more detail this gets rid of all the manual definition of targets for all abseil libraries with it's dependency management. This is removing around 900 lines of custom code which is not necessary with FetchContent.It also add some required integration due to other dependencies, like grpc, using
ExternalProject. We not only have to build but also install in order for those other dependencies to find abseil. This causes some timing issues between config, build, install that requires us to create a custom target to depend on so the other dependencies find abseil. As we have to install abseil for those to find it we also want to not install abseil outside of the build path that's why we override abseil'scmake_install.cmakewith no-op.Are these changes tested?
Yes, the changes are tested locally and on CI.
Are there any user-facing changes?
No