MacOS compiler error on wcsnrtombs (UNICODE) #251 #252

pedro-vicente · 2020-11-12T22:27:47Z

What does this PR do?

Fix #251

What are related issues/pull requests?

Tasklist

Environment

Provide environment details, if relevant:

DBMS name/version:
ODBC connection string:
OS and Compiler:

Fix described in

#251

lexicalunit · 2020-11-12T23:03:35Z

Nice! Assuming this passes the full test suite this PR is a win in my book. Thank you for the work!

pedro-vicente · 2020-11-13T02:43:24Z

Nice! Assuming this passes the full test suite this PR is a win in my book. Thank you for the work!

the total build actually has more compiling errors on the examples (unrelated to this)
If I have some time, I will try to fix it, but it seems it was pretty simple

also the Cmake change , you probably want to clean it a bit, I ignored the previous ODBC detection
and replaced with mine, basically to detect this path

find_library(ODBC_LIBRARY NAMES odbc PATHS "/usr/local/lib")

pedro-vicente · 2020-11-13T02:49:31Z

My original intent was to debug a nasty UNICODE string conversion in MacOS
going by your source code , I got a simple solution for it, as explained here

https://stackoverflow.com/questions/34861875/unicode-sqldriverconnectw-unixodbcdriver-managerdata-source-name-not-foun

defined this macro, like you do

#define WODBC_TEXT(s) u##s

and then

use std::u16string and do in the function call both a cast to SQLWCHAR* and the string to char* call

std::u16string conn;
(SQLWCHAR*)conn.c_str();

pedro-vicente · 2020-11-13T02:54:18Z

your API it's very intuitive to use , but the code itself seems more complicated than it needs to be

a while ago, I did a simple ODBC C++ wrapper (no UNICODE)

https://github.com/pedro-vicente/lib_odbc

just old fashioned and effective C++ 03

lexicalunit · 2020-11-13T03:03:39Z

your API it's very intuitive to use , but the code itself seems more complicated than it needs to be

a while ago, I did a simple ODBC C++ wrapper (no UNICODE)

https://github.com/pedro-vicente/lib_odbc

just old fashioned and effective C++ 03

I'm a firm believer that, at the API level, KISS (Keep It Simple Stupid) just passes the complexity on to the users of the API. Significant complexity is often required to offer your users the facility to keep their applications simple in the first place.

mloskot · 2020-11-13T08:56:51Z

Thanks for your PR!

your API it's very intuitive to use , but the code itself seems more complicated than it needs to be
a while ago, I did a simple ODBC C++ wrapper (no UNICODE)

The conversions in the nanodbc code is a bit complicated but for reasons:

support compilers with broken or incomplete C++11/C++14 (see inline comments, the issues and PRs)
support the string conversion routines in variants before and after deprecations in C++17
support possible types aliased by nanodbc::string (four types possible!)

https://github.com/pedro-vicente/lib_odbc
just old fashioned and effective C++ 03

AFAIS and IMHO, that would too simplistic for what nanodbc aims to offer.

mloskot

I suggest to update this PR to include only changes to test/base_test_fixture.h and the CMake issue on OSX is solved in separate PR.

As I explained in previous comment, I'd prefer to use CMake 3.12 with FindODBC.cmake or incorporate detection logic from the Find-module in our configuration.

mloskot · 2020-11-13T09:02:37Z

CMakeLists.txt

+if(UNIX)
+  find_path(ODBC_INCLUDE sql.h ${find_opt})
+  if(NOT ODBC_INCLUDE)
+    message(FATAL_ERROR "sql.h header file not found")
+  else()
+    message("-- Found sql.h header file at: " ${ODBC_INCLUDE})
+  endif()
+  include_directories(${ODBC_INCLUDE})
+  find_library(ODBC_LIBRARY NAMES odbc PATHS "/usr/local/lib")
+  if(NOT ODBC_LIBRARY)
+    message(FATAL_ERROR "ODBC library not found$")
+  else()
+    message("-- Found ODBC library at: " ${ODBC_LIBRARY})
+  endif()
+  set(lib_dep ${lib_dep} ${ODBC_LIBRARY})
+endif()


I'm sorry but I'm not convinced to this solution.

I'd rather prefer to switch to CMake 3.12 or later which provides FindODBC.cmake module which I submitted a while (https://gitlab.kitware.com/cmake/cmake/-/merge_requests/2069) and is well tested on multiple Unix platforms.

If CMake 3.12 seems too new, then we should simply copy the detection from FindODBC.cmake into nanodbc CMake configuration.

By the way, our CMake configuration does need some modernisation in general, and we should strive to make it target-based and avoid commands like include_directories.

Agreed, the CMake configuration for nanodbc definitely needs to be modernized. I'm not against updating to a newer version of CMake as well.

The error that happens with your Cmake version

ld: library not found for -lodbc

is because you are setting an hard coded library relative name "odbc".

set(ODBCLIB odbc)

A possible solution is to use an absolute path for the library name.
Using

find_library(ODBC_LIBRARY NAMES odbc PATHS "/usr/local/lib")

it returns an absolute path, the recommended use

https://cmake.org/cmake/help/v3.2/command/link_directories.html

Other solution is to use the absolute path returned by

odbc_config --libs -L/usr/local/Cellar/unixodbc/2.3.9/lib -lodbc

and then use a call to

https://cmake.org/cmake/help/latest/command/target_link_libraries.html

one way to extract the absolute path

if(APPLE) # string(SUBSTRING <string> <begin> <length> <output_variable>) string(SUBSTRING ${ODBC_LINK_FLAGS} 2 -1 ODBCLIB_tmp) #trim first 3 characters "-L/" message(STATUS "ODBCLIB_tmp = ${ODBCLIB_tmp}") string(REPLACE " -lodbc" "" ODBCLIB_tmp ${ODBCLIB_tmp}) message(STATUS "ODBCLIB_tmp = ${ODBCLIB_tmp}") set(ODBCLIB "${ODBCLIB_tmp}/libodbc.dylib") message(STATUS "ODBCLIB = ${ODBCLIB}") endif()

-- ODBCLIB_tmp = /usr/local/Cellar/unixodbc/2.3.9/lib -lodbc -- ODBCLIB_tmp = /usr/local/Cellar/unixodbc/2.3.9/lib -- ODBCLIB = /usr/local/Cellar/unixodbc/2.3.9/lib/libodbc.dylib

using

https://cmake.org/cmake/help/latest/command/string.html

running the tests, I get the expected result of "1" for

REQUIRE(nid == 1);

pushed it

367ac1a

@pedro-vicente

The error that happens with your Cmake version

ld: library not found for -lodbc

is because you are setting an hard coded library relative name "odbc".

set(ODBCLIB odbc)

Please, could you try CMake 3.12 or later with its FindODBC.cmake?

dest += src.at(idx);

This is just

char s; wchar_t d; d = s;

and it's not the character conversion we want (std::widen is not either).

could you try CMake 3.12 or later with its FindODBC.cmake?

FindODBC.cmake found the correct library, but not the right include path;
So, I used it to set the library, and kept some of the old detection for the path
Regarding all string casts (that code was just a cast for the test), it's better that you decide what to use; pushed this version

https://github.com/pedro-vicente/nanodbc/commit/e3fd3d430a4fe4a6991cde665d2e1dacf83809b5

Thanks for testing.

FindODBC.cmake found the correct library, but not the right include path;

Can you tell what is the right include path?
Was the unixODBC installed from the official packages?

It's important to remember that CMake Find-modules, in general, are just guessers and as such they do not promise to accurately detect every library in every environment, and that is why Find-modules provide variables a user can define to workaround such limitations, as I explain here https://gitlab.kitware.com/cmake/cmake/-/merge_requests/2069#note_410663

The Find*.cmake modules are "guessers" only.
So, the module has some detection logic for find_module, but users may find it useful to specify the non-standard or not inspected by the module locations via ODBC_INCLUDE_DIR and ODBC_LIBRARY

We need to draw a line how far we want to push complexity of CMake script we maintain.

my bad, I had a missing call to

include_directories(${ODBC_INCLUDE_DIRS})

I installed with HomeBrew and doing

odbc_config --include-prefix /usr/local/Cellar/unixodbc/2.3.9/include

gives me a different path than the FindODBC module does, but this path also contains the same file

-- ODBC include: /usr/local/include -- ODBC library: /usr/local/lib/libodbc.dylib -- ODBC config: /usr/local/bin/odbc_config

so, now only the call to FindODBC.cmake is present

mloskot

I'm sorry but this PR has become a bit too tangled and I'm not confident it is ready to be merged.
So, I labelled it with work-in-progress for now.

As I suggested in one of comments in this review round, @pedro-vicente, I'd extract simple PR that corrects the preprocessor conditions for LLVM.

Next, let's discuss upgrade to CMake 3.12

After we upgrade to CMake 3.12, we can try again to find out what issues still need to be fixed for Mac OS, and we address those.

Thank you for your work and help @pedro-vicente

/cc @lexicalunit lex

mloskot · 2020-11-26T09:58:15Z

CMakeLists.txt

@@ -1,4 +1,5 @@
-cmake_minimum_required(VERSION 3.0.0)
+cmake_minimum_required(VERSION 3.12)


@lexicalunit What do you think about requiring CMake 3.12 and rely on FindODBC.cmake?

See #252 (comment)

Any reason not to bump all the way to 3.19?

@lexicalunit I think, users who build nanodbc with CMake installed from packages of their OS distributions may complain.

Personally, I'm always on the latest CMake.

CMakeLists.txt

mloskot · 2020-11-26T09:59:08Z

test/mssql_test.cpp

+void nanodbc_to_str(std::wstring &dest, const nanodbc::string &src)
+{
+  for (size_t idx = 0; idx < src.size(); idx++)
+  {
+    dest += src.at(idx);
+  }
+}
+


As I explained in earlier comments, this is too simplistic.

mloskot · 2020-11-26T09:59:27Z

test/mssql_test.cpp

-    auto const nid = std::stoll(sid);
+    std::wstring tmp;
+    nanodbc_to_str(tmp, sid);
+    auto const nid = std::stoll(tmp);


I'm not sure why this is needed

mloskot · 2020-11-26T10:00:35Z

test/base_test_fixture.h

@@ -16,7 +16,7 @@

 #ifdef NANODBC_ENABLE_BOOST
 #include <boost/locale/encoding_utf.hpp>
-#elif defined(__GNUC__) && __GNUC__ < 5
+#elif defined(__GNUC__) && __GNUC__ < 5 && !defined(__llvm__)


As far as I understand, this is needed for Mac OS regardless of changes to the CMakeLists.txt.
If that is correct, then I'd suggest to submit the fix to these conditions in separate PR.

@lexicalunit am I right?

It's been a while since I compiled the library but I've only ever worked on macOS myself and I've never needed these. Maybe something changed within the last couple years? 🤷‍♀️

No idea, I don't use macOS myself.
Based on @pedro-vicente experience as reported in this issue, I assume that still is a problem.

This PR has 2 issues, I don't know what is the best way to fix, I'll wait for you to make a decision

issue 1 is Mac port
as seen in MacOS compiler error on wcsnrtombs (UNICODE) #251 this is the latest MacOS; since you don't have a Mac, I suggest to do whatever changes you feel must be done on this item, test it, and push here , and I'll test on the Mac

issue 2 is CMake

I think, users who build nanodbc with CMake installed from packages of their OS distributions may complain.

the homebrew installed Cmake is

/usr/local/opt/cmake/bin/cmake --version cmake version 3.18.4

so, that should be fine

pvn added 2 commits November 12, 2020 17:18

MacOs compiler errors

3e957cf

#251

Mac build

234d0de

mloskot requested changes Nov 13, 2020

View reviewed changes

pvn added 4 commits November 14, 2020 04:47

Mac build, use ODBC absolute library path

032d152

Mac build, convert string

367ac1a

use FindODBC.cmake

e3fd3d4

use FindODBC.cmake

43fcb44

mloskot added the status/need-tests This PR needs to be completed with test cases label Nov 26, 2020

mloskot requested changes Nov 26, 2020

View reviewed changes

pedro-vicente mentioned this pull request Nov 30, 2020

use find_package for ODBC #257

Closed

5 tasks

mloskot deleted the branch nanodbc:master April 1, 2022 20:13

mloskot closed this Apr 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MacOS compiler error on wcsnrtombs (UNICODE) #251 #252

MacOS compiler error on wcsnrtombs (UNICODE) #251 #252

pedro-vicente commented Nov 12, 2020

lexicalunit commented Nov 12, 2020

pedro-vicente commented Nov 13, 2020

pedro-vicente commented Nov 13, 2020

pedro-vicente commented Nov 13, 2020

lexicalunit commented Nov 13, 2020

mloskot commented Nov 13, 2020

mloskot left a comment

mloskot Nov 13, 2020

lexicalunit Nov 13, 2020

pedro-vicente Nov 14, 2020 •

edited

pedro-vicente Nov 14, 2020 •

edited

pedro-vicente Nov 14, 2020 •

edited

pedro-vicente Nov 14, 2020 •

edited

mloskot Nov 16, 2020

pedro-vicente Nov 17, 2020

mloskot Nov 17, 2020 •

edited

pedro-vicente Nov 17, 2020

mloskot left a comment •

edited

mloskot Nov 26, 2020

lexicalunit Nov 26, 2020

mloskot Nov 29, 2020

mloskot Nov 26, 2020

mloskot Nov 26, 2020

mloskot Nov 26, 2020

lexicalunit Nov 26, 2020

mloskot Nov 29, 2020

pedro-vicente Nov 29, 2020

		@@ -1,4 +1,5 @@
		cmake_minimum_required(VERSION 3.0.0)
		cmake_minimum_required(VERSION 3.12)

MacOS compiler error on wcsnrtombs (UNICODE) #251 #252

MacOS compiler error on wcsnrtombs (UNICODE) #251 #252

Conversation

pedro-vicente commented Nov 12, 2020

What does this PR do?

What are related issues/pull requests?

Tasklist

Environment

lexicalunit commented Nov 12, 2020

pedro-vicente commented Nov 13, 2020

pedro-vicente commented Nov 13, 2020

pedro-vicente commented Nov 13, 2020

lexicalunit commented Nov 13, 2020

mloskot commented Nov 13, 2020

mloskot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedro-vicente Nov 14, 2020 • edited

Choose a reason for hiding this comment

pedro-vicente Nov 14, 2020 • edited

Choose a reason for hiding this comment

pedro-vicente Nov 14, 2020 • edited

Choose a reason for hiding this comment

pedro-vicente Nov 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mloskot Nov 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mloskot left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedro-vicente Nov 14, 2020 •

edited

pedro-vicente Nov 14, 2020 •

edited

pedro-vicente Nov 14, 2020 •

edited

pedro-vicente Nov 14, 2020 •

edited

mloskot Nov 17, 2020 •

edited

mloskot left a comment •

edited