Conversation
|
@microsoft-github-policy-service agree [company="SAP"] |
|
@microsoft-github-policy-service agree company="SAP" |
This reverts commit 937cdc4.
| "dependencies": [ | ||
| { | ||
| "name": "unixodbc", | ||
| "platform": "linux | osx" |
There was a problem hiding this comment.
How is ODBC provided on other platforms which are not !windows?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Following unixodbc's supports is right.
| -FIND_PACKAGE(ODBC REQUIRED) | ||
| +IF(UNIX) | ||
| + FIND_PACKAGE(unixodbc CONFIG) | ||
| + IF(unixodbc_FOUND) | ||
| + SET(ODBC_TARGET UNIX::odbc) | ||
| + ENDIF() | ||
| +ENDIF() | ||
| +IF(NOT ODBC_TARGET) | ||
| + FIND_PACKAGE(ODBC REQUIRED) | ||
| + SET(ODBC_TARGET ODBC::ODBC) | ||
| +ENDIF() |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
|
Hey, Odbccpp is an open source wrapper for the odbc api to use in c++. Currently, it is mostly used for two other open source projects GDAL and QGIS, it would be cool for them to have it available in vcpkg. |
JavierMatosD
left a comment
There was a problem hiding this comment.
Tagging team review to discuss the proper find_package call
|
|
||
| -FIND_PACKAGE(ODBC REQUIRED) | ||
| +IF(UNIX) | ||
| + FIND_PACKAGE(unixodbc CONFIG) |
There was a problem hiding this comment.
Hmm.. https://github.com/microsoft/vcpkg/blob/master/ports/unixodbc/usage#L4 I'll need to consult the team since this ports exports unofficial targets. Tagging vcpkg-team-review
There was a problem hiding this comment.
Ok, it seems this is allowed and this should use the vcpkg provided configs instead.
There was a problem hiding this comment.
Hey Javier, thanks for checking the change. I used the directions in the usage file of unixodbc: https://github.com/microsoft/vcpkg/blob/39a6cc0e44641977a7ccdfdb01a14eaf832aa330/ports/unixodbc/usage
Still, this failed. I think the correct target is unofficial::unixodbc::unixodbc, at least for me it looks like when checking the config https://github.com/microsoft/vcpkg/blob/39a6cc0e44641977a7ccdfdb01a14eaf832aa330/ports/unixodbc/unofficial-unixodbc-config.cmake
If this is correct and it makes sense, I could open an issue or push a small fix.
ports/odbccpp/vcpkg.json
Outdated
| "dependencies": [ | ||
| { | ||
| "name": "unixodbc", | ||
| "platform": "!windows" |
There was a problem hiding this comment.
The port in vcpkg supports "linux | osx"
There was a problem hiding this comment.
Fixed it. I thought the same originally, but it seems to be the common way to use !windows for other packages. https://github.com/search?q=repo%3Amicrosoft%2Fvcpkg+%22name%22%3A+%22unixodbc%22%2C+++++++%22platform%22%3A+%22%21windows%22&type=code
But I'm fine with either and for me "linux | osx" seems to be more precise, too.
This reverts commit 03d31da.
| -FIND_PACKAGE(ODBC REQUIRED) | ||
| +IF(UNIX) | ||
| + FIND_PACKAGE(unofficial-unixodbc CONFIG) | ||
| + IF(unixodbc_FOUND) |
There was a problem hiding this comment.
| + 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.
| +IF(NOT ODBC_TARGET) | ||
| + FIND_PACKAGE(ODBC REQUIRED) | ||
| + SET(ODBC_TARGET ODBC::ODBC) | ||
| +ENDIF() |
There was a problem hiding this comment.
| +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.
I changed it to an if else clause, but in general it is necessary for for Windows, I think. Hope this fits for you.
dg0yt
left a comment
There was a problem hiding this comment.
Hm, my review wasn't submitted when I wrote it.
| "dependencies": [ | ||
| { | ||
| "name": "unixodbc", | ||
| "platform": "linux | osx" |
There was a problem hiding this comment.
Following unixodbc's supports is right.
| -FIND_PACKAGE(ODBC REQUIRED) | ||
| +IF(UNIX) | ||
| + FIND_PACKAGE(unixodbc CONFIG) | ||
| + IF(unixodbc_FOUND) | ||
| + SET(ODBC_TARGET UNIX::odbc) | ||
| + ENDIF() | ||
| +ENDIF() | ||
| +IF(NOT ODBC_TARGET) | ||
| + FIND_PACKAGE(ODBC REQUIRED) | ||
| + SET(ODBC_TARGET ODBC::ODBC) | ||
| +ENDIF() |
There was a problem hiding this comment.
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.
| -FIND_PACKAGE(ODBC REQUIRED) | ||
| +IF(UNIX) | ||
| + FIND_PACKAGE(unixodbc CONFIG) | ||
| + IF(unixodbc_FOUND) | ||
| + SET(ODBC_TARGET UNIX::odbc) | ||
| + ENDIF() | ||
| +ENDIF() | ||
| +IF(NOT ODBC_TARGET) | ||
| + FIND_PACKAGE(ODBC REQUIRED) | ||
| + SET(ODBC_TARGET ODBC::ODBC) | ||
| +ENDIF() |
There was a problem hiding this comment.
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.
vcpkg.json, or explicitly disabled through patches or build system arguments such as CMAKE_DISABLE_FIND_PACKAGE_Xxx or VCPKG_LOCK_FIND_PACKAGEvcpkg.jsonmatches what upstream says.vcpkg.jsonmatches what upstream says../vcpkg x-add-version --alland committing the result.