Fix includes

[mikhailnov]

Header files which are located in the same directory that the file from where it is included must be included using " ", not < >. Otherwise the compiler (gcc 5) cannot understand #include <tommath_class.h> in /usr/include/tommath/tommath.h.

[sjaeckel]

IMO this patch won't fix the problem you're describing.

After reading MoarVM/MoarVM#997 resp. your changes to ROSA I think it boils down to be a packaging problem in ROSA or a build-configuration problem in MoarVM!

[mikhailnov]

The build system of libtommath supports a custom include dir, the patch did fix the problem. But I don't know all details about how the compiler works with includes

[mikhailnov]

Why does tommath_class.h include itself?

[sjaeckel]

The build system of libtommath supports a custom include dir, the patch did fix the problem. But I don't know all details about how the compiler works with includes

I'm not 100% sure but I think I know how this patch fixed your problem...

1. you're having a system-installed libtommath
2. MoarVM builds after this patch against its own bundled headers which leads to:
3. you're basically building an incompatible MoarVM in cases where the header-files of the system-installed libtommath differ from the bundled one...

please correct me if I'm wrong!

Why does tommath_class.h include itself?

please have a look at the documentation in bn.pdf "Chapter 1.4 Build Configuration"

[mikhailnov]

No, moarvm does not have bundled libtommath, you may make sure by looking into src.rpm here: https://abf.io/build_lists/2952993

moarvm developers suggest to manually put libtommath source into 3rdparty/libtommath, but we did not put it there, and I made a small patch which switches includes: https://abf.io/perl6/moarvm/blob/rosa2016.1/ROSA-Use-system-headers.patch

Now I've rebuilt packages that depend from libtommath as a build dependency, e.g. https://abf.io/perl6/dropbear , they are built correctly with the patched libtommath.

[sjaeckel]

Apparently that now looks like the correct patch to me, but I'm still unsure about how the initial patch of this PR fixed your problem :-)

This issue can be closed, right?

[mikhailnov]

b0f31a9 fixed my problem, because on your system it is /usr/include/tommath*.h, bot on mine it is /usr/include/tommath/tommath*.h by setting this directory to the build script of libtommath. In my configuration the compiler could not understand #include <tommath*.h> from inside /usr/include/tommath/tommath*.h, the initial patch of this PR should make it work in both configurations.

This issue can be closed, right?

It's up to you to decide wether to merge this PR or not :-)

[sjaeckel]

Apparently that now looks like the correct patch to me, but I'm still unsure about how the initial patch of this PR fixed your problem :-)

Ah, I just had a look again and apparently the patch in https://abf.io/perl6/moarvm/blob/rosa2016.1/ROSA-Use-system-headers.patch still isn't correct IMO

The patch should be an updated e.g. libtommath.pc or something similar with the fixed location of the header files IIUC

@rofl0r can you probably have a look please? ... as I'm a bit confused now :D ... 1. if we should merge this or not? and 2. what the correct fix should be? thx :)

[rofl0r]

Header files which are located in the same directory that the file from where it is included must be included using " ", not < >.

that's correct, unless the path containing those headers is passed as an -I include dir during compilation, and that's almost certainly what libtommath's build system does by default. i suspect the build system used by OP does not do this.

apart from that there's nothing speaking against using "header"... so i'd recommend to test if everything still builds fine with this change and also test if it still works after it is installed (by compiling e.g. libtomcrypt against the installed heades), and if so this change can be merged.

[sjaeckel]

@karel-m can you please have a look if your builds are fine with these changes?

[mikhailnov]

libtomctrypt does build OK (https://abf.io/import/libtomcrypt/commit/e5fcc07a3c9de648fcb0f928bbdd0ff3d31fb7fb)

[mikhailnov]

Ah, I just had a look again and apparently the patch in https://abf.io/perl6/moarvm/blob/rosa2016.1/ROSA-Use-system-headers.patch still isn't correct IMO

-#include "tommath.h"
+#include <tommath/tommath.h>

This patch for moarvm is correct only for ROSA, where libtommath headers are located in /usr/include/libtommath, I did not test if

-#include "tommath.h"
+#include <tommath.h>

works, maybe yes.

[karel-m]

@karel-m can you please have a look if your builds are fine with these changes?

the changes in this PR work fine for me

[sjaeckel]

-#include "tommath.h"
+#include <tommath/tommath.h>

This patch for moarvm is correct only for ROSA, where libtommath headers are located in /usr/include/libtommath, I did not test if

-#include "tommath.h"
+#include <tommath.h>

works, maybe yes.

I still think that the correct patch would either be in rosa or even in moarvm, but if this patch helps you and since it doesn't hurt us it's rebased and merged

[mikhailnov]

Thank you.