Skip to content
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

[nanodbc] Allow specifing the ODBC version for nanodbc #17974

Merged
merged 1 commit into from
Jul 23, 2021

Conversation

AnthonyCalandra
Copy link
Contributor

@AnthonyCalandra AnthonyCalandra commented May 18, 2021

This patch adds the option to specify an ODBC version directly to the nanodbc library, which up to this point has been looking for ODBC versions itself (see here). It only passes in the NANODBC_ODBC_VERSION option to nanodbc's cmake script if this flag has been set by the user.

  • What does your PR fix?

    See the following comment on what this PR is fixing: SQL_OV_ODBC3_80 issue on Ubuntu 16.04 with unixODBC 2.3.1 nanodbc/nanodbc#114 (comment)
    I experienced this issue on a CentOS box with unixODBC 2.3.1. In summary: unixODBC 2.3.1 reports an incorrect ODBC version which nanodbc picks up, but should be overridden with the version passed in through NANODBC_ODBC_VERSION.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    Not sure I need to update the baseline file; please let me know if I do.

  • Does your PR follow the maintainer guide?

    Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    Yes

@ghost
Copy link

ghost commented May 18, 2021

CLA assistant check
All CLA requirements met.

vcpkg_configure_cmake(
SOURCE_PATH ${SOURCE_PATH}
PREFER_NINJA
OPTIONS
-DNANODBC_DISABLE_EXAMPLES=ON
-DNANODBC_DISABLE_TESTS=ON
-DNANODBC_ENABLE_UNICODE=OFF
${NANODBC_ODBC_VERSION}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't pass this in directly as an option because nanodbc's cmake file only checks to see if NANODBC_ODBC_VERSION is defined -- unlike an option which can be either ON/OFF. See: https://github.com/nanodbc/nanodbc/blob/master/CMakeLists.txt#L78

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the value of NANODBC_ODBC_VERSION is passed on to the compilation:

https://github.com/nanodbc/nanodbc/blob/cbc4ec2418a9feec559e06ea5dc0a632f305d5d1/CMakeLists.txt#L81-L82

And how is it specified for building port nanodbc? Would I need to modify the triplet?

Copy link
Contributor Author

@AnthonyCalandra AnthonyCalandra May 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my original comment wasn't clear enough. Yes, it is passed to the compilation, but I'm talking about embedding the option directly, i.e.

vcpkg_configure_cmake(
    SOURCE_PATH ${SOURCE_PATH}
    PREFER_NINJA
    OPTIONS
        -DNANODBC_DISABLE_EXAMPLES=ON
        -DNANODBC_DISABLE_TESTS=ON
        -DNANODBC_ENABLE_UNICODE=OFF
        -DNANODBC_ODBC_VERSION=${NANODBC_ODBC_VERSION}
)

I made this comment to make it clear I intentionally didn't do it this way.

As for your second question, that's correct, you could add a line to the triplet file, and that's what I did on my machine (another work-around without adding this patch would be setting the flag as part of VCPKG_CXX_FLAGS but this patch is more appropriate IMO).

@JackBoosY
Copy link
Contributor

Depends on #17983.

@JackBoosY JackBoosY added the category:port-bug The issue is with a library, which is something the port should already support label May 18, 2021
@PhoebeHui PhoebeHui changed the title Allow specifing the ODBC version for nanodbc. [nanodbc] Allow specifing the ODBC version for nanodbc. Jul 21, 2021
@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Jul 22, 2021
@JackBoosY JackBoosY changed the title [nanodbc] Allow specifing the ODBC version for nanodbc. [nanodbc] Allow specifing the ODBC version for nanodbc Jul 22, 2021
@vicroms vicroms merged commit 7a985e5 into microsoft:master Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants