Skip to content

Automake #277

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

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open

Automake #277

wants to merge 29 commits into from

Conversation

kugel-
Copy link
Member

@kugel- kugel- commented Jul 28, 2015

This PR applies http://karelzak.blogspot.de/2013/02/non-recursive-automake.html combined with automakes %reldir% feature, in order to keep make -C $subdir working.

I call this super-recursive automake. However, it is generally non-recursive, i.e. a make invocation in the root does not fork more make for the subdirs. It rather refers to the ability to transparently continue to make only subdirs with make -C $subdir (or cd $subdir; make).

The non-recursive part has to major advantages:

  • depenencies are globally known to make; for G-P this means that plugins don't need to be build in a specific order, but simply as soon as possible
  • in fact, all plugins can be built in parallell, given infinite CPU cores. But even on normal 4 core machine this gives a nice speed up of about 20s (timed make install: 1min:15s vs 1min:36s). It is most noticeable in the install phase which takes only 4s instead of 9s even on my SSD.

For this PR i haven't converted all plugins yet. Only the most trivial ones plugins git-changebar. Since a good third of the plugins is is still missing I expect the final speedup to be 30s (on my laptop).

See c8ea6d5 for a detailed description how the conversion is done, and also for the workaround we have to apply for an automake limitation (see foo_SHORTNAME).

@kugel-
Copy link
Member Author

kugel- commented Jul 28, 2015

I forgot to add: This requires automake 1.14 when building from git. As tarballs are naturally unaffected so packages generally aren't concerned. automake 1.14 was released in june 2013 and is part of Ubuntu LTS 14.04.

@sardemff7
Copy link
Contributor

I did not fully reviewed all of this yet, but here are my first thoughts.

I truly think having all those generated Makefiles around is not ideal. If you want to allow a subdir make call, you better have a stub Makefile. That remove the need for any workaround you did.

You should put the migration code to the first commit, not a random one converting the first plugin you picked.

Depending on Automake 1.14 was not planed as of #164 but @b4n may have changed his mind.

@kugel-
Copy link
Member Author

kugel- commented Jul 30, 2015

It's not a workaround, it's intended and absolutely my intention. Only generated Makefiles support all automake-specific targets automatically and the corresponding Makefile.am require no maintainance (each is the same 4 lines).

Stub makefiles (assuming you mean the ones that contain rules like "all:\nmake -C $(top_srcdir) $(plugin)/src/lib$(plugin).la") on the other hand require extra maintainance and you have to insert rules to make automake happy (for various non-all targets like check, dist-clean, etc).

Not sure what you mean with "migration code"? Do you mean the detailed description in the commit message?

@kugel-
Copy link
Member Author

kugel- commented Aug 16, 2015

Will this be considered? @b4n @frlan @eht16 ?

@b4n
Copy link
Member

b4n commented Aug 17, 2015

As I said when you mentioned that the first time, I don't think I myself feel like depending on Automake 1.14 yet. Yes, most current distros have it, but not all (like Debain oldstable (which isn't that old yet), or even Ubuntu 12.04 LTS).

%D%/wscript_build \
%D%/wscript_configure

plugin = addons
Copy link
Member

Choose a reason for hiding this comment

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

plugin= will break stuff if it's used, as it will keep getting redefined when using the toplevel Makefile.
Though, it seem to just be a leftover and not be used anymore, is it?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, it's a left-over

@b4n
Copy link
Member

b4n commented Aug 17, 2015

This said, and while I didn't try nor really review this, I like the idea of non-recursive AM -- yet being able to get normal features from inside subdirs.

@kugel-
Copy link
Member Author

kugel- commented Aug 17, 2015 via email

@kugel-
Copy link
Member Author

kugel- commented Oct 30, 2015

@frlan @b4n rebased to master to resolve merge conflicts

@frlan
Copy link
Member

frlan commented Oct 30, 2015

@b4n If you think it's fine to merge, I will do ;) Having a package time dependency is ok for me if we speed up build a lot. But I cannot say something about auto* quality

@frlan frlan added this to the 1.27 milestone Nov 7, 2015
This is to be included by wrapper Makefile.ams (and top-level Makefile.am) in a
super-recursive build system. This allows these Makefile.am to include
Makefile.rel.am files in order to reduce maintainance (Makefile.rel.am need
AM_* and other variables pre-initialized since they only add to them).

vars.auxfiles.mk uses it, and geanylua needs a tiny modification to work with
it (other plugins should be fine).
This and the following commits are done in two steps

Step geany#1 is completely scripted doing the following:

for each plugin do
  mv Makefile.am Makefile.rel.am
  apply small mods to Makefile.rel.am[1]
  echo $template > Makefile.am[2]
  cd src
    mv Makefile.am Makefile.rel.am
    apply small mods to Makefile.rel.am
    echo $template > Makefile.am[2]
end

[1]: Wrap the contents within if ENABLE_XXX (this is needed due to an autotools
limitation, see note about foo_SHORTNAME below) and add an include
src/Makefile.rel.am.

[2] $plugin/Makefile.am and $plugin/src/Makefile.am are all the same and require
no maintance at all.

Step geany#2 is done by hand, doing the following

for each plugin do
  apply %D% / %C% to $plugin/src/Makefile.rel.am [3]
  define %C%_foo_SHORTNAME if necessary[4]

[3]: These very modifications are the key part of super-recursive automake.
This uses the reldir feature of automake 1.14+. They allow the Makefile.rel.am
to be included by the top-level Makefile.am as well as by supdir Makefile.ams.

[4]: foo_SHORTNAME is necessary so that running make in the root builds the
same .lo files that make in a subdir does (make -C $plugin or make -C
$plugin/src). Otherwise automake encodes the directory part into the prefix of
the .lo files. Unforunately foo_SHORTNAME is not supported within conditional
statements. That's why it has to be outside if ENABLE_FOO ... endif. This is
also the reason that fragments are uncluded unconditionally. The definition of
foo_SHORTNAME is only necessary if there are target specific rules (e.g.
foo_CFLAGS).
%D%/wscript_build \
%D%/wscript_configure

plugin = geanyvc
Copy link
Member

Choose a reason for hiding this comment

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

leftover

@codebrainz
Copy link
Member

Should check with @techee, I think OSX has super old-timey Automake, though likely a newer version can be built/installed using one of the package managers available for OSX.

@b4n
Copy link
Member

b4n commented Nov 30, 2015

