Differentiate gap vs standard lib headers

[ptrrsn]

The header io.h from MinGW is hidden by gap's io.h which is causing compilation error. Changing -I to -iquote ensures that all gap header files are included with quotes, while the standard library header files are included with the <>.

This helps the work for #4157

[dimpase]

OK, that's a good catch.

[dimpase]

This is certainly the correct change, although one should check that this doesn't break building GAP packages (e.g. io) - they might need similar fixes, in fact.

[ptrrsn]

Thank you for the approval! I have manually checked make libgap and make in Linux, both still work. Is there anything else that I need to check?

[fingolfin]

First off: thank you for your contribution, it would of course be great if someone finally worked a bit on improving "native" (well, MinGW based) Windows support.

That said, I am torn about this PR because GAP is not only used and compiled "directly from source", but also in installed form, after make install, and as shipped by Linux distros. This includes installing the GAP header files so that GAP packages can build kernel extensions.

With your proposed patch, everyone everywhere will have to use -iquote (or the equivalent in whatever compiler they use)...

Mind you, I am not saying "no", but I need a bit more time to ponder it (and perhaps @ChrisJefferson would like to chime in, too). Alternatives I can think of right now include:

• we rename our io.h (but of course then the question is if more similar errors pop up)
• long term: let's change all our packages with kernel extension which currently use

  #include "compiled.h"   // GAP headers
  

  to instead use

  #include "gap_all.h"
  

  or

  #include <gap_all.h>
  

  and then we can arrange it so that in both variants of GAP ("out of source tree" and "installed system wide") there is a directory which contains only a gap_all.h file and which includes everything it should in a way that requires no further -I statements (roughly speaking, it should suffice if it consists of a single line #include "../path/to/real/gap_all.h")

(Actually I guess my second point could already be done now, if we just place both gap_all.h and common.h into that directory. "That" directory for system wide install would be PREFIX/include, and it would contain #include "gap/gap_all.h")

[ChrisJefferson]

Yes, I would prefer not to adjust the build system, just because adding extra complication to all the places GAP is built (in various distros, and many packages have slightly different build systems for historical reasons).

I would be happy to rename the file (git tends to cope with that fairly well). It would be good to know if anything else collided, so we could do the renaming in one go.

[dimpase]

IMHO, it's better to put all the GAP headers into a subdirectory, e.g. libgap/. Then the header to include would be something like libgap/gap.h, not something with all in the name.

The old code can either do a straightforward renaming, or add -I libgap/ to the CFLAGS etc.

It's certainly not the first time such changes become inevitable in various projects - GAP held out for so long just due to the lack of make install.

[ptrrsn]

Thank you all for the comments. I will be happy to make some improvements to unblock the native Windows build. For this PR, I can do the renaming, and I like @dimpase's idea to move all headers under a subdirectory to effectively namespace the header files and solve the conflicting names once and for all.

@fingolfin, forgive my very limited understanding, but were you suggesting hiding all the headers and exposing only gap_all.h instead? I am not familiar with the concept of kernel extensions. My understanding is that kernel extensions outside this repository use these headers too, and your suggestion involved a slow migration process, i.e., by maintaining backward compatibility for those external kernel extensions by still providing the rest of the .h headers for a specific period. Am I understanding your suggestion correctly? Since this sounds like something that can be done in parallel with the renaming, I will proceed with the subdirectory idea for now, and we can open a separate issue to discuss this long-term migration further if it is still needed, as I can see @dimpase has a different opinion on it (CMIIW). What do you think?

[dimpase]

historically, GAP kernel modules were always built in-tree, that's why very little had been done as far as header namespaces go.

Many packages had to move to headers in subdirectories: to name a few in the area of algebra: gmp, flint/flintlib, cdd/cddlib.

As far as distributions go, they would very much appreciate neat header handling, as dumping everything in /usr/include or in /usr/local/include isn't working at scale.

[fingolfin]

@dimpase all GAP headers are already in a subdirectory PREFIX/gap/ when installed, as is common for libraries. If you are suggesting to put all headers into a subdirectory in the source tree: that would be cumbersome for various reasons, and also simply not necessary. It may surprise you, but I actually thought about all these things years ago when I set out to get GAP ready to support make install. It's one of the various small things here and there that I simply did not yet get around. Luckily pretty much straightforward these days.

Your suggestion to adding -I libgap/ misses the entire point, that would reintroduce the problem with io.h.

@ptrrsn no, that's not my suggestion. Or rather: the GAP headers are already "hidden away" when GAP is installed, namely in PREFIX/gap/. However, this does not contribute to solving the io.h problem because at the same time the CPPFLAGS include -IPREFIX/gap.

What I am proposing is this instead:

1. we add a new file src/extra/gap_all.h to the repo which would basically consist of #include "../gap_all.h" plus a header comment
   • this makes use of the fact that the preprocessor first searches in the directory in which the current file is
   • CPPFLAGS then would only contain -Ipath/to/gap.git/src/extra
2. for make install we install this header like all others, i.e. asPREFIX/gap/extra/gap_all.h
   • CPPFLAGS then would only contain -IPREFIX/gap/extra
3. Packages should be changed to do #include "gap_all.h"
   • this was the plan all along, but I held off from doing it because some package authors don't like requiring "the latest" GAP
   • but this headers has been there there since GAP 4.12 from 2022, and requiring GAP >= 4.12 should be fine for all packages with kernel extensions (in fact I think we already did that last year, when we changed them to use IsKernelExtensionAvailable and LoadKernelExtension
   • I will get right on that in fact, and make releases of most if not all those packages

The main drawback is that it'll break packages with kernel extensions not in the GAP distro right now. But we can solve this for many of them by also adding a src/extra/compiled.h header which has identical content to gap_all.h.

[dimpase]

If you are suggesting to put all headers into a subdirectory in the source tree: that would be cumbersome for various reasons, and also simply not necessary

I was talking about kernel modules, and got carried away a bit, sorry for confusion. As far as the GAP build itself, moving all src/*.h to e.g. include/gap/ (with adding gap/ prefix throughout) would clean things a bit IMHO, and allow for more flexibility in e.g. a Windows build.

Also, IMHO, the de facto standard is that the internal headers be #included with "", not with <>. (although the C standard is totally silent on this, it's all "implementation-dependent").

I don't see this, or the solution offered by this PR, as being problematic for various downstreams. Distros (including SageMath) will adapt easily, GAP package builders ought to use the installed GAP headers, anyway. But let's ask @orlitzky - who maintains GAP in Gentoo, what he would prefer.

Your suggestion to adding -I libgap/ misses the entire point, that would reintroduce the problem with io.h.

No, that was meant for downstream users of GAP, not for the GAP itself. But, sorry, my memory was fuzzy on the state of make install in GAP today.

[fingolfin]

@dimpase err you are apparently ignorant of what GAP currently does despite me explicitly stating it: we already install headers in a separate dir and our make install alwaya did that -- making sure this works cleanly was one of the major road blocks for it, which a bunch of "helpful" people making comments of the nature "this is easy, why don't you simply... " conveniently ignored -- which why this is a bit upsetting for me right now, being lectured by you.

Overall your comments really don't contribute positively to the discussion. I'd appreciate if you just kept out if it, we have it under control.

[dimpase]

@fingolfin I only ventured into it cause @ptrrsn happens to be my former student. But I don't mean to lecture anyone, I'm as usual totally tone-deaf, apologies..

By the way, what upsets me about GAP is that I have a PR ready in gap-packages/ace for literally years, and you just seem to ignore it, despite reminders.

[ptrrsn]

Thank you everyone for the comments.

@fingolfin I updated my changes with my interpretation of your proposal. Could you check if my understanding is correct?

The way I tested it by:

1. running make and make install in Linux,
2. ./configure and make for tst/mockpkg,
3. write a C source code that include extra/gap_all.h and checked manually that the extra headers are installed in my Linux filesystem after make install,
4. The "io.h" problem in MinGW is resolved (by the iquote).

But looking at the CI breakage, it seems that those tests are not enough...

[ptrrsn]

Ah, okay, the failure is due to io package build failure. And it can be fixed by changing the use of #include "profile.h" in io package. I will make that change in the io package.

[ptrrsn]

To accomodate this, I created two PRs on two packages: https://github.com/gap-packages/datastructures/pull/156 https://github.com/gap-packages/io/pull/128

[ptrrsn]

Ah, sorry, I accidentally triggered a CI run from a sync. I will try to fix the failures, except the "CI / testbuildsys; testmockpkg testinstall - NO_COVERAGE = 1 ..." seems to be an infrastructural failure. I also saw that in my other PR.

[ptrrsn]

@fingolfin I think I have fixed all the CI problems (except the infrastructure failure one). As for HPC, I added all src instead of just src/extra in SYSINFO_CPPFLAGS. Without that special case, headers like hpc/tls.h, which has #include "system.h", will not work because system.h is not in the same directory as hpc/tls.h and it is not in the SYSINFO_CPPFLAGS.

An alternative but more intrusive option is to change all such includes in hpc directories to be in the form of #include "../system.h". I personally think this would be better than having a special case in Makefile.rules because it avoids different expectations between hpc vs non-hpc. If this one is preferred, I can make this change in a separate PR so as not to clutter the current PR further. What do you think?

[ptrrsn]

Sorry, I resolved them on my local branch and was preparing to push, but I found some errors. I will push first before replying to/resolving your comments next time to avoid such confusion.

I have addressed your comments in this last update.

[ptrrsn]

The PR is ready for review. Sorry for the confusion!

[orlitzky]

I'm at a conference and don't have access to my usual array of weird machines, but I am skeptical that -iquote is portable. It looks like clang does support it, even though it's not documented. In any case, POSIX only has -I: https://pubs.opengroup.org/onlinepubs/9799919799/utilities/c17.html

[ptrrsn]

I'm at a conference and don't have access to my usual array of weird machines, but I am skeptical that -iquote is portable. It looks like clang does support it, even though it's not documented. In any case, POSIX only has -I: https://pubs.opengroup.org/onlinepubs/9799919799/utilities/c17.html

Both clang (https://clang.llvm.org/docs/ClangCommandLineReference.html) and gcc (https://gcc.gnu.org/onlinedocs/gcc/Directory-Options.html) seem to support iquote and they are both documented. If those are insufficient, I can try to remove the iquote use here.

[ptrrsn]

Thank you for the reviews! I have updated the PR and addressed all the comments.

[fingolfin]

So... please don't hate me for asking this, considering how much effort you have put into this PR by now, but, just thinking about this again now, remind me: why do we (resp. you, thank you!) go to all this effort and ever growing list of complications, instead of simply renaming io.h to gap_io.h or inout.h or input_output.h or whatever... ?

[ptrrsn]

I am trying to resolve the problem cleanly by implementing your long-term suggestion, which, in my understanding, is preferred for GAP. If possible, I would prefer not to introduce technical debt by renaming "io.h" as a short-term fix. The long-term suggestion that I meant can be found in your comment here: https://github.com/gap-system/gap/pull/6007#issuecomment-2987070411, which also seems to provide a better boundary between GAP and its packages (so not just fixing the "io.h" conflicts problem).

However, I can also revert this and perform a simple renaming instead if that's what you prefer. Please let me know your preference :-)

[fingolfin]

I am honestly on the fence. Complications in the build system also are a form of technical debt.

I think the idea of adding the extra subdir and letting packages by default only use that, is a good one.

But a lot of the other hoops you had to jump through all revolve around letting GAP source files outside of src (such as ffdata.c, a modified main.c) access the header files and this requires more and more contortions.

Like, it is OK to inline debug.h into common.h but we really only did it as a workaround.

Then again, overall the PR is not too bad as it is right now. I am inclined to just merge it. But I'd love to hear what e.g. @ChrisJefferson thinks

[fingolfin]

(Oh, I just realize that "the PR is not too bad" might sound a bit insulting -- if so, I sincerely apologize, that's not what I intended -- the work put into this PR was/is excellent and you did everything as requested, that was done beautifully. If there are any reservations on my side, that's entirely my own fault for not being able to make up my mind. Sorry for that, too.

[ptrrsn]

No worries, I will follow whatever you decide, as you are the one who has deep insight into what will be best for the repository :-)

But I also agree that the current change makes the build system more complicated. How about we do something in the middle:

• We expose only extra directory on SYSINFO_CPPFLAGS (i.e., the last state of the PR)
• For GAP_CPPFLAGS, we rename io.h to input_output.h and keep the original -I$(srcdir)/src instead of -I$(srcdir)/src/extra -I$(srcdir)/src/system.

That way, we still restrict packages to only be able to include the extra directory while maintaining the simplicity for the internal GAP code (then, we can revert all the complications on build directory: the inlining debug.h, etc) + solving the "io.h" problem. This may be a better option, what do you think? @fingolfin

[ChrisJefferson]

I realise this has ended up being fairly epic, but I quite like where it is now.

I like being consistent with how we include GAP headers (I never fully realised we had some relative, some absolute, because it all compiled before). I also tried doing an 'emscripten' build of GAP (this isn't officially supported, because it currently isn't remotely polished, but it does do some unusual build system things), and it worked.

We could keep tweaking forever, but personally I think this is a general improvement.

[fingolfin]

So @ChrisJefferson kinda convinced me, but then I also had another think, and realized we might be able to simplify a few things again...

So I tweaked this PR a bit to avoid the need for build/common.h and src/system/system_all.h, which then allowed me to restored src/debug.h. Please have a look at the commits I added.

I'd be happy to merge this now if you are also OK with it, @ptrrsn

[ptrrsn]

Wow, thank you for the additional patches! They look really great!