Skip to content

Rewrite extconf.rb, add Sybase support to test suite. #68

Closed
wants to merge 6 commits into from

2 participants

@vjt
vjt commented Dec 20, 2011

Hello,

as referenced in #11, now extconf.rb works cleanly both on platform having libiconv and others having it embedded in glibc. I've based the new one on the well-written nokogiri one, and tested compilation and cross compilation on Linux and Macos, both on 1.8.7 and 1.9.2.

Moreover, I've added a Sybase test schema and adjusted relevant tests, but the work is not complete, still many tests fail on Sybase ASE 15.0.

Eventually, I added a teardown {} that closes the connection to the database, or a new pending connection for each test would have been left open :-).

Cheers,

~Marcello

@metaskills
Collaborator

Looking over this now. FYI, the close teardown should not be needed, the connection is closed as part of the GC sweep since the TinyTds::Client does this in its rb_tinytds_client_free function.

  if (cwrap->client && !cwrap->closed) {
    dbclose(cwrap->client);
@metaskills
Collaborator

Something @luislavena pointed out to me. First, you should know that our extconf.rb was done a certain way so that I could look for things by the load path and let the paths altered by miniportile take precedence. This meant that when we were testing locally, it was using and liking to all the static libs downloaded by miniportile. Ignoring the fact that he just pointed out a bug in our current code on master not linking to the static version, you can see how your changes to extconf makes things worse.

(kencollins@station) - (~/Repositories/tiny_tds) ree master_me 
 ∴ otool -L lib/tiny_tds/tiny_tds.bundle
lib/tiny_tds/tiny_tds.bundle:
    /opt/local/lib/libiconv.2.dylib (compatibility version 8.0.0, current version 8.1.0)
    /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 159.1.0)
    /usr/lib/libobjc.A.dylib (compatibility version 1.0.0, current version 228.0.0)
(kencollins@station) - (~/Repositories/tiny_tds) ree sybase 
 ∴ otool -L lib/tiny_tds/tiny_tds.bundle 
lib/tiny_tds/tiny_tds.bundle:
    /opt/local/lib/libct.4.dylib (compatibility version 5.0.0, current version 5.0.0)
    /opt/local/lib/libsybdb.5.dylib (compatibility version 6.0.0, current version 6.0.0)
    /opt/local/lib/libiconv.2.dylib (compatibility version 8.0.0, current version 8.1.0)
    /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 159.1.0)
    /usr/lib/libobjc.A.dylib (compatibility version 1.0.0, current version 228.0.0)

So, we should do a few things. I'll chat with @luislavena and see if I can get the libiconv bug in master worked out. But can you take a less copy and paste approach from nokogiri and just patch our extconf to address your needs from ticket #11?

@metaskills
Collaborator

Or patch up our copy of extconf.rb to use the path.

@metaskills
Collaborator

Ah, I have this diff https://gist.github.com/1504331 on top of these commits for extconf.rb with these results here.

 ∴ otool -L lib/tiny_tds/tiny_tds.bundle 
lib/tiny_tds/tiny_tds.bundle:
    /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 159.1.0)
    /usr/lib/libobjc.A.dylib (compatibility version 1.0.0, current version 228.0.0)

@luislavena If you are watching, your point should be fixed when I merge this work in.

@metaskills metaskills added a commit that referenced this pull request Dec 21, 2011
@metaskills metaskills Changes to Issue #68 for Sybase and libiconv/extconf.
  * Organize README to be more general vs SQL Server / Sybase
  * Make teardown for @client.close global to the test case.
  * Change nokogiri copy of extconf.rb to insert our paths/dirs before other
    dirs so that local ports win.
  * Make a single 'from a stored procedure' test with internal db switches.
  * Keep schema/sybase_ase.sql line by line consistent with other files.
  * Remove the "not for sybase ASE" section and put back the ntext and
    uniqueidentifier tests in the standard section with simple sybase_ase?
    unless checks on the should block. Keeps things organized like I had
    and conditional for sybase.
  * Create a new sqlserver? class/instance test helper.
556dc6a
@metaskills
Collaborator

OK, changes are in master with my touch up mentioned above on top. Please QA and let me know if we need another round.

@metaskills metaskills closed this Dec 21, 2011
@vjt
vjt commented Dec 21, 2011

All good - compiles cleanly and works perfectly both on Linux and OSX. Thanks! :-)

@metaskills
Collaborator

Thanks, please feel free to keep submitting Sybase stuff. Always need help with tests and documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.