Update MacOS Version for ODBC Driver #136

forestmvey · 2022-10-18T17:48:28Z

Description

Updating MacOS from 10.15 to 12 poses compilation issues for libraries aws-sdk-cpp and googletest. A better solution than building from source is to manage dependencies through a package manager like vcpkg. Libraries rapidjson and rabbit I could not eliminate building from source completely. Windows failed to compile with Visual C++ 2019, and rabbit is not included in vcpkg.

Compilation Error Fixes -

VS 2022:

Addition of <algorith> includes

error C2039: 'any_of': is not a member of 'std'

Changes from std::string to Aws::String

error C2440: 'initializing': cannot convert from 'A
ws::String' to 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>'

XCode 13.3:

Removal of basic_array(const basic_array& other) sql-odbc/libraries/rabbit/include/rabbit.hpp

src/../libraries/rabbit/include/rabbit.hpp:1153:3: error: definition of implicit copy assignment operator for 'basic_array<rabbit::details::value_ref_traits<rapidjson::UTF8<>>>' is deprecated because it has a user-provided copy constructor [-Werror,-Wdeprecated-copy-with-user-provided-copy]
  basic_array(const basic_array& other)

Removal of bindpara_msg_to_utf8 function as it is never used, contains unused variables, and has potential for exploit when executing malloc(strlen(buff)) with a non-null terminated buffer.
Multiple deletions for un-used variables made throughout.

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

codecov · 2022-10-18T17:57:28Z

Codecov Report

Merging #136 (7bdf3e2) into integ-update-macos-version (f9e8e1f) will not change coverage.
The diff coverage is n/a.

@@                      Coverage Diff                      @@
##             integ-update-macos-version     #136   +/-   ##
=============================================================
  Coverage                         95.21%   95.21%           
  Complexity                         3137     3137           
=============================================================
  Files                               313      313           
  Lines                              8441     8441           
  Branches                            620      620           
=============================================================
  Hits                               8037     8037           
  Misses                              350      350           
  Partials                             54       54

