search

mittinatten / freesasa

C-library for calculating Solvent Accessible Surface Areas
http://freesasa.github.io/
MIT License
105 stars 37 forks source link

Fixes forced libxml dependency #29

Closed bp-kelley closed 6 years ago

bp-kelley commented 6 years ago

Small fix to remove the libxml dependency when USE_XML is set to 0

We might consider also adding an

#ifndef USE_XML

as well.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 97.763% when pulling ad407782f7cf6921307057d4037adf571375591f on bp-kelley:fix/json_xml_dependencies into aef643bbbaab12c28a2d8f7dc96aad71dd8e3d39 on mittinatten:master.

mittinatten commented 6 years ago

Hi! Looks good, but could you submit the PR to the windows branch (if that's possible), just to keep things clean.

bp-kelley commented 6 years ago

Absolutely, FYI this patch is required on unix as well if libxml doesn't exist.

On Tue, Sep 12, 2017 at 2:08 PM, Simon Mitternacht <notifications@github.com

wrote:

Hi! Looks good, but could you submit the PR to the windows branch (if that's possible), just to keep things clean.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mittinatten/freesasa/pull/29#issuecomment-328935892, or mute the thread https://github.com/notifications/unsubscribe-auth/AJbioFH0tYykaZmPaRphEkj_1nYSLZ6_ks5shsiUgaJpZM4PUxJQ .

mittinatten commented 6 years ago

Hmm, I thought I had tests for that. But if so, we can just keep it this way.

mittinatten commented 6 years ago

I see now that I install libxml in all the test setups, it's just not enabled in all of them. Will try to fix this.

bp-kelley commented 6 years ago

Thanks :) We're very close to releasing the RDKit integration btw. I'll let you know when we pull the trigger.

On Tue, Sep 12, 2017 at 2:50 PM, Simon Mitternacht <notifications@github.com

wrote:

I see now that I install libxml in all the test setups, it's just not enabled in all of them. Will try to fix this.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mittinatten/freesasa/pull/29#issuecomment-328946707, or mute the thread https://github.com/notifications/unsubscribe-auth/AJbioNLm6vJl5OE7tmFEtU2GhOKTLvgqks5shtKDgaJpZM4PUxJQ .

mittinatten commented 6 years ago

Good to hear!

mittinatten commented 6 years ago

Ok, so I changed the Travis file so that one of the tests will not have libxml and JSON-C installed, but it still doesn't fail for me (in a branch that doesn't have the above changes, https://travis-ci.org/mittinatten/freesasa/jobs/274742471). In one of my configuration attempts (https://travis-ci.org/mittinatten/freesasa/jobs/274739213) I forgot to install them in one case where they were required and the configure script failed on JSON-C - so at least that one isn't already installed by default on trusty.

As you can see in the configure file (https://github.com/mittinatten/freesasa/blob/master/configure.ac#L77) USE_XML is defined as 0 if the library is disabled, and 1 if it is enabled and present. So it should always be defined. Could there be something else was causing problems on your Unix system? I'm by no means an autotools expert, could there be incompatibilities between versions?

Either way, it doesn't matter that much which form we use I guess, it's only a matter of style, but I don't understand were the problem was on Unix systems.

mittinatten commented 6 years ago

Or did you get a linker error for freesasa_write_xml()? The differences would make sense if that statement was optimized away by my compiler and not yours (since USE_XML and USE_JSON are constants).

bp-kelley commented 6 years ago

The problem is that if you don't use ifdefs the symbols from libxml need to be found. I.e. you need to link to libxml.

At least that is the behavior I'm seeing when building with the rdkit build system.

Brian Kelley

On Sep 12, 2017, at 3:31 PM, Simon Mitternacht notifications@github.com wrote:

Ok, so I changed the Travis file so that one of the tests will not have libxml and JSON-C installed, but it still doesn't fail for me (in a branch that doesn't have the above changes, https://travis-ci.org/mittinatten/freesasa/jobs/274742471). In one of my configuration attempts (https://travis-ci.org/mittinatten/freesasa/jobs/274739213) I forgot to install them in one case where they were required and the configure script failed on JSON-C - so at least that one isn't installed by default.

As you can see in the configure file (https://github.com/mittinatten/freesasa/blob/master/configure.ac#L77) USE_XML is defined as 0 if the library is disabled, and 1 if it is enabled and present. So it should always be defined. Could there be something else was causing problems on your Unix system? I'm by no means an autotools expert, could there be incompatibilities between versions?

Either way, it doesn't matter that much which form we use I guess, it's only a matter of style, but I don't understand were the problem was on Unix systems.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

bp-kelley commented 6 years ago

Yes, we're not doing dead code analysis.

mittinatten commented 6 years ago

Ok, then it makes sense

mittinatten commented 6 years ago

I'd like to have a test case for this in my Travis configuration to avoid regression. I've tried to generate the error by turning off dead code elimination. When I compile with gcc flags -fno-dce -fno-dse -fno-tree-dce -fno-tree-dse I still don't get an error when libxml and libjsonc are missing from the system.

A quick search didn't give any obvious flags to use for clang.

What compiler and flags did you use?