jonkeane commented on code in PR #38970:
URL: https://github.com/apache/arrow/pull/38970#discussion_r1409311880
##########
r/configure:
##########
@@ -405,6 +405,21 @@ find_or_build_libarrow
# Now set `PKG_LIBS`, `PKG_DIRS`, and `PKG_CFLAGS` based on that.
if [ "$_LIBARROW_FOUND" != "false" ] && [ "$_LIBARROW_FOUND" != "" ]; then
set_pkg_vars ${_LIBARROW_FOUND}
+
+ # If we didn't find any libraries with pkg-config, try again without
pkg-config
+ if [ -z "`echo "$PKG_LIBS" | tr -d '[:space:]'`" ] && [
"$PKG_CONFIG_AVAILABLE" = "true" ]; then
Review Comment:
It might be easier to read / reason about if we did something like this:
```suggestion
# If we didn't find any libraries with pkg-config, try again without
pkg-config
FOUND_PKG_LIBS=`echo "$PKG_LIBS" | tr -d '[:space:]'`
if [ -z "$FOUND_PKG_LIBS" ] && [ "$PKG_CONFIG_AVAILABLE" = "true" ]; then
```
Would that work?
##########
r/configure:
##########
@@ -293,15 +293,15 @@ set_pkg_vars () {
# If we have pkg-config, it will tell us what libarrow needs
set_lib_dir_with_pc () {
- LIB_DIR="`${PKG_CONFIG} --variable=libdir --silence-errors
${PKG_CONFIG_NAME}`"
+ LIB_DIR="`${PKG_CONFIG} --variable=libdir ${PKG_CONFIG_NAME}`"
Review Comment:
I think I'm fine with leaving out `--silence-errors` for now, but would you
mind showing examples of what kinds of output might be spit out without it?
##########
r/configure:
##########
@@ -405,6 +405,21 @@ find_or_build_libarrow
# Now set `PKG_LIBS`, `PKG_DIRS`, and `PKG_CFLAGS` based on that.
if [ "$_LIBARROW_FOUND" != "false" ] && [ "$_LIBARROW_FOUND" != "" ]; then
set_pkg_vars ${_LIBARROW_FOUND}
+
+ # If we didn't find any libraries with pkg-config, try again without
pkg-config
+ if [ -z "`echo "$PKG_LIBS" | tr -d '[:space:]'`" ] && [
"$PKG_CONFIG_AVAILABLE" = "true" ]; then
+ echo "*** pkg-config failed to find libraries. Running detection without
pkg-config."
+ if arrow_built_with ARROW_S3 || arrow_built_with ARROW_GCS; then
+ S3_LIBS="-lcurl -lssl -lcrypto"
+ GCS_LIBS="-lcurl -lssl -lcrypto"
+ fi
+ PKG_CONFIG_AVAILABLE="false"
+ set_pkg_vars ${_LIBARROW_FOUND}
Review Comment:
We don't have to fix this here, but I wonder if `S3_LIBS="-lcurl -lssl
-lcrypto"` and `GCS_LIBS="-lcurl -lssl -lcrypto"` would be better inside of one
of the functions like `set_pkg_vars_with_pc` (possibly guarded by detection of
if they are already there / if they have an alternate form) and then this
becomes basically:
```
echo "*** pkg-config failed to find libraries. Running detection without
pkg-config."
PKG_CONFIG_AVAILABLE="false"
set_pkg_vars ${_LIBARROW_FOUND}
```
The main worry I have is that currently, with this set up if someone has
never found pkgconfig in the first place, they won't ever attempt to add those
curl/ssl/crypto libs on. Maybe that's a circumstance that never happens, but it
seems like an oversight from here.
Maybe would you mind opening an issue to explore / address this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
