You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/03/13 01:13:41 UTC
[GitHub] [arrow] nealrichardson opened a new pull request #9689: [WIP] Restore simpler ARROW_R_WITH_ARROW wrapping
nealrichardson opened a new pull request #9689:
URL: https://github.com/apache/arrow/pull/9689
Without the ARROW_R_WITH_ARROW wrapping around the code in arrowExports.cpp, if Arrow C++ is not available, the build fails--as expected. However, it does so with on the order of 10,000 lines of compiler errors because all 400 function definitions in that file fail to compile.
For 4.0, given that CRAN is running Solaris checks again, we need to not fail anymore. We could just restore the previous wrapping of every function, but I wanted to try something lighter weight.
Going forward, once we decide that ARROW_R_WITH_ARROW must always be true, we should fail earlier and clearer, maybe in `configure` if possible, so that the user's terminal doesn't blow up and so they can see what the actual problem is.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #9689: [WIP] Restore simpler ARROW_R_WITH_ARROW wrapping
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #9689:
URL: https://github.com/apache/arrow/pull/9689#issuecomment-813556612
@ianmcook do you have an opinion on 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jonkeane commented on pull request #9689: [WIP] Restore simpler ARROW_R_WITH_ARROW wrapping
Posted by GitBox <gi...@apache.org>.
jonkeane commented on pull request #9689:
URL: https://github.com/apache/arrow/pull/9689#issuecomment-810420129
I've taken a look at this and I think this is an overall better (temporary?) solution than wrapping each function call. Absolutely agree that an earlier + clearer error will be good when we can do that.
I haven't yet run this locally (yet), but I wanted to confirm my reading of the test suite is correct: All of the operative tests will be skipped on CRAN (when/if the C++ library is unavailable) because of [our wrapping of `test_that`](https://github.com/apache/arrow/blob/master/r/tests/testthat/helper-arrow.R#L58-L63) and [setting `..skip.tests` when arrow isn't available](https://github.com/apache/arrow/blob/master/r/tests/testthat/helper-arrow.R#L19) (tangentially: could we move up our `test_that` wrapper so they are closer, or do we need the `test_that` wrapping lower down for some other reason?).
Overall, I think it's good to reduce the code duplication (which makes searching for strings harder) and to me has a slightly easier to see that everything in the arrowExports.cpp will be not included if that one variable is set (compared to now where it might look like we are selectively turning on or off functions as opposed to turning each one of individually).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson closed pull request #9689: [WIP] Restore simpler ARROW_R_WITH_ARROW wrapping
Posted by GitBox <gi...@apache.org>.
nealrichardson closed pull request #9689:
URL: https://github.com/apache/arrow/pull/9689
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #9689: [WIP] Restore simpler ARROW_R_WITH_ARROW wrapping
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #9689:
URL: https://github.com/apache/arrow/pull/9689#issuecomment-813625787
nevermind, I decided to cherry-pick this into #9896. Closing.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #9689: [WIP] Restore simpler ARROW_R_WITH_ARROW wrapping
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #9689:
URL: https://github.com/apache/arrow/pull/9689#issuecomment-813587397
Ok. Then I'll merge this, rebase #9896, and doublecheck that the experience is what we want and tweak anything else there.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9689: [WIP] Restore simpler ARROW_R_WITH_ARROW wrapping
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9689:
URL: https://github.com/apache/arrow/pull/9689#issuecomment-797839819
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW
Then could you also rename pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #9689: [WIP] Restore simpler ARROW_R_WITH_ARROW wrapping
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #9689:
URL: https://github.com/apache/arrow/pull/9689#issuecomment-813583489
> @ianmcook do you have an opinion on this?
Just on the basis of how much code was removed by ARROW-10735 versus how little code is added here, this seems sensible, but I haven't dug into this enough to have a well-informed opinion
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org