this shouldn’t be necessary and same for r66404, gmath.h contains already grass/la.h. Any possible compilation issue is an issue with configuration (local or in GRASS) or with the gmath.h file. What was the reason for the change?
2015-10-05 3:04 GMT+02:00 Vaclav Petras <wenzeslaus@gmail.com>:
this shouldn't be necessary and same for r66404, gmath.h contains already
grass/la.h. Any possible compilation issue is an issue with configuration
(local or in GRASS) or with the gmath.h file. What was the reason for the
change?
compilation was simply failing on build server (Debian stable):
In file included from main.c:47:0:
global.h:28:29: error: unknown type name ‘vec_struct’
global.h:28:43: error: unknown type name ‘vec_struct’
global.h:31:8: error: unknown type name ‘mat_struct’
2015-10-05 9:41 GMT+02:00 Martin Landa <landa.martin@gmail.com>:
compilation was simply failing on build server (Debian stable):
In file included from main.c:47:0:
global.h:28:29: error: unknown type name ‘vec_struct’
global.h:28:43: error: unknown type name ‘vec_struct’
global.h:31:8: error: unknown type name ‘mat_struct’
...
even GRASS is compiled with lapack and blas support. I checked config.h again:
/* define if LAPACK exists */ #define HAVE_LIBLAPACK 1
/* define if BLAS exists */ #define HAVE_LIBBLAS 1
compilation was simply failing on build server (Debian stable):
It is working on Ubuntu and probably Fedora as well. Perhaps it is a local issue.
In file included from main.c:47:0:
global.h:28:29: error: unknown type name ‘vec_struct’
global.h:28:43: error: unknown type name ‘vec_struct’
global.h:31:8: error: unknown type name ‘mat_struct’
…
even GRASS is compiled with lapack and blas support. I checked config.h again:
/* define if LAPACK exists */ #define HAVE_LIBLAPACK 1
/* define if BLAS exists */ #define HAVE_LIBBLAS 1
Then la.h should be included in gmath.h. Including it in modules in a workaround, not the solution. There are three possible solutions:
remove la.h from gmath.h and declare that modules must include it (I don’t see a reason for this)
change la.h include in gmath.h to something else; perhaps #if defined(…) && defined(…) is wrong (but I don’t think so, seems to fit with the standard)
find out why it is different on that server and think about the proper fix afterwards (ideal)
I can’t test it since I can’t reproduce it.
Sorry for being -pedantic but I don’t think we should leave the code messy.
On Mon, Oct 5, 2015 at 3:44 AM, Martin Landa <landa.martin@gmail.com
<mailto:landa.martin@gmail.com>> wrote:
>
> Hi,
>
> 2015-10-05 9:41 GMT+02:00 Martin Landa <landa.martin@gmail.com
<mailto:landa.martin@gmail.com>>:
> > compilation was simply failing on build server (Debian stable):
It is working on Ubuntu and probably Fedora as well. Perhaps it is a
local issue.
> >
> >> In file included from main.c:47:0:
> >> global.h:28:29: error: unknown type name ‘vec_struct’
> >> global.h:28:43: error: unknown type name ‘vec_struct’
> >> global.h:31:8: error: unknown type name ‘mat_struct’
> > ...
>
> even GRASS is compiled with lapack and blas support. I checked
config.h again:
>
> /* define if LAPACK exists */
> #define HAVE_LIBLAPACK 1
>
> /* define if BLAS exists */
> #define HAVE_LIBBLAS 1
Then la.h should be included in gmath.h. Including it in modules in a
workaround, not the solution. There are three possible solutions:
1) remove la.h from gmath.h and declare that modules must include it (I
don't see a reason for this)
2) change la.h include in gmath.h to something else; perhaps #if
defined(...) && defined(...) is wrong (but I don't think so, seems to
fit with the standard)
3) find out why it is different on that server and think about the
proper fix afterwards (ideal)
I can't test it since I can't reproduce it.
Sorry for being -pedantic but I don't think we should leave the code messy.
Sorry for being -pedantic but I don’t think we should leave the code
messy.
+1
Then it needs to be removed everywhere here:
What we need however, is to figure out why it is not working on that Debian server. make distclean? svn revert? Dockerfile for Debian would be helpful in this case to have reproducible environment.
I would remove it only in 7. It should work and it worked without it (except for that Debian server). I don’t know what is the situation in 6. Let’s not be bothered with code correctness for 6, the only priority there is that the main code works and addons are ported to 7.
Sorry for joining in late, but from my understanding of the code, it
seems that the inclusion of <grass/la.h> should be in the modules, not
in <grass/gmath.h>.
<grass/gmath.h> defines a number of macros plus the G_math_spvector
type. It then includes <grass/defs/gmath.h> which contains a number of
function declarations, some of which use G_math_spvector in their
return or parameter types.
<grass/gmath.h> also includes <grass/la.h>, but doesn't depend upon
anything defined in that header. If the include of <grass/la.h> is
removed, compiling a source file containing only
#include <grass/gmath.h>
succeeds. Consequently, there is no need for <grass/gmath.h> to
include <grass/la.h>.
Having one header for "base" functionality which is always available
and another for additional functionality which is only available when
GRASS was built with BLAS/LAPACK support seems cleaner than having a
single header expose both, particularly when the two have already been
separated.
Additionally, requiring individual modules to include <grass/la.h>
explicitly would make it clear when code depends upon BLAS/LAPACK.
On Tue, Oct 6, 2015 at 12:26 PM, Glynn Clements <glynn@gclements.plus.com>
wrote:
Markus Neteler wrote:
> >> Sorry for being -pedantic but I don't think we should leave the code
> >> messy.
> >
> > +1
>
> Then it needs to be removed everywhere here:
>
> cd grass-addons
> find . -type f | grep -v pristin | xargs grep 'la.h>'
> ./grass6/imagery/i.spec.unmix/global.h:#include <grass/la.h>
> ./grass6/imagery/i.spec.unmix/la_extra.c:#include <grass/la.h>
> ./grass6/imagery/i.spec.grass63/include/gmath.h:#include <grass/la.h>
> ./grass6/imagery/i.spec.grass63/lib/gmath/la.c:#include <grass/la.h>
> ./grass7/imagery/i.spec.unmix/global.h:#include <grass/la.h>
> ./grass7/vector/v.kriging/local_proto.h:#include <grass/la.h>
Sorry for joining in late, but from my understanding of the code, it
seems that the inclusion of <grass/la.h> should be in the modules, not
in <grass/gmath.h>.
<grass/gmath.h> defines a number of macros plus the G_math_spvector
type. It then includes <grass/defs/gmath.h> which contains a number of
function declarations, some of which use G_math_spvector in their
return or parameter types.
<grass/gmath.h> also includes <grass/la.h>, but doesn't depend upon
anything defined in that header. If the include of <grass/la.h> is
removed, compiling a source file containing only
#include <grass/gmath.h>
succeeds. Consequently, there is no need for <grass/gmath.h> to
include <grass/la.h>.
Having one header for "base" functionality which is always available
and another for additional functionality which is only available when
GRASS was built with BLAS/LAPACK support seems cleaner than having a
single header expose both, particularly when the two have already been
separated.
Additionally, requiring individual modules to include <grass/la.h>
explicitly would make it clear when code depends upon BLAS/LAPACK.
I agree.
I was not able to determine from svn why gmath.h contains la.h.
I was not able to determine from svn why gmath.h contains la.h.
AFAICT, it's because the functions are in lib/gmath. Of course, that
then brings up the question why they're there, rather than in a
separate library ...
Incidentally, the whole of lib/gmath/la.c is conditionalised upon
but individual functions (and even sections of functions) are
conditionalised as necessary.
And there are a few functions for which we could reasonably supply our
own implementation if BLAS/LAPACK aren't available. E.g.
G_matrix_product() and G_vector_norm_euclid() should be
straightforward.
AFAICT, G_matrix_LU_solve() is the only function that's moderately
complex.
I was not able to determine from svn why gmath.h contains la.h.
AFAICT, it’s because the functions are in lib/gmath. Of course, that
then brings up the question why they’re there, rather than in a
separate library …
Incidentally, the whole of lib/gmath/la.c is conditionalised upon
but individual functions (and even sections of functions) are
conditionalised as necessary.
I have seen this code before and I was not courageous enough to clean it up.
And there are a few functions for which we could reasonably supply our
own implementation if BLAS/LAPACK aren’t available. E.g.
G_matrix_product() and G_vector_norm_euclid() should be
straightforward.
Perhaps this is the idea of having the functionality in gmath itself is behind having it part of gmath. Then the question is if we actually want this or we want to move it. If we will be able to provide our implementations, it doesn’t matter where the functions are.
I like the idea of a separate header but I would leave the implementation in lib/gmath. Or is this going too much against how other libraries are done? I’m also not sure how the prefixes should work, but these seems OK since there are functions with same prefix (G_) in several libraries.
Having la.h as a separate header would conveniently hide the compilation issue on that Debian server.
So, should I remove la.h from gmath.h?
AFAICT, G_matrix_LU_solve() is the only function that’s moderately
complex.
I like the idea of a separate header but I would leave the implementation
in lib/gmath. Or is this going too much against how other libraries are
done? I'm also not sure how the prefixes should work, but these seems OK
since there are functions with same prefix (G_) in several libraries.
Having separate headers for self-contained sections of a library isn't
an issue.
Ideally, if a localised section of a library has dependencies not
required for the rest of the library, that section is a candidate for
being a separate library (so that a lack of the dependency only
disables the features which require it, rather than the entire
library).
But this isn't rigorously followed (e.g. libgis has compile-time
dependencies on iconv, pthread, regex, and zlib, even though each of
those libraries is only used for isolated features).
Having la.h as a separate header would conveniently hide the compilation
issue on that Debian server.
Having la.h as a separate header would conveniently hide the compilation
issue on that Debian server.
So, should I remove la.h from gmath.h?
I think so.
la.h removed from gmath.h in r66485. I tested that with and without LAPACK configured. Work as expected. Perhaps the #error can be removed from the modules if it makes sense to have it in la.h (as I did that).
On Wed, Oct 14, 2015 at 5:16 AM, Vaclav Petras <wenzeslaus@gmail.com> wrote:
la.h removed from gmath.h in r66485. I tested that with and without LAPACK
configured. Work as expected. Perhaps the #error can be removed from the
modules if it makes sense to have it in la.h (as I did that).
I suppose yes, but only if gmath.h gets also updated in relbranch70 to
avoid user confusion with addons.