jonkeane commented on code in PR #38970:
URL: https://github.com/apache/arrow/pull/38970#discussion_r1410746512
##########
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:
Ok, thanks. IMO that's pretty noisy (and it all sounds very bad ™️ to
someone who isn't used to this, which will be confusing for them since it
almost certainly isn't the error if they run into it) so I would advocate for
adding the silencing back in as soon as we can.
If you think it's better to leave it for just this one release that's ok,
but if I'm reading this correctly we should get a good sign that this has
happened from line
https://github.com/apache/arrow/pull/38970/files#diff-089697faebdb7820ca629a2bb316b878cc0ba18a5bfb0b60996f8dbcd1fa11e7R407
right after with a much nicer message, so maybe we don't really need it in
this release? Regardless, we should plan to add it back in next release at the
latest (and if we're going to do that, we should make an issue / add a comment,
yeah?).
##########
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:
Ok, thanks. IMO that's pretty noisy (and it all sounds very bad ™️ to
someone who isn't used to this, which will be confusing for them since it
almost certainly isn't the error if they run into it) so I would advocate for
adding the silencing back in as soon as we can.
If you think it's better to leave it for just this one release that's ok,
but if I'm reading this correctly we should get a good sign that this has
happened from line
https://github.com/apache/arrow/pull/38970/files#diff-089697faebdb7820ca629a2bb316b878cc0ba18a5bfb0b60996f8dbcd1fa11e7R407
right after with a much nicer message, so maybe we don't really need it in
this release? Regardless, we should plan to add it back in next release at the
latest (and if we're going to do that, we should make an issue / add a comment,
yeah?).
--
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]