Flag	Coverage Δ
query-workbench	`62.76% <ø> (ø)`
sql-engine	`97.95% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Yury-Fridlyand · 2022-10-18T19:00:40Z

Can you use already installed vcpkg? See https://github.com/actions/runner-images/blob/main/images/macos/macos-12-Readme.md

sql-odbc/build_mac_release64.sh

Yury-Fridlyand · 2022-10-19T16:24:35Z

Is it possible to fix (windows build):

Could NOT find ZLIB (missing: ZLIB_LIBRARY) (found version "1.2.11")

acarbonetto · 2022-10-26T18:18:57Z

.github/workflows/sql-odbc-main.yml

@@ -103,7 +102,7 @@ jobs:
    - name: build-installer
      if: success()
      run: |
-        .\scripts\build_installer.ps1 Release Win32 .\src $Env:ODBC_BUILD_PATH $Env:AWS_SDK_INSTALL_PATH
+        .\scripts\build_installer.ps1 Release Win32 .\src $Env:ODBC_BUILD_PATH .\src\vcpkg_installed\x86-windows


we should consider making .\src\vcpkg_installed an environment variable with ODBC_BUILD_PATH

acarbonetto · 2022-10-26T18:22:44Z

sql-odbc/libraries/rabbit/include/rabbit.hpp

@@ -1150,10 +1150,6 @@ class basic_array : public basic_value<Traits, array_tag>
    : base_type(alloc)
  {}

-  basic_array(const basic_array& other)


xcode 13.3 produces error upon compilation:

src/../libraries/rabbit/include/rabbit.hpp:1153:3: error: definition of implicit copy assignment operator for 'basic_array<rabbit::details::value_ref_traits<rapidjson::UTF8<>>>' is deprecated because it has a user-provided copy constructor [-Werror,-Wdeprecated-copy-with-user-provided-copy] basic_array(const basic_array& other)

acarbonetto · 2022-10-26T18:23:39Z

sql-odbc/build_win_debug64.ps1

@@ -1,2 +1,6 @@
 $WORKING_DIR = (Get-Location).Path
+$env:VCPKG_DEFAULT_TRIPLET = 'x64-windows'
+cd src
+vcpkg install


is there any sanity check would could log in case vcpkg install fails?

Not too sure how we would accomplish this nicely. Build errors will output to CMakeOutput.log and CMakeError.log. Vcpkg errors will output to STDOUT and CMake will fail to find the libraries. Users can check their vcpkg logs but that can be dependent on how and where vcpkg is installed. We could output STDOUT to some file for vcpkg, but that doesn't seem overly useful imo.

sql-odbc/scripts/build_installer.ps1

sql-odbc/scripts/build_driver.ps1

sql-odbc/src/sqlodbc/win_unicode.c

forestmvey · 2022-10-26T23:13:54Z

sql-odbc/src/IntegrationTests/ITODBCCatalog/test_odbc_catalog.cpp

@@ -3,6 +3,7 @@
 #include "pch.h"
 #include "unit_test_helper.h"
 #include "it_odbc_helper.h"
+#include <algorithm>


Avoids compilation error:

error C2039: 'any_of': is not a member of 'std'

MaxKsyunz

Nicely done!

Why did you change from std string and util methods to AWS SDK ones in a few places? It's fine just want to understand.

Also could you make sure all shell scripts have #!/bin/bash? I noticed that the build_mac* scripts do not.

sql-odbc/build_mac_debug64.sh

sql-odbc/src/sqlodbc/win_unicode.c

sql-odbc/libraries/rabbit/include/rabbit.hpp

MaxKsyunz · 2022-10-26T18:16:07Z

sql-odbc/src/IntegrationTests/ITODBCCatalog/test_odbc_catalog.cpp

@@ -3,6 +3,7 @@
 #include "pch.h"
 #include "unit_test_helper.h"
 #include "it_odbc_helper.h"
+#include <algorithm>


How come this is necessary?

Avoids compilation error in VS 2022:

error C2039: 'any_of': is not a member of 'std'

sql-odbc/build_mac_debug64.sh

forestmvey · 2022-10-27T14:58:38Z

VS 2022 give compiler errors for using non-aws string types in Aws functions:

error C2440: 'initializing': cannot convert from 'A
ws::String' to 'std::basic_string<char,std::char_traits<char>,std::allocator<char>>'

sql-odbc/src/sqlodbc/info.c

Yury-Fridlyand · 2022-10-28T01:10:54Z

.github/workflows/sql-odbc-main.yml

+  VCPKG_X64_INSTALL_PATH: ".\src\vcpkg_installed\x64-windows"
+  VCPKG_X86_INSTALL_PATH: ".\src\vcpkg_installed\x86-windows"


No need to double backslash \\ or forward slash / here?

Some magic? Strange I would think it wouldn't work either now that I see it, but the installer failed before I made this change. I updated to use double backslashes but I'm not sure why they aren't required.

Yury-Fridlyand · 2022-10-28T01:13:30Z

I'm sure that no changes were done without a reason, but it is impossible to validate them without knowing the context.
Please, add a brief note for each [group] to PR description - that will be a great piece of documentation.

…s to be included through vcpkg. Fixed compilation errors with newer compilers on libraries required to build from source. Signed-off-by: forestmvey <[email protected]>

forestmvey · 2022-10-28T16:12:54Z

That's a good idea and would've been a smart thing to include when opening this PR. I have updated the description, thanks for the input.

MaxKsyunz reviewed Oct 18, 2022

View reviewed changes

sql-odbc/build_mac_release64.sh Outdated Show resolved Hide resolved

sql-odbc/build_mac_release64.sh Outdated Show resolved Hide resolved

MitchellGale approved these changes Oct 19, 2022

View reviewed changes

forestmvey marked this pull request as draft October 19, 2022 17:45

forestmvey force-pushed the dev-update-macos-gha-version branch 5 times, most recently from d8922fa to 000cdce Compare October 20, 2022 23:01

forestmvey changed the title ~~Dev update macos gha version~~ Update MacOS Version for ODBC Driver Oct 26, 2022

forestmvey force-pushed the dev-update-macos-gha-version branch 5 times, most recently from e7bfdcb to 83e0598 Compare October 26, 2022 15:35

forestmvey marked this pull request as ready for review October 26, 2022 16:39

Yury-Fridlyand requested review from Yury-Fridlyand, acarbonetto, GabeFernandez310, GumpacG, MaxKsyunz and MitchellGale October 26, 2022 16:51

acarbonetto approved these changes Oct 26, 2022

View reviewed changes

forestmvey force-pushed the dev-update-macos-gha-version branch from 83e0598 to e65769b Compare October 26, 2022 23:06

forestmvey commented Oct 26, 2022

View reviewed changes

MaxKsyunz reviewed Oct 27, 2022

View reviewed changes

sql-odbc/build_mac_debug64.sh Outdated Show resolved Hide resolved

forestmvey force-pushed the dev-update-macos-gha-version branch from e65769b to 8534cb1 Compare October 27, 2022 15:38

GumpacG reviewed Oct 27, 2022

View reviewed changes

sql-odbc/src/sqlodbc/info.c Show resolved Hide resolved

GumpacG approved these changes Oct 27, 2022

View reviewed changes

Yury-Fridlyand mentioned this pull request Oct 28, 2022

SQL ingestion and ops - standup opensearch-project/sql#791

Open

Yury-Fridlyand reviewed Oct 28, 2022

View reviewed changes

forestmvey force-pushed the dev-update-macos-gha-version branch from 8534cb1 to cb9029e Compare October 28, 2022 15:43

Update MacOS version to supported level. Updated suitable dependencie…

7bdf3e2

…s to be included through vcpkg. Fixed compilation errors with newer compilers on libraries required to build from source. Signed-off-by: forestmvey <[email protected]>

forestmvey force-pushed the dev-update-macos-gha-version branch from cb9029e to 7bdf3e2 Compare October 28, 2022 15:59

MaxKsyunz self-requested a review October 28, 2022 18:46

MaxKsyunz approved these changes Oct 28, 2022

View reviewed changes

MitchellGale approved these changes Oct 28, 2022

View reviewed changes

Yury-Fridlyand approved these changes Oct 28, 2022

View reviewed changes

forestmvey merged commit d420180 into integ-update-macos-version Oct 28, 2022

forestmvey deleted the dev-update-macos-gha-version branch October 28, 2022 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update MacOS Version for ODBC Driver #136

Update MacOS Version for ODBC Driver #136

forestmvey commented Oct 18, 2022 •

edited

Loading

codecov bot commented Oct 18, 2022 •

edited

Loading

Yury-Fridlyand commented Oct 18, 2022

Yury-Fridlyand commented Oct 19, 2022

acarbonetto Oct 26, 2022

acarbonetto Oct 26, 2022

forestmvey Oct 26, 2022

acarbonetto Oct 26, 2022

forestmvey Oct 26, 2022 •

edited

Loading

forestmvey Oct 26, 2022

MaxKsyunz left a comment

MaxKsyunz Oct 26, 2022

forestmvey Oct 27, 2022

forestmvey commented Oct 27, 2022 •

edited

Loading

Yury-Fridlyand Oct 28, 2022

forestmvey Oct 28, 2022

Yury-Fridlyand commented Oct 28, 2022

forestmvey commented Oct 28, 2022

		VCPKG_X64_INSTALL_PATH: ".\src\vcpkg_installed\x64-windows"
		VCPKG_X86_INSTALL_PATH: ".\src\vcpkg_installed\x86-windows"

Update MacOS Version for ODBC Driver #136

Update MacOS Version for ODBC Driver #136

Conversation

forestmvey commented Oct 18, 2022 • edited Loading

Description

Check List

codecov bot commented Oct 18, 2022 • edited Loading

Codecov Report

Yury-Fridlyand commented Oct 18, 2022

Yury-Fridlyand commented Oct 19, 2022

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

forestmvey Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxKsyunz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forestmvey commented Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yury-Fridlyand commented Oct 28, 2022

forestmvey commented Oct 28, 2022

forestmvey commented Oct 18, 2022 •

edited

Loading

codecov bot commented Oct 18, 2022 •

edited

Loading

forestmvey Oct 26, 2022 •

edited

Loading

forestmvey commented Oct 27, 2022 •

edited

Loading