Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing header file #100

Closed
1 task done
akhmerov opened this issue Dec 16, 2023 · 7 comments · Fixed by #102
Closed
1 task done

Missing header file #100

akhmerov opened this issue Dec 16, 2023 · 7 comments · Fixed by #102
Labels

Comments

@akhmerov
Copy link
Contributor

akhmerov commented Dec 16, 2023

Solution to issue cannot be found in the documentation.

  • I checked the documentation.

Issue

The latest version of mumps-include seems to miss a header file. Specifically, mumps_c_types.h from mumps-include has these lines:

/* mumps_int_def.h will define either MUMPS_INTSIZE32 (default)
   or MUMPS_INTSIZE64 (if compilation is with -DINTSIZE64 to
   match Fortran -i8 or equivalent option). This allows one to
   test from an external code whether MUMPS_INT is 64bits or not */
#include "mumps_int_def.h"

however mumps_int_def.h is missing from mumps-include.

Installed packages

mumps-include                         5.6.2         ha770c72_0               conda-forge
mumps-seq                             5.6.2         h5c37537_0               conda-forge

Environment info

-
@akhmerov akhmerov added the bug label Dec 16, 2023
@traversaro
Copy link
Contributor

Thanks for the report. I guess everything was fine for earlier version of mumps, right?

@akhmerov
Copy link
Contributor Author

Yes, I believe that this is due to a change introduced in 5.5.1, where MUMPS was switching integer formats. The corresponding segment of mumps_c_types.h from 5.2.1 (the version that was previously packaged via conda) is:

#include <stdint.h>
#ifdef INTSIZE64
#define MUMPS_INT int64_t
#else
#define MUMPS_INT int
#endif

and so it doesn't include the other file.

@traversaro traversaro mentioned this issue Dec 17, 2023
5 tasks
@traversaro
Copy link
Contributor

Ok, I investigate a bit:

  • On Windows, the issue is not a problem as the 64-bit header is used (see

    copy %src%\src\mumps_int_def64_h.in %src%\src\mumps_int_def.h
    )

  • On Linux/macOS, the 32-bit version is selected by the makefile:

2023-12-12T15:32:54.9625337Z if echo " -O " | grep DINTSIZE64; then cat mumps_int_def64_h.in > ../include/mumps_int_def.h; else cat mumps_int_def32_h.in > ../include/mumps_int_def.h; fi

However, this is not picked as mumps-include was built before the make script was ever execute.

So the possible action is to move mumps-include build after make as been run.

A non-directly related issue is that Windows uses 64 bit indeces, while Linux/macOS 32 bit. Was this intentional @minrk ?

@minrk
Copy link
Member

minrk commented Dec 18, 2023

Was this intentional @minrk ?

No, I updated the build scripts until they worked and figured defaults would apply. It probably makes sense to use 64b indices everywhere.

@dalcinl
Copy link
Contributor

dalcinl commented Dec 18, 2023

@minrk Wouldn't this blow in our faces as a huge ABI breakage? Maybe this package would need two build variants for 32 and 64 bit indices that can be explicitly selected via the build string?

@minrk
Copy link
Member

minrk commented Dec 19, 2023

If it had been out in use by any packages, it would be. But this change is only in a new version just published (which should perhaps be yanked, but I don't see a big reason to because this missing header means nobody can link against it), which presumably nobody can build against due to the missing header, which is the subject of this Issue.

Reading a bit more, I think we should stick with the 32b indices as the default, and the 64b choice for Windows was probably wrong.

@akhmerov
Copy link
Contributor Author

akhmerov commented Jan 16, 2024

The latest windows version win-64_mumps-seq-5.6.2-h1f49738_1 still misses the header file mumps_int_def.h, so I believe this issue was not completely fixed, please reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants