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.

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

ARROW-15388: [C++] Avoid including absl from flatbuffers #12204

Closed

Conversation

pitrou
Copy link
Member

@pitrou pitrou commented Jan 20, 2022

There may be an incomplete/broken copy of the Abseil library lying around
(for example because of a race condition while installing it as a bundled library).

Since absl is only used by Flatbuffers to provide a string_view facility,
use our own vendored string_view instead.

There may be an incomplete/broken copy of the Abseil library lying around
(for example because of a race condition while installing it as a bundled library).

Since absl is only used by Flatbuffers to provide a string_view facility,
use our own vendored string_view instead.
@pitrou pitrou requested a review from bkietz January 20, 2022 11:50
@pitrou
Copy link
Member Author

pitrou commented Jan 20, 2022

@coryan Not sure whether you have an opinion on this?

@github-actions
Copy link

@github-actions
Copy link

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@coryan
Copy link
Contributor

coryan commented Jan 20, 2022

Not sure whether you have an opinion on this?

To me, it seems odd to patch the code to fix a race condition during the build. Seems like the "Right Thing"[tm] would be to fix the dependency graph to make flatbuffers depend on Abseil? Is that very hard for some reason?

@pitrou
Copy link
Member Author

pitrou commented Jan 20, 2022

It's a bit convoluted and awkward, as Abseil is only an optional indirect dependency of the GCSFS implementation. Furthermore, since Flatbuffers is vendored, I don't think it has an existence in the dependency graph at all currently (it's basically always there).

@coryan
Copy link
Contributor

coryan commented Jan 20, 2022

It's a bit convoluted and awkward, as Abseil is only an optional indirect dependency of the GCSFS implementation. Furthermore, since Flatbuffers is vendored, I don't think it has an existence in the dependency graph at all currently (it's basically always there).

I see.

Consider that (by default) absl::string_view is just an alias for std::string_view when compiling with C++ >= 17, and it is a template class when compiling with C++ <= 14 (something I do not happen to like but it is a thing: abseil/abseil-cpp#696). I am not sure if Flatbuffers depends on that (my guess would be no), hopefully not. If it did, you may run intro trouble as it tries to use std::string_view in C++17 but the Arrow version of string_view does not magically convert to std::string_view.

You may want to just disable the automatic detection of Abseil's string view. Just my $0.02, I think your change should work as-is too.

@pitrou
Copy link
Member Author

pitrou commented Jan 20, 2022

Flatbuffers' use of string_view is merely to enable two APIs (that we don't currently use AFAICT, so we may indeed as well disable it). It doesn't store any std::string_view under the hood, so that should be fine.

@pitrou pitrou requested a review from lidavidm January 20, 2022 14:56
@lidavidm
Copy link
Member

If it just enables APIs we don't use, I would lean towards avoiding the mess in the first place and just removing the detection as well. But as-is things are OK here too.

@kszucs
Copy link
Member

kszucs commented Jan 20, 2022

@pitrou shall we include this in 7.0 (in the case I need to cut another RC)?

@pitrou
Copy link
Member Author

pitrou commented Jan 20, 2022

@kszucs There's no real need to.

@pitrou
Copy link
Member Author

pitrou commented Jan 20, 2022

@pitrou pitrou closed this in 22dc537 Jan 20, 2022
@pitrou pitrou deleted the ARROW-15388-flatbuffers-string-view branch January 20, 2022 19:44
@ursabot
Copy link

ursabot commented Jan 20, 2022

Benchmark runs are scheduled for baseline = a3efe72 and contender = 22dc537. 22dc537 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.91% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.3% ⬆️0.04%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants