-
Notifications
You must be signed in to change notification settings - Fork 7.5k
[odbccpp] Add new port #50152
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
base: master
Are you sure you want to change the base?
[odbccpp] Add new port #50152
Changes from 6 commits
a7ea7ea
937cdc4
c95f982
8810753
b763f24
af41392
44e8c9e
2ac765b
03d31da
f5087a4
8615dfb
8078683
d6459cc
fc3442f
906eaf6
53ab57e
3d4c8e9
b01fd81
cb172d5
cf9cbe1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| vcpkg_from_github( | ||
| OUT_SOURCE_PATH SOURCE_PATH | ||
| REPO SAP/odbc-cpp-wrapper | ||
| REF "v${VERSION}" | ||
| SHA512 8e79536bc24bd4f59ddc9554824df255fabb7ca651839ff2c90f8a352f61da99beb1df5042c2785d86a3294bcf8c0a93064ed89a62400063755cb5a7df47ca58 | ||
| HEAD_REF master | ||
| PATCHES | ||
| use-vcpkg-unixodbc.patch | ||
| ) | ||
|
|
||
| vcpkg_cmake_configure( | ||
| SOURCE_PATH "${SOURCE_PATH}" | ||
| OPTIONS | ||
| -DGTEST_FOUND=OFF | ||
| -DDOXYGEN_FOUND=OFF | ||
| ) | ||
|
|
||
| vcpkg_cmake_install() | ||
|
|
||
florsap marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug/include") | ||
|
|
||
| vcpkg_install_copyright(FILE_LIST "${SOURCE_PATH}/LICENSE") | ||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,48 @@ | ||||||||||
| diff --git a/CMakeLists.txt b/CMakeLists.txt | ||||||||||
| index 6861360..8d8e62a 100644 | ||||||||||
| --- a/CMakeLists.txt | ||||||||||
| +++ b/CMakeLists.txt | ||||||||||
| @@ -4,7 +4,17 @@ SET(CMAKE_CXX_STANDARD 11) | ||||||||||
|
|
||||||||||
| PROJECT(odbccpp) | ||||||||||
|
|
||||||||||
| -FIND_PACKAGE(ODBC REQUIRED) | ||||||||||
| +IF(UNIX) | ||||||||||
| + FIND_PACKAGE(unixodbc CONFIG) | ||||||||||
|
||||||||||
| + IF(unixodbc_FOUND) | ||||||||||
|
||||||||||
| + IF(unixodbc_FOUND) | |
| + IF(unofficial-unixodbc_FOUND) |
I haven't dug too deep into the problem here but I suspect that this is still falling back to the find_package(ODBC REQUIRED) call below which ends up searching for odbc_config and attempts to execute 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.
This must match the dependencies expressed in the manifests. I have more suggestions once the manifest dependency question is answered.
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.
Good point, this is wrongfully adapted. Depending on the result of the other comment, this probably should be IF(LINUX OR APPLE).
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.
This must match the dependencies expressed in the manifests. I have more suggestions once the manifest dependency question is answered.
@dg0yt, is there anything else you want to suggest? Otherwise, this port looks good to go to me
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.
IF(UNIX) still seems wrong to me. This would become an installation order dependency when unixodbc is extended to support bsd or ios or android.
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.
The safest wiring would be an option which is controlled by the portfile. Here (project mode):
if(ODBCCPP_USE_UNIXODBC)
FIND_PACKAGE(unofficial-unixodbc CONFIG REQUIRED)
SET(ODBC_TARGET unofficial::unixodbc::unixodbc)
ELSE()
...and in the portfile (vcpkg script mode):
vcpkg_list(SET options)
if(VCPKG_TARGET_IS_LINUX OR VCPKG_TARGET_IS_OSX)
list(APPEND options -DODBCCPP_USE_UNIXODBC=ON)
endif()
vcpkg_cmake_configure(
...
OPTIONS
${options}
...
)- I would probably create an alias ODBC::ODBC instead of replacing its use with a variable.
- There is no installed CMake config. But unless using dynamic loading, there are conditional transitive usage requirements. User must duplicate the condition.
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 the careful check. I added a config file to odbccpp, this is a good thing anyway. Also, I added your suggestions, I hope it fits now.
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.
| +IF(NOT ODBC_TARGET) | |
| + FIND_PACKAGE(ODBC REQUIRED) | |
| + SET(ODBC_TARGET ODBC::ODBC) | |
| +ENDIF() |
I don't think this fallback is needed.
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 changed it to an if else clause, but in general it is necessary for for Windows, I think. Hope this fits for you.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| { | ||
| "name": "odbccpp", | ||
| "version": "1.4", | ||
| "description": "An object-oriented C++ wrapper of the ODBC API from SAP", | ||
| "homepage": "https://github.com/SAP/odbc-cpp-wrapper", | ||
| "license": "Apache-2.0", | ||
| "dependencies": [ | ||
| { | ||
| "name": "unixodbc", | ||
| "platform": "linux | osx" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is ODBC provided on other platforms which are not
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for taking a look. In general I oriented on nanodbc and freetds, but I took the platform here from the supports config of unixodbc (https://github.com/microsoft/vcpkg/blob/b2f068faf45a3f04145bec0f52a66526ad590227/ports/unixodbc/vcpkg.json). For other platforms it must be provided by the system. Let me know if I miss something, I have no issue to change it to !windows if it makes sense.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following unixodbc's |
||
| }, | ||
| { | ||
| "name": "vcpkg-cmake", | ||
| "host": true | ||
| }, | ||
| { | ||
| "name": "vcpkg-cmake-config", | ||
| "host": true | ||
| } | ||
| ] | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| { | ||
| "versions": [ | ||
| { | ||
| "git-tree": "ebc55eefbac8df6fa1f173b7352da7d215412b51", | ||
| "version": "1.4", | ||
| "port-version": 0 | ||
| } | ||
| ] | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.