rails-sqlserver / tiny_tds

TinyTDS - Simple and fast FreeTDS bindings for Ruby using DB-Library.
Other
606 stars 189 forks source link

Need to specify iconv location to compile. #11

Closed JuliaDana closed 12 years ago

JuliaDana commented 13 years ago

On a 64-bit version of OS X (Snow Leopard), I am unable to compile the gem as-is. The error I get is:

ld: in /usr/local/lib/libiconv.2.dylib, missing required architecture x86_64 in file

I was able to resolve the problem by using another version of iconv (that is not installed in the standard location), but this required adding the following line to ext/tiny_tds/extconf.rb

dir_config('iconv')

Could this be included as a permanent fix for other people who have iconv in a non-standard location? I'm new to attempting to contribute, so I'm not sure if this was the right way submit this. Let me know if there is something else I need to do.

Julia

metaskills commented 13 years ago

Would this have worked?

$ gem install tiny_tds -- --with-libiconv-prefix=/some/dir

metaskills commented 13 years ago

I just did this commit too. https://github.com/rails-sqlserver/tiny_tds/commit/fb3b14e93d331952707ace8a520d694e6d321198

JuliaDana commented 13 years ago

Trying

$ gem install tiny_tds -- --with-libiconv-prefix=/some/dir

did not work for me, with or without the new fix. I should have mentioned that with the fix I did have to install the gem with the following command, which is similar to what you suggested, but this command didn't work before the fix.

$ gem install tiny_tds -- --with-iconv-dir=/some/dir

Thanks for the quick response!

metaskills commented 13 years ago

OK, good to know. Thanks too!

michaelkirk commented 13 years ago

This breaks compilation on Debian (and presumably anything using glibc), which includes iconv in libc (so gcc bails when you try to link iconv).

metaskills commented 13 years ago

@michaelkirk Iconv is important for FreeTDS to compile against. Can you help me find some middle ground and proper way to structure extconf.rb? We recently added that for the future cross compilation on Windows to work which is also important for compiling locally.

Dose something like this not work for you?

$ gem install tiny_tds -- --with-iconv-dir=/some/dir

Or maybe I should make the iconv optional in some fashion. Not sure. I am very curious to find out how FreeTDS is installed on your system now. If you do tsql -C does it show iconv library: yes?

As a note to myself, I may go back to using iconv as the compile locally option, give a usage above to specify for @JuliaDana but ultimately not stop the installation process. To that end, here is a code snippet for me.

Rake::ExtensionTask.new('tiny_tds', gemspec) do |ext|
  ext.lib_dir = 'lib/tiny_tds'
  ext.config_options << "--enable-iconv" unless ENV['TINYTDS_SKIP_PORTS']
end

FREETDS_LIBRARIES = ['sybdb']
FREETDS_LIBRARIES.unshift 'iconv' if enable_config('iconv')
FREETDS_HEADERS = ['sybfront.h', 'sybdb.h']

def have_freetds?
  have_iconv = enable_config('iconv') ? have_library('iconv') : true
  have_iconv && find_freetds_libraries_path && find_freetds_include_path
end
metaskills commented 13 years ago

Or maybe this?

$ gem install tiny_tds -- --without-iconvlib
michaelkirk commented 13 years ago

@metaskills I'll happilly work with you to get this resolved. Disclaimer: most of this stuff I figured out last night while trying to get tiny_tds to work, my opinion on what's happening should probably be taken with a grain of salt.

I don't think the --with-iconv-dir=/some/dir will work, the problem is that there is no "iconv lib" on glibc systems (most anything linux). Instead, it's included as part of the basic c library, thus no explicit linking is required. Since there is no separate iconv lib, trying to link to it will break the compile process.

# will fail
$ gcc some_program_that_uses_iconv.c -liconv  
/usr/bin/ld.bfd.real: cannot find -liconv
collect2: ld returned 1 exit status

# will succeed
$ gcc some_program_that_uses_iconv.c

It's worth noting that there is an iconv header file. For me it lives at /usr/include/iconv.h, and is part of the libc6-dev package.

I'll get back to you about gem install tiny_tds -- --without-iconvlib

michaelkirk commented 13 years ago

re: gem install tiny_tds -- --without-iconvlib

I've just tried installing the 0.4.1 version of the gem, but to no avail. Here's the command line generated:

"gcc -o conftest -I. -I/usr/lib/ruby/1.8/i486-linux -I.  -D_FILE_OFFSET_BITS=64  -fno-strict-aliasing -g -g -O2  -fPIC   conftest.c  -L. -L/usr/lib -L.  -rdynamic -Wl,-export-dynamic -L/usr/local/lib     -lruby1.8-static -lfalse  -lpthread -lrt -ldl -lcrypt -lm   -lc"
/usr/bin/ld.bfd.real: cannot find -lfalse
collect2: ld returned 1 exit status

notice it's trying to link a library called "false"

If you'd like, before you publish a new version, I can test it on my environment.

metaskills commented 13 years ago

I'll do that, give me a few to redo this iconv thing.

metaskills commented 13 years ago

OK 0.4.2 is out, let me know if that fixes it. @JuliaDana, if you need to specify iconv, you should be able to pass options down like this.

$ gem install tiny_tds -- --enable-iconv --with-iconv-dir=/some/dir
michaelkirk commented 13 years ago

succeeded! Thanks

On Thu, Mar 24, 2011 at 11:10 AM, metaskills < reply@reply.github.com>wrote:

OK 0.4.2 is out, let me know if that fixes it. @JuliaDana, if you need to specify iconv, you should be able to pass options down like this.

$ gem install tiny_tds -- --enable-iconv --with-iconv-dir=/some/dir

Reply to this email directly or view it on GitHub: https://github.com/rails-sqlserver/tiny_tds/issues/11#comment_913459

metaskills commented 13 years ago

w00t

asdavey commented 12 years ago

I just came across this error but from a different angle.

I'm on cygwin (1.7). When running gem install tiny_tds the installation would fail while trying to build the extension. The gist of the error message was an undefined reference to '_libinconv_open'.

I confirmed that libiconv was installed, but I couldn't work out why the gem wasn't linking against it.

Once I found the line in extconf FREETDS_LIBRARIES.unshift 'iconv' if enable_config('iconv') and I worked out how to use that when invoking gem: gem install tiny_tds -- --enable-iconv all was good.

What was really confusing was I didn't have this problem on another cygwin machine. After hunting around I discovered that if you have libiconv previously installed (which I did on one machine and not the other) when you build freetds it will be compiled with iconv without you having to do anything (if you don't have libiconv it will compile fine too). But if your freetds was compiled with iconv, then the only way to get tiny_tds to install is to pass the enable_iconv argument.

I think that the README should include some text explaining that if you are having problems installing, and you suspect that your freetds was compiled with iconv then you will need to pass the enable-iconv argument. It should also show an example since a lot of windows devs won't be familiar with how to pass args to the extension builder from gem install.

metaskills commented 12 years ago

Interesting, but TinyTDS has native gems for Windows precompiled so no "need" for anyone Windows to download and compile FreeTDS or libiconv since it is all statically linked in the x86-mingw32 platform gem.

https://rubygems.org/gems/tiny_tds/versions

I'm not Windows specialist, but I believe cygwin is nothing akin to mingw32 and hence you would not get that by default, but wouldn't it still work if you explicitly installed that platform version? Much unknowns to me on that topic? @luislavena any opinions?

@asdavey, can you work up what I should include in the README or something?

luislavena commented 12 years ago

I believe in the case of cygwin, it should be used like POSIX, meaning: install libiconv and install FreeTDS linking to libiconv, and then install tiny_tds, the need for --enable-libiconv will not be required since FreeTDS would be already linked to it.

Cygwin has a package manager so installing libiconv shouldn't be complicated, then compile FreeTDS should be able to detect iconv (and if not, you can force that).

AFAIK, haven't used cygwin in many years but that will be the right behavior.

asdavey commented 12 years ago

@luislavena there is a good chance that I'm doing something wrong given I don't have a great deal of experience in getting ruby extensions to compilet (my 3 years ruby experience has all been in the cozy confines of jruby). But if you don't supply the -enable-iconv, isn't extconf.rb set to NOT include libiconv as one of the link flags?

On my machine, cygwin has libiconv installed as a package, and when I built freetds it picked up libiconv without any configuration changes (I assume that ./configure just found the libs and did what it had to do to link iconv).

But tiny_tds would not install. Running ruby extconf.rb in the extension lib worked, but make was another story. I initially thought it was because the iconv libs were in /usr/lib and /usr/include whereas my freetds libs are in /usr/local/lib and /usr/local/include. But adding some symbolic links in the /usr/local folders to the appropriate files didn't solve my problem.

This portion of output from make might help diagnose whats going on:

gcc -shared -s -o tiny_tds.so client.o result.o tiny_tds_ext.o -L. -L/home/Andrew/.rvm/rubies/ruby-1.9.2-p290/lib -L/usr/local/lib -L.  -L/usr/local/lib -Wl,--enable-auto-image-base,--enable-auto-import  -lruby191 -lsybdb  -lpthread -lrt -ldl -lcrypt
/usr/local/lib/libsybdb.a(iconv.o): In function `tds_set_iconv_name':
/home/Andrew/sdk/freetds-0.91/src/tds/iconv.c:206: undefined reference to `_libiconv_open'

I was surprised that the error message lists the location of the source code that was used to build freetds (this probably just highlights my lack of compiling C stuff experience). I guess I was thinking the error would have been in the source of the extension, not because some third-party binary can't find one of it's dependencies. Is there some sort of dynamic link going on here vs static link?

@metaskills I'll get something for you to look at shortly, assuming that @luislavena doesn't point out that I did something stupid.

luislavena commented 12 years ago

@asdavey

Seems you're right, without --enable-libiconv it will not link properly because FreeTDS links against it but our extension doesn't.

I believe a warning/note should be added to the README so others do not suffer the same issue you had.

asdavey commented 12 years ago

@metaskills, the update to the README could read (as the last paragraph of the Install section):

On some systems (e.g. Cygwin) FreeTDS is linked against libiconv. If you have problems installing the gem due to building the extension, try: `gem install tiny_tds -- --enable-iconv`. This will instruct the build steps to include the libiconv library when linking.

But I was wondering if it is better to just fix the build process so that it detects if libiconv is available, and if so, links against it.

So I made some small modifications to extconf.rb and then tested that the build process would work with and without libiconv being available on the system. I was able to successfully get the extension built in the following scenarios:

| Ruby Version | Platform           | FreeTDS linked against libiconv? |
| 1.9.2        | Cygwin 1.7         | Yes                              |
| 1.9.2        | Cygwin 1.7         | No                               |
| 1.9.2        | Ubuntu 10.10 (x86) | No                               |
| 1.9.2        | Ubuntu 10.10 (x86) | Yes                              |
| 1.9.2        | mingw32            | Yes                              |
| 1.9.2        | mingw32            | No                               |
| 1.8.7        | mingw32            | Yes                              |
| 1.8.7        | mingw32            | No                               |

I believe that the modifications should still support the scenarios raised by @JuliaDana (you can still tell extconf where to find libiconv) and @michaelkirk (if libiconv is not on the system the compile or glibc is being used instead then the compile will still work). Though full disclaimer, I'm pretty new to all this compiling C code business.

Here are the changes. In summary, I've removed iconv being conditionally added to FREETDS_LIBRARIES (and the related code path) and instead directly call have_library("iconv") - to the best of my understanding the build will only link against libiconv if have_library is successful.

diff --git a/ext/tiny_tds/extconf.rb b/ext/tiny_tds/extconf.rb
index 9aa6cc5..817622c 100644
--- a/ext/tiny_tds/extconf.rb
+++ b/ext/tiny_tds/extconf.rb
@@ -1,10 +1,9 @@
 require 'mkmf'

 FREETDS_LIBRARIES = ['sybdb']
-FREETDS_LIBRARIES.unshift 'iconv' if enable_config('iconv')
 FREETDS_HEADERS = ['sybfront.h', 'sybdb.h']

-dir_config('iconv') if enable_config('iconv')
+dir_config('iconv') 
 dir_config('freetds')

 def root_paths
@@ -70,6 +69,8 @@ def have_freetds?
   find_freetds_libraries_path && find_freetds_include_path
 end

+have_library("iconv")
+
 if enable_config("lookup", true)
   unless have_freetds?
     abort "-----\nCan not find FreeTDS's db-lib or include directory.\n-----"
@@ -79,6 +80,6 @@ else
   unless have_freetds_libraries?(*FREETDS_LIBRARIES) && have_freetds_headers?(*FREETDS_HEADERS)
     abort "-----\nCan not find FreeTDS's db-lib or include directory.\n-----"
   end
-end
+end 

 create_makefile('tiny_tds/tiny_tds')
metaskills commented 12 years ago

Wow, this is a lot to consume, really good information too! I had always assumed that if FreeTDS was compiled and found libiconv that it would negate TinyTDS needing to do the same. But after thinking about this, seems the later makes sense too. I am going to apply the diff and see if we can get some people to test. Repoened the issue too.

metaskills commented 12 years ago

I like the patch to the build too. Confirmed that this works well on both local rake compile as well as during a gem install without miniportile. I think I'll release a 0.5.1.rc1 to see how it goes.

metaskills commented 12 years ago

I had to do this commit too so cross compile works.

https://github.com/rails-sqlserver/tiny_tds/commit/df41ff564b2ba4ef379e7d81bf09674912d31058

Basically we have to put iconv in the standard libraries we need and take out that root have_library since we need to use the path search. This makes it work with cross compile too.

metaskills commented 12 years ago

OK, I have done a 0.5.1.rc1 release. Would love to know how it goes. Please let me know.

asdavey commented 12 years ago

I was able to test this today on my cygwin box and everything worked as expected. Later I'll be able to do the mingw and ubuntu tests.

Out of curiosity why does extconf.rb have the exotic path search vs using stock standard 'have_library' calls? (not trying to sound snarky - I am genuinely interested).

metaskills commented 12 years ago

No worries. I copied that path first search loop concept from other native gems like nokogiri, mysql2, etc. It turns out to be a win for us because when we are running our local tests or building native gems using miniportile, the path is altered so the local downloaded libraries have precedence. I suspect this process plays nicely with other build scenarios too.

vjt commented 12 years ago

Hi, unluckily adding back iconv in the list of required freetds libraries breaks compilation on Linux again :-).

I'd suggest, because it's a MacOS specific issue to link against libiconv, to add the library only on that platform. Or, to not try to link against iconv on Linux - as it is included in the glibc.

What do you think?

metaskills commented 12 years ago

I would be willing to open this ticket and apply another patch, but I am not sure what you suggested is best. There has to be some way to make extconf.rb happy for all. Other gems have solved this, say like nokogiri.

https://github.com/tenderlove/nokogiri/blob/master/ext/nokogiri/extconf.rb#L94-107

They have a few open tickets about iconv too, but I think that code above is fine?

https://github.com/tenderlove/nokogiri/issues/381 https://github.com/tenderlove/nokogiri/issues/442

So my question is, what are others doing, what have they learned and how can I make this work for everyone?

vjt commented 12 years ago

I'd go with have_func('iconv_open', 'iconv.h') or have_library('iconv', 'iconv_open', 'iconv.h') -- this way if iconv_open is available in glibc (Linux) it'll go ok, otherwise it'll look for the function in libiconv (MacOS, BSD, ...).

The "asplode" bit is über nice as well, tenderlove style :D -- and the search paths as well. Anyway I'd refrain from grabbing too much from that source, less is more.

My .2c, I'll be able to work on this tomorrow (CEST)

PS. we're maintaining a Sybase adapter for Rails that uses TinyTDS, if you're interested, on https://github.com/ifad/activerecord-sybase-adapter

Thanks!!1oneone

~Marcello (via iPhone)

~ marcello.barnaba@gmail.com ~ http://sindro.me/ ~ http://www.linkedin.com/in/marcellobarnaba

On Dec 19, 2011, at 8:19 PM, Ken Collins reply@reply.github.com wrote:

I would be willing to open this ticket and apply another patch, but I am not sure what you suggested is best. There has to be some way to make extconf.rb happy for all. Other gems have solved this, say like nokogiri.

https://github.com/tenderlove/nokogiri/blob/master/ext/nokogiri/extconf.rb#L94-107

They have a few open tickets about iconv too, but I think that code above is fine?

https://github.com/tenderlove/nokogiri/issues/381 https://github.com/tenderlove/nokogiri/issues/442

So my question is, what are others doing, what have they learned and how can I make this work for everyone?


Reply to this email directly or view it on GitHub: https://github.com/rails-sqlserver/tiny_tds/issues/11#issuecomment-3207990

metaskills commented 12 years ago

I reopened the ticket. Throw me a patch and I will put this in RC2.

vjt commented 12 years ago

Library compiles both on Linux and OSX, now making all tests pass on Sybase ASE 15.0 :-).

metaskills commented 12 years ago

Is there a patch or pull request?

vjt commented 12 years ago

Still not - I'm verifying the cross compilation is OK with the new extconf. I'll send you a pull soon.

vjt commented 12 years ago

Pull #68 is on for your review :-)

metaskills commented 12 years ago

Please let me know how the latest master works and if I should re-open this or not. If not, I will create an RC2.

metaskills commented 12 years ago

FYI, the last change we had was in ticket #69

tbuyle commented 12 years ago

Hello,

I am hacing the libiconv is missing error while trying to install the gem :

ERROR:  Error installing tiny_tds:
ERROR: Failed to build gem native extension.

        /Users/myuser/.rvm/rubies/ruby-1.9.2-p290/bin/ruby extconf.rb --enable-iconv --with-iconv-dir=/mydir/
checking for iconv_open() in iconv.h... no
checking for iconv_open() in -liconv... no

I tried several variation of the --with-iconv-dir but can not get it to work.

LMaybe I'm just poiting to the wrond dir... which directory should it be ?

luislavena commented 12 years ago

@tbuyle needs to be the directory of a installed libiconv installation.

If you manually installed libiconv, then will be the directory defined by --prefix or the default which I believe is /usr/local

tbuyle commented 12 years ago

I installed libiconv using HomeBrw - should the dir then be /usr/local/Cellar or/usr/local/Cellar/libiconv ?

luislavena commented 12 years ago

@tbuyle no idea about homebrew, probably you should read their documentation or ask them.