render the local header systemwise

[jgmbenoit]

This patch renders the local header local.h systemwise.
Note that this header should be actually installed in an architecture dependent include hierarchy.

[codecov]

Codecov Report

Merging #235 (98f2766) into master (acad331) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #235   +/-   ##
=======================================
  Coverage   92.00%   92.00%           
=======================================
  Files         105      105           
  Lines        2075     2075           
=======================================
  Hits         1909     1909           
  Misses        166      166           

Impacted Files	Coverage Δ
libeantic/e-antic/renf_elem.h	100.00% <ø> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update acad331...98f2766. Read the comment docs.

[saraedum]

Thanks for the PR @jgmbenoit.

Note that this header should be actually installed in an architecture dependent include hierarchy.

Where would such headers go typically? I only know about includedir which is "The directory for installing C header files" according to the autoconf manual. Isn't that architecture dependent already typically?

[jgmbenoit]

On Debian multi-arch boxes (e.g., i386 (32bits) + amd64 (64bits)) the hierarchy /usr/include is shared, so it is architecture independent. Debian puts architecture dependent header under the hierarchy /usr/include/<triplet> as it puts libraries under /usr/lib/<triplet>. My current understanding is that we have to set it by hand when we use autotools. This page might be a good start. For Debian, I am applying a Debian specific patch so that the <triplet> can be passed at building time (at the time of writing, the git repository used by Debian is down, so I cannot provide a link to the patch)
.
Otherwise, local.h is a very generic name for an header: it would be a very good idea to rename it to avoid any name collision. Something as e-antic-local.h would make the trick.

[saraedum]

On Debian multi-arch boxes (e.g., i386 (32bits) + amd64 (64bits)) the hierarchy /usr/include is shared […]

I see. Thanks for the explanation.

Otherwise, local.h is a very generic name for an header: it would be a very good idea to rename it to avoid any name collision. Something as e-antic-local.h would make the trick.

Wouldn't it always be installed into an e-antic/ subdirectory? Even if it it's architecture dependent?

[saraedum]

This looks good to me. It would be great if we could add some comments (as suggested) on the changes since they are not obvious (at least not to me). Also, I think you need to add the search path in https://github.com/flatsurf/e-antic/blob/master/doc/manual/Makefile.am#L6 for the docbuild to work.

Finally, could you add an entry in doc/news? See instructions in https://raw.githubusercontent.com/flatsurf/e-antic/master/.github/pull_request_template.md

[saraedum]

Suggested change

	#include <e-antic/local.h>
	// Include architecture specific settings from e-antic/local.h and not just local.h since the header might be installed to separate architecture specific location on the search path.
	#include <e-antic/local.h>

Can we also add a disclaimer like that everywhere? Otherwise, we might just revert this in a few years when we have forgot why this was necessary I am afraid.

[jgmbenoit]

idem

[jgmbenoit]

Wouldn't it always be installed into an e-antic/ subdirectory? Even if it it's architecture dependent?

Indeed. Having said that, a "ceinture et bretelles" ("belt and braces") approach cannot hurt here.

[jgmbenoit]

is this patch applied in version 1.2.0 ?

[saraedum]

is this patch applied in version 1.2.0 ?

No. I think there were still some open discussions here that we were waiting for.