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

Add etc/Makefile.gappkg, for use in the build system of GAP package with kernel extensions #3683

Merged
merged 1 commit into from
Oct 18, 2019

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Oct 4, 2019

Summary

The file etc/Makefile.gappkg provides the core of a build system for GAP packages with a kernel extension, with nice features, such as automatic recompilation if any source files or parts of the build system changed, or if GAP itself changed.

It is already use by several GAP packages. By shipping it with GAP, it gets easier for new GAP packages to provide simple kernel extensions.

Goals and questions

There are two ways this could be used:

  1. We simply bundle it with GAP, as a reference implementation, so that package authors have a definitive place to look for the "newest" version of this file.
  2. Or, we could in addition strongly encourage package authors to use the copy of this file bundled with GAP, not with their package. If they want to be compatible with GAP < 4.11, they can of course still bundle a copy of Makefile.gappkg in their package, but should have a line like this in their configure script:
test -r $GAPPATH/etc/Makefile.gappkg && cp $GAPPATH/etc/Makefile.gappkg .

The two approaches balance work between the GAP team, and package maintainers:

Approach 1 is more defensive; it means that in order to benefit from improvements to this file, package authors will have to copy the file over to their package from time to time (but carefully so, as newer versions might not be compatible with older versions of GAP; right now, this file is compatible with at least GAP 4.9, 4.10 and 4.11, but I already have an improvement in mind that would require a new gac version and thus would not be backwards compatible). After copying the file, they also need to make a release. Based on past experience, this may never happen, or only with a delay of several years, unless somebody from the GAP team actively pushes all involved packages resp. their maintainers to action, which means a lot of work.

Approach 2 requires virtually no effort from package maintainers. It does, however, slightly increase the strain on the GAP team itself, as we'd have to be careful with any change so as not to break existing packages relaying on Makefile.gappkg. However, we already have CI tests which build and test all deposited packages; so we should notice regressions quickly. In the worst case, a change may need to be coordinated with updates to some packages, but that is still less work than with approach 1.

Hence, overall I prefer approach 2, but would be interested to learn what others think.

Other considerations

The name Makefile.gappkg is not set in stone. Alternative suggestions are welcome.

I marked this as backport-to-4.11, and referenced 4.11 in the code comments, because (a) there is no risk of regression when adding a completely new file like this, and (b) the sooner this is in a GAP release, the sooner people can use it, and the sooner they can drop backwards compatibility code (I'd still expect that to take a couple years, but reducing from N to N-1 years still is good ;-).

Some packages need to build other things than just a GAP kernel extension. I tried to write Makefile.gappkg with this in mind; hence most variables are prefixed with KEXT_ to avoid clashes, build rules are set up to be extensible, and so on). However, this may require some further tweaking based on experience with integrating this into packages with more complex build requirements.

Ideally I'd like to use this in the Example package, so that people can see how to use it there; and of course also in PackageMaker.

Affected packages

Packages that use (a version of) this file, resp. for which I submitted a PR to use a copy of it, include these:

A few other packages either use older versions of the build rules in here, or could otherwise easily be adapted to use them, if so desired, including:

  • Browse
  • EDIM
  • GaloisGroups
  • Gauss
  • (Example) (doesn't currently build a kernel extension, but has a TODO for it)
  • (PackageMaker: could use it in its default kernel extension template)
  • profiling

Packages that could perhaps use it (I didn't look closely), but might still require more complicated configure scripts (autoconf based?) to check for required 3rd party dependencies include:

  • BlissInterface
  • Digraphs
  • NautyTracesInterface
  • NormalizInterface
  • PythonInterface
  • Semigroups
  • ZeroMQInterface
  • curlInterface
  • ferret
  • float
  • io
  • meataxe64

@fingolfin fingolfin added do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) topic: build system topic: packages issues or PRs related to package handling, or specific to a package (for packages w/o issue tracker) kind: discussion discussions, questions, requests for comments, and so on backport-to-4.11 labels Oct 4, 2019
# build rule for C++ code
# The dependency on Makefile ensures that re-running configure recompiles everything
gen/%.lo: %.cc Makefile
$(QUIET_GAC)$(GAC) -d -p "$(KEXT_DEPFLAGS)" -p "$(KEXT_CXXFLAGS)" -c $< -o $@
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this perhaps also support KEXT_CPPFLAGS, which would be passed to both C and C++ compilers?

Should we also support other C++ file extensions, such as .cxx, .cpp? The gac tool supports all three, although .cxx only starting with GAP 4.11.

fingolfin added a commit to fingolfin/profiling that referenced this pull request Oct 4, 2019
On Travis, test against GAP 4.9, too (this package claims to be compatible
with it).

As for the build system: stop using autotools, use Makefile.gappkg instead
(see <gap-system/gap#3683>); this has several
advantages, e.g. running configure is much quicker (esp. on macOS and
Windows), it automatically rebuilds if GAP changed (detected via changes to
GAP's sysinfo.gap), and of course there is no need for autoconf, automake,
autogen.sh etc., nor need to deal with M4 syntax in cryptic configure.ac
scripts
@coveralls
Copy link

coveralls commented Oct 4, 2019

Coverage Status

Coverage remained the same at 84.476% when pulling ce601d0 on fingolfin:mh/pkg-buildsys into 2c596b3 on gap-system:master.

@ChrisJefferson
Copy link
Contributor

I think we should go with option (2) and plan not to break it. If something really serious happened and we decided we had to make breaking changes, we could also introduce Makefile-v2.gappkg or something crazy like that.

@fingolfin
Copy link
Member Author

Yeah, indeed, of course the plan is to avoid breaking stuff as much as possible ;-). To this end, before merging this PR, I'd also try hard to get as many packages as possible working with a version of it. I already submitted a bunch of PRs resp. updated repos. Two more tricky ones are gap-packages/PARIInterface#24 and gap-packages/GaloisGroups#9 which already lead me to make several major changes to the code in here, to make it easier to combine it with other build steps.

@fingolfin fingolfin removed the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Oct 14, 2019
@fingolfin fingolfin changed the title Add parts for GAP package build systems (DO NOT MERGE; request for comments) Add parts for GAP package build systems Oct 14, 2019
The file Makefile.gappkg is already use by several GAP packages. By
shipping it with GAP, it gets easier for new GAP packages to provide
simple kernel extensions.
@fingolfin fingolfin merged commit 015dd2d into gap-system:master Oct 18, 2019
@fingolfin fingolfin deleted the mh/pkg-buildsys branch October 18, 2019 13:33
@fingolfin
Copy link
Member Author

Backported to stable-4.11 in fb0f52d

fingolfin added a commit to fingolfin/profiling that referenced this pull request Oct 27, 2019
Stop using autotools, and instead start to use Makefile.gappkg (see
<gap-system/gap#3683>); this has several advantages,
e.g. running configure is much quicker (esp. on macOS and Windows), it
automatically rebuilds if GAP changed (detected via changes to GAP's
sysinfo.gap), and of course there is no need for autoconf, automake,
autogen.sh etc., nor need to deal with M4 syntax in cryptic configure.ac
scripts
fingolfin added a commit to fingolfin/profiling that referenced this pull request Oct 27, 2019
Stop using autotools, and instead start to use Makefile.gappkg (see
<gap-system/gap#3683>); this has several advantages,
e.g. running configure is much quicker (esp. on macOS and Windows), it
automatically rebuilds if GAP changed (detected via changes to GAP's
sysinfo.gap), and of course there is no need for autoconf, automake,
autogen.sh etc., nor need to deal with M4 syntax in cryptic configure.ac
scripts
fingolfin added a commit to fingolfin/json that referenced this pull request Oct 27, 2019
Stop using autotools, and instead start to use Makefile.gappkg (see
<gap-system/gap#3683>); this has several advantages,
e.g. running configure is much quicker (esp. on macOS and Windows), it
automatically rebuilds if GAP changed (detected via changes to GAP's
sysinfo.gap), and of course there is no need for autoconf, automake,
autogen.sh etc., nor need to deal with M4 syntax in cryptic configure.ac
scripts
fingolfin added a commit to fingolfin/profiling that referenced this pull request Oct 27, 2019
Stop using autotools, and instead start to use Makefile.gappkg (see
<gap-system/gap#3683>); this has several advantages,
e.g. running configure is much quicker (esp. on macOS and Windows), it
automatically rebuilds if GAP changed (detected via changes to GAP's
sysinfo.gap), and of course there is no need for autoconf, automake,
autogen.sh etc., nor need to deal with M4 syntax in cryptic configure.ac
scripts
ChrisJefferson pushed a commit to gap-packages/json that referenced this pull request Nov 3, 2019
Stop using autotools, and instead start to use Makefile.gappkg (see
<gap-system/gap#3683>); this has several advantages,
e.g. running configure is much quicker (esp. on macOS and Windows), it
automatically rebuilds if GAP changed (detected via changes to GAP's
sysinfo.gap), and of course there is no need for autoconf, automake,
autogen.sh etc., nor need to deal with M4 syntax in cryptic configure.ac
scripts
ChrisJefferson pushed a commit to gap-packages/profiling that referenced this pull request Nov 3, 2019
Stop using autotools, and instead start to use Makefile.gappkg (see
<gap-system/gap#3683>); this has several advantages,
e.g. running configure is much quicker (esp. on macOS and Windows), it
automatically rebuilds if GAP changed (detected via changes to GAP's
sysinfo.gap), and of course there is no need for autoconf, automake,
autogen.sh etc., nor need to deal with M4 syntax in cryptic configure.ac
scripts
@fingolfin fingolfin changed the title Add parts for GAP package build systems Add etc/Makefile.gappkg, for use in the build system of GAP package with kernel extensions Nov 28, 2019
@fingolfin fingolfin added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Nov 28, 2019
@fingolfin fingolfin added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-4.11-DONE kind: discussion discussions, questions, requests for comments, and so on release notes: added PRs introducing changes that have since been mentioned in the release notes topic: build system topic: packages issues or PRs related to package handling, or specific to a package (for packages w/o issue tracker)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants