spral icon indicating copy to clipboard operation
spral copied to clipboard

METIS 5 with 64bit integers only supported if --with-metis-inc-dir passed to configure

Open jfowkes opened this issue 2 years ago • 3 comments

It turns out that the current autotools build system only supports 64bit METIS 5 index types if --with-metis-inc-dir=/path/to/metis.h is passed to configure. This is not documented anywhere and clearly confusing for users.

As a result anyone who tries to build SPRAL with 64bit METIS without passing --with-metis-inc-dir=/path/to/metis.h to configure will almost certainly experience segfaults. We should update the documentation to support this use case.

jfowkes avatar Sep 18 '23 09:09 jfowkes

@mjacobse do you have any thoughts on a better way of doing this? Not necessarily in autotools, the plan is to transition over to a Meson build system (see #141) but the SPRAL_HAVE_METIS_H and SIZEOF_IDX_T defines are a pain point (see #135).

For reference support for 64bit METIS 5 index types was added in commit ff78163

jfowkes avatar Sep 21 '23 08:09 jfowkes

I think it would be reasonable to require a Metis header to be available and otherwise error out of the compilation. So basically get rid of SPRAL_HAVE_METIS_H and the default case if it is not defined. Then always check sizeof(idx_t) and define SIZEOF_IDX_T (or perhaps a better name like SPRAL_SIZEOF_METIS_IDX_T). That might require users to specify an extra path for the include (if not using default system paths), but that seems like better practice to me anyways. Just linking a C library and assuming declarations rather than getting them from a header is kind of asking for problems.

mjacobse avatar Sep 21 '23 09:09 mjacobse

Thanks @mjacobse I like your suggestion, it's probably not worth the effort of changing it for the autotools build system but we should definitely be able to do this with Meson.

jfowkes avatar Sep 21 '23 10:09 jfowkes