Project

General

Profile

Bug #439

broken gmx headers

Added by Christoph Junghans over 9 years ago. Updated about 9 years ago.

Status:
Closed
Priority:
Normal
Category:
mdrun
Target version:
Affected version - extra info:
Affected version:
Difficulty:
uncategorized
Close

Description

Created an attachment (id=472)
patch to fix the headers

A patch to fix the headers.

All gmx header should be included other gmx headers with #include "dir/xxx.h", with this one can then use them after installation with
#include <gromacs/xxx.h>
and
#include <gromacs/types/xxx.h>

Please take an extra look at statusio.h, it seem useless to me.
It is used nowhere and it includes sheader.h which is also missing, and it was changed in 2003 the last time.

0001-fixed-broken-headers.patch (11.6 KB) 0001-fixed-broken-headers.patch patch to fix the headers Christoph Junghans, 06/15/2010 03:42 PM

Associated revisions

Revision 3b7fbe74 (diff)
Added by Sander Pronk over 9 years ago

Fixed header files to be able to link 3d party code against gmxlib

The header files (but not the source files) now respect the
difference between #include "" and #include <>, and
it should now be possible to link against gromacs libraries
when they're installed. Fixes bug #439.

History

#1 Updated by Christoph Junghans over 9 years ago

One can find broken headers with (in bash):

for i in $(find include -name "*.h"); do
for h in $(sed -n 's/^#include "\([^"]*\)".*$/\1/p' $i); do
[ -f "${i%/*.h}/$h" ] || echo Header $h missing in file $i
done
done

#2 Updated by Christoph Junghans over 9 years ago

Not to forget to take a look at gmx header which are included with <>:

for i in $(find include -name "*.h"); do
sed -n 's/^#include <\([^"]*\)>.*$/\1/p' $i;
done | sort -u

but this look ok for me, after the patch

#3 Updated by Berk Hess over 9 years ago

I removed statusio.h.
We should fix this include formatting.

Berk

#4 Updated by Christoph Junghans over 9 years ago

I think it is not good to have the

#ifdef HAVE_CONFIG_H
#include <config.h>
#endif

in the headers for two reasons.

1. config.h is not installed
2. other project have their own config.h which will lead to conflicts

I see that the definition of GMX_DOUBLE (or so) is needed in some headers, that is why I would rename config.h to gmxconfig.h and install it as well.

#5 Updated by Sander Pronk over 9 years ago

Would installing it into gromacs/config.h lead to problems? That way we can keep the code the same but still have a unique name for config.h

#6 Updated by Christoph Junghans over 9 years ago

that is fine if you change the block to

#ifdef HAVE_CONFIG_H
#include "config.h"
#endif

(mind the "")

However, then the problem will come due to define HAVE_CONFIG_H, which is maybe defined or not by our project. I think it would be much cleaner to rename it to gmxconfig.h, that is what also the fftw people do.

#7 Updated by Sander Pronk over 9 years ago

Good point. It's not entirely clear to me, either, whether the HAVE_CONFIG_H is appropriate at all for source that changes its programming interface depending on the configure options.
I'll bring it up for discussion.

#8 Updated by Erik Lindahl over 9 years ago

Hi,

Formally it's a bad idea to use config.h in any API headers. The problem is that the headers might be common for several architectures, and there are no guarantees that you are compiling with the same compiler/settings/etc.

My gut feeling is that we could get rid of it fairly easy in the headers, probably not more work than we'd have to do to change it to a different name everywhere.

For instance, in vec.h we can simply use the slow/vanilla invsqrt routines if HAVE_CONFIG_H isn't set, and for all places (simple.h ?) where there are mutually exclusive options we can just have sure the default alternative when HAVE_CONFIG_H isn't set makes sense.

The only thing I can imagine is that the user might have to define GMX_DOUBLE when using double precision, and possible GMX_MPI when compiling a parallel tool, but that is easy to do on the command line of course.

My fear if we start to include it as gmxconfig.h is that everybody will happily add new tests and make it much harder to remove in the future?

#9 Updated by Christoph Junghans over 9 years ago

Most headers are fixed in commit e5c846962751a682f79fd9567b8d57d7701a8551

There are 3 header left, which failed link test:
- gpp_atomtype.h & grompp.h > circular dependency
nsb.h: where is defined t_nsborder, I think the header is obsolete because it is only used by pmetest.c, which is not build anymore. Should it be removed?

- headers in thread_mpi/atomic, I guess only the headers which are really used by the system should be installed. e.g. ppc header will never work on x86, so why install them.

#10 Updated by Sander Pronk over 9 years ago

(In reply to comment #9)

Most headers are fixed in commit e5c846962751a682f79fd9567b8d57d7701a8551

Thanks!

- headers in thread_mpi/atomic, I guess only the headers which are really used
by the system should be installed. e.g. ppc header will never work on x86, so
why install them.

See the previous comment by Erik: selectively installing them only adds more complication (currently, it is necessary to have the compiler choose which of these headers is included at compile time), while removing the possibility of having an architecture-independent set of headers.

#11 Updated by Christoph Junghans over 9 years ago

Ok, you are right, for the threads headers that is fine.

So what do we do with nsb.h and gpp_atomtype.h?

#12 Updated by Erik Lindahl over 9 years ago

Could you elaborate a bit on the specific problem with these two files?

I agree that circular dependencies are bad for formal code testing reasons (we're working on that for 5.0 :-), but technically I think this should work since both files have multiple-inclusion guards?

Cheers,

Erik

#13 Updated by Christoph Junghans over 9 years ago

nsb.h uses t_nsborder, which is defined nowhere in the code, so how to compile e.g. pmetest or any program using this header.

circular dependency lead to effect that one can not include gpp_atomtype.h directly, which is ok, but not nice. We can keep that for 5.0 ;-)
$cat t.c
#include <gromacs/gpp_atomtype.h>
int main (){
return 0;
}
$gcc -c -IXXX t.c
./gromacs/gpp_atomtype.h:88: error: expected declaration specifiers or ‘...’ before ‘t_param’
./gromacs/gpp_atomtype.h:101: error: expected declaration specifiers or ‘...’ before ‘t_param’
./gromacs/gpp_atomtype.h:111: error: expected ‘)’ before ‘plist’

#14 Updated by Erik Lindahl over 9 years ago

Fixed. nsb.h has been deprecated, and the cross-inclusion dependency broken by moving selected topology I/O code to src/kernel/pdb2top.h. Committed into git master.

#15 Updated by Christoph Junghans over 9 years ago

Thanks, but the including of config.h has still to be solved.

Aside from GMX_THREADS and GMX_DOUBLE, which I will add to cflags in the pkg-config files, I see no big problem to remove that include.

In funtil.h and string2.h there are some HAVE_XXX defines, which are useless without config.h, but that's it.

#16 Updated by Christoph Junghans about 9 years ago

-DGMX_THREADS, -DGMX_DOUBLE and friends have been added to the CFLAGS stored in the pkg-config files (commit 053e0c52d9937eac9873ed2fa890e2b273488c8f and commit b5d842ff1d94174b6e70120855ded6d5d62e6f51). Example:
$ pkg-config --libs --cflags libmd_d
-DGMX_DOUBLE -DGMX_SOFTWARE_SQRT -L/sw/linux/gromacs/4.0/lib -lmd_d -lgmx_d -lfftw3 -lm

This makes it is very easy for the user to build own gmx tools, see
Makefile.pkg and CMakeLists.txt.template in share/template for examples or just use:

gcc `pkg-config --libs --cflags libmd_d` template.c

In the next step I will move the '#ifdef HAVE_CONFIG_H' blocks from the .h files to the .c files. This will avoid the config.h header clash with third party programs like votca, which are useing autotools too, and includes a gromacs header in the source.

@Eric: Is this fine with you?

#17 Updated by Erik Lindahl about 9 years ago

Hi,

I see a couple of potential problems we need to solve first:

1. How does this interfere with headers that checks for sizes of (integer) type, in particular simple.h?
2. Sander: Will this do something potentially bad to thread-mpi?

Basically, for every header we remove it from we need to go through and make sure it doesn't do horrible things to trajectory IO and stuff like that. If it's just a matter of lower performance in a user analysis tool we can live with it.

#18 Updated by Sander Pronk about 9 years ago

The thread_mpi include files don't contain anything that depends on config.h; there are a bunch of compiler and platform specific header files, but any dependencies are all automatically handled at compile time.

#19 Updated by Christoph Junghans about 9 years ago

Well, for gromacs internal stuff the moving of '#include <config.h>' to the .c files makes no difference if we add it to all .c files.
And for third party programs it also make no difference, because after the installation of gromacs config.h is not accessible anyway.

I see the problem with SIZEOF_INT in types/simple.h, but I have no easy solution to that.

#20 Updated by Erik Lindahl about 9 years ago

I had a look through the headers, and while most things in config.h aren't used, we need to solve the following problems before removing that include:

1. How do we make sure to include the right headers to get the type time_t?
2. How do we make sure bool is available?
3. How do we detect a 64-bit integer type to use for ?
4. Is any "inline" keyword supported, and what is it?
5. set gmx_off_t to a 64-bit type if fseeko() or _fseeki64() is present, otherwise use long int for it.
6. Is size_t available? Otherwise set it to unsigned int.
7. How do we call Fortran77 functions? (now F77_FUNC macro)
8. How can we check if unistd.h and sched.h are available, so they can be used in thread_mpi headers?

Item (3) I think I can do by checking limits, and for item (7) I think we can add a default scheme if none is defined.

The remaining ones aren't entirely trivial, unfortunately.

#21 Updated by Erik Lindahl about 9 years ago

Fixed in commit 3f62f5f2035225b68bca6de83d84de14233ae856. The install headers no longer depend on config.h.

We still check for it in types/simple.h, though, but don't need a gromacs-specific config.h. Any config.h file can help us with getting the integer types right...

Also available in: Atom PDF