Skip to content
This repository has been archived by the owner on Aug 25, 2018. It is now read-only.

ANSI/Unicode static/shared libraries #184

Closed
wants to merge 5 commits into from
Closed

ANSI/Unicode static/shared libraries #184

wants to merge 5 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 7, 2016

Continued from #180

@ghost
Copy link
Author

ghost commented Jun 7, 2016

I believe the tests are failing because in this version of the CMake scripts, the names of the test executables are different than expected in CI.

@mloskot
Copy link
Contributor

mloskot commented Jun 7, 2016

Then, this PR is too intrusive and should be fixed - compiler/toolset specific changes should not affect the overall build configuration.

Also, this CMake warning clearly suggests something is wrong:

  Manually-specified variables were not used by the project:
    NANODBC_USE_UNICODE

as the PR should not make NANODBC_USE_UNICODE redundant in any way.

My 5 cents.

@ghost
Copy link
Author

ghost commented Jun 7, 2016

Thanks @mloskot! Of course it is intrusive. It changes the build procedure. I wast just pointing out that to perform all tests in the new precedure, I had to change the names of the executables to be as legible as possible, and that the build failure does not mean the tests would fail. In my machine in particular, the SQLite test passes for both versions of the library.

As for NANODBC_USE_UNICODE, I think it should be redundant in this case as it is the purpose of this specific PR. Note that this PR should be viewed as a feature enhancement, not a bug correction. But It is you who decide if these changes are worth it. Certainly it would break client code, due to the name of the library. Maybe my choice of names could be better to account for compatibility.

@mloskot
Copy link
Contributor

mloskot commented Jun 8, 2016

Perhaps new target could be added that preserves current name used by CI builds and runs all, ANSI and Unicode, tests against particular DBMS:

For example, for MySQL, it would be a target configured as follows:

add_custom_target(mysql_check DEPENDS mysql_ansi_test mysql_unicode_test)
add_custom_target(mysql_test DEPENDS mysql_ansi_test mysql_unicode_test)

Finally, does this PR already replace: #175? If it does, please, close #175.

@mloskot mloskot mentioned this pull request Jun 8, 2016
@ghost
Copy link
Author

ghost commented Jun 9, 2016

That's a great idea. I'll make the changes in the CMake scripts.

Yes, #175 is contained in this PR. It is closed now.

@mloskot
Copy link
Contributor

mloskot commented Jun 9, 2016

👍

@mloskot
Copy link
Contributor

mloskot commented Jul 13, 2016

FYI, We have now MinGW builds on AppVeyor (#196).
I'm going to cherry pick and copy @dsogari 's changes manually to at least make the compilation phase pass.

@mloskot
Copy link
Contributor

mloskot commented Jul 13, 2016

@dsogari I have cherry-picked two commits from the previous version of this PR (#180), ae05a4c and 912f4b0, into master.

I'm not sure what is "back to cmake version 2.8.7" commit about (d78234f).

If you want, you can git rebase master your branch in this PR and git push -f to update the PR.

Anyhow, this PR increases complexity of the CMake configuration, so I'll rely on @lexicalunit final opinion.

@lexicalunit
Copy link
Owner

Hrm, are the checks supposed to be failing here?

@ghost
Copy link
Author

ghost commented Jul 13, 2016

Thanks, @mloskot

@lexicalunit I truly do not know why the tests are failing and I did not have the time to investigate further. Perhaps we should drop this PR? I had created it only with the intent of letting you know about my approach to distributing two versions of the library that can coexist in the target machine.

FWI, I'm using a recent version of CMake to compile my forked repo and the tests for SQLite run ok.

@mloskot
Copy link
Contributor

mloskot commented Jul 14, 2016

@dsogari Those builds are month old.
If you bring your ansi-unicode-static-shared-recreated branch up to date with master, via git rebase and then update this PR via git push -f, new builds will be triggered, then we will see what is the situation.

@mloskot
Copy link
Contributor

mloskot commented Mar 2, 2017

Closing due to lack of updates for long time and unclear status of this PR.

However useful it might be, I don't think increased complexity of CMake configuration is necessary, especially that the PR completeness/correctness is not clear.

@mloskot mloskot closed this Mar 2, 2017
@ghost
Copy link
Author

ghost commented Mar 2, 2017

That's ok.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants