search

etmc / tmLQCD

tmLQCD is a freely available software suite providing a set of tools to be used in lattice QCD simulations. This is mainly a HMC implementation (including PHMC and RHMC) for Wilson, Wilson Clover and Wilson twisted mass fermions and inverter for different versions of the Dirac operator. The code is fully parallelised and ships with optimisations for various modern architectures, such as commodity PC clusters and the Blue Gene family.
http://www.itkp.uni-bonn.de/~urbach/software.html
GNU General Public License v3.0
32 stars 47 forks source link

config.h.in is versioned in git #364

Closed martin-ueding closed 7 years ago

martin-ueding commented 7 years ago

I just tried to merge the etmc/master into plabus/qphix_devel since they have diverged quite far already. There are a lot of merge conflicts in config.h.in which should not be versioned anyway. Can we remove that file from the repository?

When trying to recreate this file using autoreconf -fvi or just autoheader, I get the following on my system:

$ autoheader
autoheader: warning: missing template: ALIGN32
autoheader: Use AC_DEFINE([ALIGN32], [], [Description])

The ALIGN32 looks like it belongs to this project and is not something that is available in GNU Autotools by default. How can this be added such that the config.h.in can be properly generated by the user/developer?

kostrzewa commented 7 years ago

config.h.in is hand-written and should be versioned. If you can automate it, please feel free.

kostrzewa commented 7 years ago

I guess one would have to write a config.h.am (or .ac?), which lists the macros which will be defined by configure, I'm not sure though.

kostrzewa commented 7 years ago

Keep in mind that tmLQCD does not use the whole chain of autotools...

martin-ueding commented 7 years ago

Okay, that is fine. Then I propose to remove the first line in that file:

/* config.h.in.  Generated from configure.in by autoheader.  */
sunpho84 commented 7 years ago

Question is, why is not tmLQCD using the full set of GNU Autotools... ?

The config.h.in can be created by autoheader and should NOT be versioned!

The AC_DEFINE(ALIGN32) issue is fixed if you change one of the entry putting explicitly a description for it. For example like this:

AC_DEFINE(ALIGN32, [__attribute__ ((aligned (16)))], ["Ciao! Why do you read here?"])

You don't need to specify a description for all of them but at least for one. Note that all other defines have at least one description. Note also that [] does not count for a description, you need at least [""]

My guess is that it is expected that only one AC_DEFINE is used for a given MACRO. I think that an internal variable should be assigned in place of the various AC_DEFINE, and the variable should be assigned in a single AC_DEFINE at the bottom of the configure.in But this is a sophistication.

martin-ueding commented 7 years ago

I have made the change [] to [""] and now calling autoheader works just fine.

Looking at the Autotools Workflow graphic it seems that the file configure.in should be rather named configure.ac because it is the input file for autoconf? Either way, it seems to work.

Since running autoheader now works, I have let it run and looked at the change in config.in.h. The order of the #undef directives changes, so I have extracted them all and did some set substraction. With that I find the following.

#undef directives removed:

#undef directives added:

The two ones that have vanished seem to be used throughout the codebase, so they must be retained somehow. I presume that one just has do add two AC_DEFINE to the configure.in. If they are supposed to be undefined by default, they probably have to be put into some conditional in the configure.in. I am not sure how exactly to do this because I do not know what those options are supposed to do.

It seems that using autoheader is not really hard, just a few more lines to configure.in and we are done. I think that we should do it then, purge the config.in.h from the git repository in all branches and have fewer merge conflicts.

Just for reference, this is the script that I have used for extraction:

#!/usr/bin/env python3
# -*- coding: utf-8 -*-

# Copyright © 2017 Martin Ueding <dev@martin-ueding.de>
# Licensed under the MIT license.

import argparse
import re

def get_undefs(path):
    undefs = set()

    with open(path) as f:
        for line in f:
            m = re.search('#\s*undef\s+(\S+)', line)
            if m:
                undefs.add(m.group(1))

    return undefs

def print_set(s):
    for elem in sorted(s):
        print('- `{}`'.format(elem))

def main():
    options = _parse_args()

    undefs_old = get_undefs(options.old)
    undefs_new = get_undefs(options.new)

    print('`#undef` directives removed:\n')
    print_set(undefs_old - undefs_new)

    print('\n`#undef` directives added:\n')
    print_set(undefs_new - undefs_old)

def _parse_args():
    '''
    Parses the command line arguments.

    :return: Namespace with arguments.
    :rtype: Namespace
    '''
    parser = argparse.ArgumentParser(description='')
    parser.add_argument('old')
    parser.add_argument('new')
    options = parser.parse_args()

    return options

if __name__ == '__main__':
    main()
urbach commented 7 years ago

So, how would we benefit from autoheader? I don't see it...

Am 23. April 2017 16:06:26 MESZ schrieb Martin Ueding notifications@github.com:

I have made the change [] to [""] and now calling autoheader works just fine.

Looking at the Autotools Workflow graphic it seems that the file configure.in should be rather named configure.ac because it is the input file for autoconf? Either way, it seems to work.

Since running autoheader now works, I have let it run and looked at the change in config.in.h. The order of the #undef directives changes, so I have extracted them all and did some set substraction. With that I find the following.

#undef directives removed:

  • _KOJAK_INST
  • _NEW_GEOMETRY

#undef directives added:

  • F77_DUMMY_MAIN
  • FC_DUMMY_MAIN_EQ_F77
  • HAVE_ENDIAN_H
  • HAVE_FLOAT_H
  • HAVE_GETTIMEOFDAY
  • HAVE_INTTYPES_H
  • HAVE_LIBCUDART
  • HAVE_LIBDDALPHAAMG
  • HAVE_LIBINTL_H
  • HAVE_LIBRT
  • HAVE_LIMITS_H
  • HAVE_MALLOC
  • HAVE_MEMORY_H
  • HAVE_OMP_H
  • HAVE_POW
  • HAVE_SQRT
  • HAVE_STDLIB_H
  • HAVE_STRINGS_H
  • HAVE_STRING_H
  • HAVE_SYS_STAT_H
  • HAVE_SYS_TIME_H
  • PACKAGE_URL
  • RETSIGTYPE
  • SIZEOF_UNSIGNED_CHAR
  • SIZEOF_UNSIGNED_INT
  • SIZEOF_UNSIGNED_LONG
  • SIZEOF_UNSIGNED_LONG_LONG
  • SIZEOF_UNSIGNED_SHORT
  • STDC_HEADERS
  • TIME_WITH_SYS_TIME
  • _USE_BGLDRAM
  • malloc

The two ones that have vanished seem to be used throughout the codebase, so they must be retained somehow. I presume that one just has do add two AC_DEFINE to the configure.in. If they are supposed to be undefined by default, they probably have to be put into some conditional in the configure.in. I am not sure how exactly to do this because I do not know what those options are supposed to do.

It seems that using autoheader is not really hard, just a few more lines to configure.in and we are done. I think that we should do it then, purge the config.in.h from the git repository in all branches and have fewer merge conflicts.

Just for reference, this is the script that I have used for extraction:

#!/usr/bin/env python3
# -*- coding: utf-8 -*-

# Copyright © 2017 Martin Ueding <dev@martin-ueding.de>
# Licensed under the MIT license.

import argparse
import re

def get_undefs(path):
   undefs = set()

   with open(path) as f:
       for line in f:
           m = re.search('#\s*undef\s+(\S+)', line)
           if m:
               undefs.add(m.group(1))

   return undefs

def print_set(s):
   for elem in sorted(s):
       print('- `{}`'.format(elem))

def main():
   options = _parse_args()

   undefs_old = get_undefs(options.old)
   undefs_new = get_undefs(options.new)

   print('`#undef` directives removed:\n')
   print_set(undefs_old - undefs_new)

   print('\n`#undef` directives added:\n')
   print_set(undefs_new - undefs_old)

def _parse_args():
   '''
   Parses the command line arguments.

   :return: Namespace with arguments.
   :rtype: Namespace
   '''
   parser = argparse.ArgumentParser(description='')
   parser.add_argument('old')
   parser.add_argument('new')
   options = parser.parse_args()

   return options

if __name__ == '__main__':
   main()

-- Carsten Urbach, www.carsten-urbach.eu

sunpho84 commented 7 years ago

@martin-ueding

Looking at the Autotools Workflow graphic it seems that the file configure.in should be rather named configure.ac because it is the input file for autoconf

this is not strictly needed but would removes a warning, which I find always positive

@urbach

So, how would we benefit from autoheader? I don't see it...

If you download tmLQCD from github, it misses the "configure" file, so my first guess (and Martin's guess, I think) looking at the folder is that I have to type "autoreconf" to generate all the usual autotools chain, which will automatically call autoconf, autoheader and automake. Automake will be skipped because you have not initialized it in the configure.in, but the presence of the command AC_CONFIG_HEADER(config.h) will trigger the autoheader command to overwrite the config.h.in

So, either you advice people to launch autoconf only (for example adding a script with a customary name as bootstrap.sh), or you will be hampered by autoheader again in the future.

As a last comment in favour of autoheader, you don't have to maintain config.h.in by hands if you use it, which is one hassle less.

urbach commented 7 years ago

okay boys... The time this project was started, or at least the GWC code, the GNU autotool chain was not as standard as it is today. And that time, indeed, the template was called configure.in. configure,ac came later, or just around that time.

The file config.h.in is usually not touched and, hence, no conflicts are created or any problem. Of course, if you type autoheader (I see why someone might do it, even though I would not...), you run into trouble, I agree. But, in the end there are not so many people installing tmlqcd not knowing how it works.

My main reason, apart from automake being a pain, is that whenever I tried it there was trouble with not compatible versions of it. (actually I once spend a week or so and gave up again). From that point on I decided I do this only, if I really benefit a lot, otherwise I prefer to write a new solver or so instead during that time.

Okay, if someone really has the spare time to set up the complete autotool chain for tmlqcd, please go ahead. Alternatively also cmake. My only requirement is that it is at least as flexible as it is now. In particular in the sense that we are not blocked on new machines and architectures because the version installed is 3.2.4-13 and not 3.2.4-12.

Personally, I still don't see enough benefit, but that is not a must.

Of course _KOJAK_INST and _NEW_GEOMETRY must not be removed. Most (all?) of the others that Martin found are not needed, of course. But it also doesn't harm if they are there.

martin-ueding commented 7 years ago

It seems that in 72b975fe4df7cb23958d049353d43cae318b5414 the config.h.in has been changed significantly, it looks like autoheader was called. This makes the config.h.in in the qphix and qphix_devel branches very different from master. In 638d00c6f215aa7ff22bb4365f1a64a7b6bb4a97 I have tried to merge master into qphix_devel and using as much as possible from the master version.

I'm not really sure what has happened there, we then just have to be careful not to run autoheader and check in the changes.

sunpho84 commented 7 years ago

Okay, if someone really has the spare time to set up the complete autotool chain for tmlqcd, please go ahead. Alternatively also cmake. My only requirement is that it is at least as flexible as it is now. In particular in the sense that we are not blocked on new machines and architectures because the version installed is 3.2.4-13 and not 3.2.4-12.

I had actually prepared a "makefile.am" for tmLQCD a few months ago, to be able to link the tmLQCD library against Nissa (you and @kostrzewa might remember). In fact another benefit of Automake is that the installation is automatically adhering to the linux scheme (bin, include, lib), which is quite convenient if you want to link tmLQCD against an autotools or cmake project. (at the moment I am mainly using DDalphaAMG with Nissa, and I am bypassing tmLQCD to be able to set more freely the internal parameters, but I can still link easily against my modified version of tmLQCD)

For what concerns the portability, the important thing is not to version any file but Makefile.am and configure.ac (plus any custom m4 macro), and launch the full autotools chain after cloning the git repository. Otherwise the remnants of other autotools version might get in the way.

I attach the Makefile.am. I think the only missing part is enabling the GPU. The configure.ac must include

AM_INIT_AUTOMAKE([1.0 no-define foreign subdir-objects])

and two conditionals

AM_CONDITIONAL([USE_SPI],[test "$enable_spi" != "no" ],[true],[false])
AM_CONDITIONAL([USE_QUDA],[test "$QUDA_AVAILABLE" != "0" ],[true],[false])

Makefile.am.txt

martin-ueding commented 7 years ago

In fact another benefit of Automake is that the installation is automatically adhering to the linux scheme (bin, include, lib), which is quite convenient if you want to link tmLQCD against an autotools or cmake project.

That can also be done by tweaking the options to configure in a certain way. This is what I do in my .spec file for creating an RPM:

%configure \
        --prefix=%{buildroot}/usr \
        --exec-prefix=%{buildroot}/usr \
        --bindir=%{buildroot}/usr/bin \