-
Notifications
You must be signed in to change notification settings - Fork 93
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
refactor(c)!: Updated include/install paths for adbc.h #1965
Conversation
c/CMakeLists.txt
Outdated
@@ -35,6 +35,8 @@ add_subdirectory(vendor/nanoarrow) | |||
add_subdirectory(driver/common) | |||
add_subdirectory(driver/framework) | |||
|
|||
install(FILES "${REPOSITORY_ROOT}/c/include/adbc.h" DESTINATION include/arrow-adbc) |
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.
I might be misunderstanding when we want this to be installed, but in main the driver_manager CMake configuration is responsible for installing the adbc.h file; figured it made more sense in the top level config
.github/workflows/native-unix.yml
Outdated
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.
I think we still want to trigger workflows when the header is updated?
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.
Ah, it's under c/include
now, cool.
c/include/adbc.h
Outdated
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.
Perhaps we can do this in two parts but I'd also like our own sources to use the 'proper' arrow-adbc/adbc.h
include path
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.
Sounds good - I think probably worth doing in two parts. From what I see so far there might need to be a few things shifted around with all of the other languages
Not sure I understand why the R build for Windows is failing - hoping @paleolimbot may know |
#1966 should do it! |
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.
I think we have to modify at least adbc_driver_manager.h since it refers to adbc.h
but the install path would be arrow-adbc/adbc.h
377f130
to
5cf439f
Compare
Thanks. Could you try the following? diff --git a/ci/linux-packages/debian/libadbc-driver-manager-dev.install b/ci/linux-packages/debian/libadbc-driver-manager-dev.install
index 6cee53d04..933e26364 100644
--- a/ci/linux-packages/debian/libadbc-driver-manager-dev.install
+++ b/ci/linux-packages/debian/libadbc-driver-manager-dev.install
@@ -1,5 +1,4 @@
-usr/include/adbc.h
-usr/include/adbc_driver_manager.h
+usr/include/arrow-adbc/
usr/lib/*/cmake/AdbcDriverManager/
usr/lib/*/libadbc_driver_manager.a
usr/lib/*/libadbc_driver_manager.so
diff --git a/ci/linux-packages/yum/apache-arrow-adbc.spec.in b/ci/linux-packages/yum/apache-arrow-adbc.spec.in
index 9af14a2bf..70105cb06 100644
--- a/ci/linux-packages/yum/apache-arrow-adbc.spec.in
+++ b/ci/linux-packages/yum/apache-arrow-adbc.spec.in
@@ -120,8 +120,7 @@ Libraries and header files for ADBC driver manager.
%defattr(-,root,root,-)
%doc README.md
%license LICENSE.txt NOTICE.txt
-%{_includedir}/adbc.h
-%{_includedir}/adbc_driver_manager.h
+%{_includedir}/arrow-adbc/
%{_libdir}/cmake/AdbcDriverManager/
%{_libdir}/libadbc_driver_manager.a
%{_libdir}/libadbc_driver_manager.so |
BTW, should we install // adbc.h
#include "arrow-adbc/adbc.h" // adbc_driver_manager.h
#include "arrow-adbc/adbc_driver_manager.h" |
No strong preference on this so will leave up to @lidavidm |
Once we get things working here I think a compatibility header is a good idea (at least for a few versions) |
I think the C# failures are unrelated, so just down to R at the moment. Can't seem to reproduce locally so any guidance there is greatly appreciated |
Rebasing would make the C# failures go away, I think. |
@paleolimbot I know you've been super busy lately but if you get any time to advise on the R failures would be greatly appreciated |
Sorry for the delay circling back! There was a CI issue with |
ab40814
to
84549ce
Compare
OK should be green, and I've added the old headers in the standard include path with a pragma message to discourage their use. Let me know what else we may need on this |
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.
Thanks for working through this!
I'll leave this for @paleolimbot and @zeroshade in case they want to have a final look at the R/Go changes as well
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.
(For the R changes!)
…2051) The R check actions are currently failing because an update to r-lib actions resulted in some quarto actions being invoked, and these have not yet been whitelisted for use in Apache repositories. We don't actually need quarto for anything in the R package; however, the action is written in such a way that even though the step is skipped, the action is still loaded (and therefore its name is checked against the whitelist). Unrelatedly, the extended R check was failing because I forgot to copy a change from #1965 into one of the workflows that only runs once a week.
closes #1150