At the very least this has to be fixed/corrected:

  • leftovers
  • fix cppcheck support
  • full conversion (don't leave a third-or-so of the plugins recursive) (I know this is known, just want to point it out to @frlan)
  • documentation update (HACKING)

However, while I do like the technical advantage of the result (faster, parallelized builds from the bottom-up, less Make overhead, etc…), I don't really like the changes this PR makes. It adds duplication on stuff that was nicely generic (i.e. plugindoc stuff) and has some subtle gotchas in the recursive vs. non-recursive versions (SHORTNAME, conditionals, custom targets, etc.).
So in the end, I'm really not sure I like the changes, as I find the benefits not enough for the (current) added complexity.

@kugel-
Copy link
Member Author

kugel- commented Nov 30, 2015

  • full conversion (don't leave a third-or-so of the plugins recursive)

...

However, while I do like the technical advantage of the result (faster,
parallelized builds from the bottom-up, less Make overhead, etc…), I
don't really like the changes this PR makes. It adds duplication on
stuff that was nicely generic (i.e. plugindoc stuff) and has some
subtle gotchas in the recursive vs. non-recursive versions
(SHORTNAME, conditionals, custom targets, etc.).
So in the end, I'm really not sure I like the changes, as I find the
benefits not enough for the (current) added complexity.

So this is why I haven't converted all plugins yet. Merging is far from guaranteed. So I would like to get a decision based the current conversion with all other remarks fixed.

@codebrainz
Copy link
Member

depenencies are globally known to make; for G-P this means that plugins don't need to be build in a specific order, but simply as soon as possible

Is that really a thing even for GP? I have not much clue about how Make works, but it seems like since none of the plugins depend on each other, they can all already be parallelized and not build in a particular order. Is it a Make-ism why this isn't the case?

@kugel-
Copy link
Member Author

kugel- commented Dec 1, 2015

Am 1. Dezember 2015 02:19:10 MEZ, schrieb Matthew Brush [email protected]:

depenencies are globally known to make; for G-P this means that
plugins don't need to be build in a specific order, but simply as soon
as possible

Is that really a thing even for GP? I have not much clue about how Make
works, but it seems like since none of the plugins depend on each
other, they can all already be parallelized and not build in a
particular order. Is it a Make-ism why this isn't the case?


Reply to this email directly or view it on GitHub:
#277 (comment)

The default recursive automake setup which GP uses prevents this. This is exactly the purpose of this PR, enabling full parallel build.

Recursive automake processes SUBDIRS in a for loop which can't be done in parallel. This PR does away with SUBDIRSand instead creates proper make targets with complete dependencies. Then the recipes can be build in parallel.

@codebrainz
Copy link
Member

Recursive automake processes SUBDIRS in a for loop which can't be done in parallel. This PR does away with SUBDIRSand instead creates proper make targets with complete dependencies.

So yeah, an (auto)Make-isms.

@kugel-
Copy link
Member Author

kugel- commented Dec 1, 2015

s/(auto)Make-ism/automake-ism/

@b4n
Copy link
Member

b4n commented Dec 1, 2015

Yeah unfortunately Automake doesn't (yet) have a way to ask SUBDIRS to be built in parallel. It would indeed work for GP's top-level Makefile, but unfortunately there's no easy way to do that.
Non-recursiveness is also (slightly) more efficient than recursiveness just because it wastes less time spawning processes, analyzing files mdates and reading Makefiles, but that's probably less of a concern -- although on my machine (no SSD) running make on GP when there's nothing to do still takes around 3.5s (parallelized or not).

All this said, when I was writing the initial comment on SUBDIRS not being parallelizable, I had an idea for a hack to work around this. It's not really beautiful, but it does the job:

diff --git a/Makefile.am b/Makefile.am
index 8cf1634..148c67e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -176,3 +176,10 @@ EXTRA_DIST = \
        wscript \
        README.waf \
        README.windows
+
+
+all-recursive: $(SUBDIRS)
+.PHONY: $(SUBDIRS)
+
+$(SUBDIRS):
+       $(am__cd) $@ && $(MAKE) $(AM_MAKEFLAGS) all

What it does is recursing in each $(SUBDIRS) first, before making all-recursive. This obviously isn't prefect, as it is effectively slower when there's nothing to do, as it'll recurse in each $(SUBDIRS) twice; but it will parallelize the first run (as per -j option), so the second one won't do anything, meaning the actual build will be parallelized.
On my machine (2 physical, 4 logical cores), such an empty run takes around 7s when unparallelizd, and less than 5s when parallelized (-j4).
OTOH, a full build with -j4 goes from around 68s to around 53s (-15s, 22%). FTR, with CCache (so the actual build time is a LOT smaller), it goes from around 24s to around 16s (-8s, 33%).

This can also be done in a slightly more hackish way outside of Automake's Makefile, meaning it doesn't change how an Automake's Makefile run behaves. Using another Makefile (I named it makefile.parallel locally), like that:

#!/usr/bin/make -f
#
# Parallelizes the top-level Makefile of a SUBDIR-based Automake build system.
#
# What it actually does is recursing in $(builddir) for each target, and in
# each $(SUBDIRS) for target 'all'.  'all' has a dependency on $(SUBDIRS) so
# they are recursed in first, respecting `-j` option as any target.
#
# Note that this means that each $(SUBDIRS) is first built in parallel, and
# then again through the top-level Makefile for target 'all'.  This is normally
# not so much of a problem, as the targets should be up-to-date the second
# time, so you only get the Make overhead but no actual building.
# This is needed to "recurse" in '.' in case it's useful.
#
# As this makes uses of normal Make features, it is very possible to force some
# ordering by declaring explicit dependencies between some elements of
# $(SUBDIRS); for example if sub-directory 'po' depends on recursing in another
# one first, i.e. 'src', declare '$(builddir)/po: $(builddir)/src' (not
# forgetting '$(builddir)/' prefix), so 'src' is build before 'po'.
#
# Only 'all' is parallelized.  First, this is because it's likely to be the
# only target that really makes sense to parallelize, as it's the one building
# real things.  Also, as the parallelized targets are run twice (see above),
# a second run should have low overhead, so it doesn't make sense to e.g.
# parallelize i.e.g 'check' the same way, as it would run the checks twice --
# although parallelization would slightly help when building test programs.
# This could be addressed by omitting recursion in '.', but this can be a
# problem if './Makefile' contains anything beside recursion.
# Additionally, there's a technical difficulty in knowing which target was
# actually asked for, as each $(SUBDIRS) is a target itself, so $@ can't be
# relied on.
#
# Limitations:
# * Inspecting the value of Automake's SUBDIRS variable is naive, and doesn't
#   support expansions and the like.  This means that if $(SUBDIRS) from
#   Makefile.am contains variable references it'll most likely will break.

# user can set either builddir (when running from the source directory) or
# srcdir (when running from the build directory).  If builddir==srcdir, no
# overrides are needed.
builddir ?= .
srcdir ?= .

upper_level = $(srcdir)/Makefile.am

SED ?= sed

# extract SUBDIRS
SUBDIRS != $(SED) -ne '/SUBDIRS/s%^.*= \(.*\$\)%$(builddir)/\1%p' $(upper_level)

# MAKEFLAGS minus builddir= and srcdir=
filtered_makeflags = `echo $(MAKEFLAGS) | \
                      $(SED) -e 's/\(build\|src\)dir=[^ ]*//g'`

# all targets we want to forward somehow
TARGETS := all check clean distclean dist distcheck

all: $(SUBDIRS)

$(TARGETS):
    $(MAKE) -C $(builddir) $(filtered_makeflags) $@

$(SUBDIRS):
    $(MAKE) -C $@ $(filtered_makeflags)

.PHONY: $(SUBDIRS) $(TARGETS)

@b4n
Copy link
Member

b4n commented Dec 1, 2015

BTW, the unparallelized case can be optimized by only recursing first if Make was passed a -j option:

diff --git a/Makefile.am b/Makefile.am
index 8cf1634..9023c10 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -176,3 +176,12 @@ EXTRA_DIST = \
        wscript \
        README.waf \
        README.windows
+
+
+all-recursive: $(SUBDIRS)
+.PHONY: $(SUBDIRS)
+
+$(SUBDIRS):
+       @if (target_option=j; $(am__make_running_with_option)); then \
+               $(am__cd) $@ && $(MAKE) $(AM_MAKEFLAGS) all; \
+       fi

@techee
Copy link
Member

techee commented Dec 1, 2015

Should check with @techee https://github.com/techee, I think OSX has
super old-timey Automake, though likely a newer version can be
built/installed using one of the package managers available for OSX

The osx-integration uses jhbuild which builds automake 1.10 through 1.15 so
it shouldn't be an issue - but I haven't had time to try the patch here
yet.

@kugel-
Copy link
Member Author

kugel- commented Dec 2, 2015

@b4n nice experiment. However, I still consider my proposal technically superior, because it's more efficient as you mentioned (no recursion at all, etc) and makes dependencies global, although the latter is less of a concern for G-P.

@frlan frlan removed this from the 1.27 milestone Feb 29, 2016
@lpaulsen93 lpaulsen93 added the build system Automake build system issues label Jun 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build system Automake build system issues pending fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants