Opened 14 years ago

Closed 13 years ago

#217 closed defect (fixed)

make check make dist make distcheck

Reported by: Jari Häkkinen Owned by: Peter
Priority: major Milestone: yat 0.4
Component: build Version: trunk
Keywords: Cc:

Description (last modified by Peter)

Must work before release of 0.3. Moved to 0.4 due to unexpected complications.

Change History (24)

comment:1 Changed 14 years ago by Jari Häkkinen

Status: newassigned

comment:2 Changed 14 years ago by Peter

(In [832]) Refs #217 cleaning up makefile.am

comment:3 Changed 14 years ago by Peter

Remove or rename /usr/local/include/yat.

This does not solve anything but makes diagnostics more accurate.

Because the problem is the cross-directory includes, and when header files are not found locally they are found in /usr/local which may give a false impression of success.

comment:4 Changed 14 years ago by Peter

Component: testbuild

comment:5 Changed 14 years ago by Peter

By default are the following dirs included: . $(srcdir) and $(top_builddir)

However, our header includes presume that $(top_srcdir) is included. In most cases $(top_builddir) equals $(top_srcdir) and everything works fine. The problem is when this equality is not true. We need to add this include so also $(top_srcdir) is included.

I am not sure whether this should be done in configure.ac or in each Makefile.am. But somehow CPPFLAGS, CXXFLAGS or corresponding AM_ variable should be set. INCLUDES is another variable that could be used for this purpose, but this variable seems to be deprecated.

http://www.gnu.org/software/automake/manual/html_mono/automake.html#Program%20variables

comment:6 Changed 14 years ago by Peter

Ok here follows what needs to be done:

1) in test/Makefile.am

LDADD = @top_srcdir@/$(YAT_LIB_LOCATION)/$(YAT_LIB) \
 	    $(GSL_LIB) $(CBLAS_LIB) $(MATH_LIB)

should be

LDADD = @top_builddir@/$(YAT_LIB_LOCATION)/$(YAT_LIB) \
 	    $(GSL_LIB) $(CBLAS_LIB) $(MATH_LIB)

2) in test/Makefile.am

INCLUDES = -I@top_srcdir@

should be

AM_CPPFLAGS = -I@top_srcdir@

in yat/*/Makefile.am

add

AM_CPPFLAGS = -I@top_srcdir@

Note, since it is a flag for the pre-processor I use CPPFLAGS but leave CPPFLAGS free for the user and insead use corresponding flag in automake namespace: AM_CPPFLAGS

INCLUDES is deprecated as stated above and should not be used.

libyat is not created realive to top_scrdir but relative to top_builddir.

For an example what it should look like see svndigest project.

comment:7 Changed 14 years ago by Jari Häkkinen

Working through the problems with 'make distcheck' leads to the problem of running the test programs. These need test data and this data is not available from the builddir. From the Automake manual:

15.1 Simple Tests
If the variable TESTS is defined, its value is taken to be a list of programs or
scripts to run in order to do the testing. Programs needing data files should look
for them in srcdir (which is both an environment variable and a make variable) so
they work when building in a separate directory (see section “Build Directories ” in 
The Autoconf Manual), and in particular for the distcheck rule (see Chapter 14
[Dist], page 93).

This means that the test programs using test data from files have to be modified.

comment:8 Changed 14 years ago by Jari Häkkinen

(In [833]) Addresses #217. Fixed so that 'make distcheck' compiles but tests fail due to data file access.

comment:9 Changed 14 years ago by Jari Häkkinen

The test programs must be changed to accept a command line option defining where to find data files. The CommandLine? class from Peters sandbox should be included into the trunk to allow us to use it for the tests.

comment:10 Changed 14 years ago by Jari Häkkinen

Description: modified (diff)
Milestone: 0.3 (Public release)0.4

comment:11 Changed 14 years ago by Jari Häkkinen

Description: modified (diff)

comment:12 in reply to:  9 Changed 14 years ago by Peter

Description: modified (diff)

Replying to jari:

The test programs must be changed to accept a command line option defining where to find data files. The CommandLine? class from Peters sandbox should be included into the trunk to allow us to use it for the tests.

for CommandLine? see ticket:240

comment:13 in reply to:  7 Changed 13 years ago by Peter

Replying to jari:

Working through the problems with 'make distcheck' leads to the problem of running the test programs. These need test data and this data is not available from the builddir. From the Automake manual:

15.1 Simple Tests
If the variable TESTS is defined, its value is taken to be a list of programs or
scripts to run in order to do the testing. Programs needing data files should look
for them in srcdir (which is both an environment variable and a make variable) so
they work when building in a separate directory (see section “Build Directories ” in 
The Autoconf Manual), and in particular for the distcheck rule (see Chapter 14
[Dist], page 93).

This means that the test programs using test data from files have to be modified.

The fact that srcdir is an environment variable means there is no need for passing argument through commandline here. Instead replace

ifstream is("data/data.txt");

with

std::string srcdir = getenv("srcdir");
std::string filename = srcdir + std::string("/data/data.txt");
ifstream is(filename.c_str());

since this will be a very frequent code snippet, perhaps we should add it to a utility functions that all tests can share?

comment:14 Changed 13 years ago by Peter

One disadvantage of the approach above is that it will not be possible to run the test separately. The reason is that environment variable srcdir is set under target check in Makefile (or more specifically check-TESTS). Under target check srcdir is exported followed by a loop over all tests. Therefore, if you run the test outside make, i.e., by issue e.g. ./svm_test, it will fail because it will throw a std::runtime_error with what(): Environment variable srcdir is not set.

The simple solution would be to add a function in Suite something like this

string f(string filename)
{
  string res;
  try {
    res = getenv("srcdir");
  }
  catch (runtime_error)
  {
    res = ".";
  }
  return res + "/" + filename;
}

comment:15 Changed 13 years ago by Peter

Description: modified (diff)

comment:16 Changed 13 years ago by Peter

It would be nice if the filename function returns the absolute path, because then the scripts will work from other places than test/.

comment:17 in reply to:  14 Changed 13 years ago by Peter

Replying to peter:

One disadvantage of the approach above is that it will not be possible to run the test separately. The reason is that environment variable srcdir is set under target check in Makefile (or more specifically check-TESTS). Under target check srcdir is exported followed by a loop over all tests. Therefore, if you run the test outside make, i.e., by issue e.g. ./svm_test, it will fail because it will throw a std::runtime_error with what(): Environment variable srcdir is not set.

The simple solution would be to add a function in Suite something like this

string f(string filename)
{
  string res;
  try {
    res = getenv("srcdir");
  }
  catch (runtime_error)
  {
    res = ".";
  }
  return res + "/" + filename;
}

Just realized that the exception mentioned above was triggered by svndigest code. I issued svndigest::getenv rather than std::getenv. Yet we need to check for that case similar to how it is done in svndigest. See http://trac.thep.lu.se/svndigest/browser/trunk/lib/utility.cc

comment:18 Changed 13 years ago by Peter

(In [1240]) refs #217. Added function 'filename' that takes relative path and returns absolute path.

comment:19 in reply to:  18 Changed 13 years ago by Peter

Replying to peter:

(In [1240]) refs #217. Added function 'filename' that takes relative path and returns absolute path.

Should be ChangeSet? [1249]

comment:20 Changed 13 years ago by Peter

Owner: changed from Jari Häkkinen to Peter
Status: assignednew

comment:21 Changed 13 years ago by Peter

Status: newassigned

comment:22 Changed 13 years ago by Peter

Resolution: fixed
Status: assignedclosed

comment:23 Changed 13 years ago by Peter

Resolution: fixed
Status: closedreopened

Appeared some issues when having doxygen around.

ERROR: files left in build directory after distclean:
./yat/utility/doxygen.stamp

Similar things could appear when implementing tickets #219 #273 and #278, so better fix those before closing this ticket.

comment:24 Changed 13 years ago by Peter

Resolution: fixed
Status: reopenedclosed
Note: See TracTickets for help on using tickets.