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

Remove attempt to link xml_ruby in extconf.rb. #11

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

sshao
Copy link
Contributor

@sshao sshao commented Sep 9, 2014

Proposing that extconf.rb not link xml_ruby at all, as it doesn't seem to use any of the xml_ruby libraries when compiling. Instead rely on existing checks to ensure that libxml-ruby is installed, the existing checks being: 1) the search for libxml-ruby's installation path in extconf.rb, and 2) the .gemspec dependency upon libxml-ruby.

(We'd also like to remove this section of the extconf.rb because it prevents libxml-ruby from building on Rubinius 1.3.3, due to the linker throwing "undefined reference" errors at this stage. Rubinius does not generate any library/shared object files to link against.)

This is tested on Debian Wheezy (libxml 2.8.0, libxml 1.1.26) with both ruby MRI 1.8.7-p358 and rubinius 1.3.3.

@jc00ke
Copy link

jc00ke commented Oct 7, 2014

@blackwinter
Copy link
Contributor

@jc00ke I'm sorry, but what makes you think we could be of any help? AFAIK, project collaborators are @cfis and possibly @dreamcat4 (see here and here).

@jc00ke
Copy link

jc00ke commented Oct 8, 2014

My bad. I went to the collaborators page and your names were listed as the top contributors. My apologies for misunderstanding the Github ui.

@blackwinter
Copy link
Contributor

No worries. I find it irritating myself that there doesn't seem to be a place where the actual project collaborators, i.e. those with commit access, are listed. Good luck, though.

@cfis
Copy link
Member

cfis commented Oct 8, 2014

So libxslt-ruby does link to libxml-ruby...except on OSX. So I think that check is necessary.

@sshao
Copy link
Contributor Author

sshao commented Oct 8, 2014

Could you show me where/why it needs that link? I've installed libxslt-ruby successfully on a non-OSX system (debian wheezy) without this check -- it seems to only need -Ilibxml-ruby-2.7.0/ext/libxml to find ruby_libxml.h, and that check is covered on line 120.

@cfis
Copy link
Member

cfis commented Oct 13, 2014

The library uses the libxml-ruby shared library for access to the document object. So it needs access to the header files to compile, and the library to run.

@sshao
Copy link
Contributor Author

sshao commented Oct 13, 2014

But it doesn't need the shared library to compile. libxslt-ruby does also run successfully (tests pass, and works within a rails project) on debian wheezy + squeeze without the library linking step in extconf.

@tekwiz
Copy link
Contributor

tekwiz commented Oct 13, 2014

@sshao I think I have an idea with what's happening, but I can't confirm it at the moment. I have a potential compromise though: would it work to add to the conditional on lines 133/134 to skip that section of code for Rbx like is done for OS X?

@sshao
Copy link
Contributor Author

sshao commented Oct 13, 2014

Mostly passing Travis build of this branch, for several rubies. (The rbx-2 build fails right now because of a bug in its implementation of rb_Array that has been fixed (rubinius/rubinius@9a9c09b) and should be out in the next release.)

Travis CI VMs are "based on Ubuntu 12.04 LTS Server Edition 64 bit".

@sshao
Copy link
Contributor Author

sshao commented Oct 13, 2014

@tekwiz That compromise would definitely work for us! I'm just still uncertain that this linking is needed during compilation at all, given that the installs/tests I've tried have been working without it.

@cfis
Copy link
Member

cfis commented Oct 14, 2014

Ok, let's do something along the lines of what Travis is saying. But it should be to include that step on platforms that don't allow unfufilled references when compiling - ie, Windows.

Conflicts:
	.travis.yml

Conflict was that master .travis.yml only contained 1.8.7 in the
rvm list, and remove-xml-ruby-link .travis.yml added more
rubies in.
should not assume that date has already been loaded
@sshao
Copy link
Contributor Author

sshao commented Nov 25, 2014

@cfis @tekwiz I implemented Travis' suggestion in ba15f86, with a fully passing Travis build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants