#321 Non-recursive makefile for Fuse

[Sergio Baldoví]

Currently libspectrum and fuse-utils use a non-recursive Makefile. I've been experimenting with a non-recursive makefile for Fuse since [#317]. Basically...

• There is a global Makefile with some parts included from 2nd level directories (e.g., sound/Makefile.am) and 3rd level directories for UIs (e.g., ui/gtk/Makefile.am).
• There are not convenience libraries (e.g., libcompatos.a).
• Some includes need a relative a path (e.g., ui/gtk/gtkcompat.h).
• `make clean' really clean all the project, not just last configured UI.

I've measured times from Linux, Cygwin and MSYS environments. Some tests do a full build or a partial build. My conclusions are:

• The non-recursive makefile performs slightly better. Fuse project is relatively small.
• There are less makefiles and autootols steps are simpler.
• The build time is almost the same. The gain is due to not traversing all subdirectories.

I've attached a patch.

Linux	Recursive	Non-recursive
autogen	7.8s	6.2s
configure	10.6s	9.0s
make all	1m 23.2s	1m 6.9s
make (nothing-to-do)	0.7s	0.4s
make (fuse.c)	1.6s	1.2s
make (disk.c)	2.6s	2.4s
make (ui.h)	33.8s	26.5s
make clean	0.7s	0.2s

Cygwin	Recursive	Non-recursive
autogen	24.4s	21.3s
configure	1m 14.9s	56.1s
make all	1m 45.6s	1m 43.1s
make (nothing-to-do)	2.3s	0.6s
make (fuse.c)	6.4s	4.8s
make (disk.c)	7.7s	6.0s
make (ui.h)	47.1s	44.6s
make clean	5.0s	1.6s

MSYS	Recursive	Non-recursive
autogen	19.2s	17.7s
configure	1m 15.6s	57.9s
make all	1m 58.2s	1m 58.8s
make (nothing-to-do)	12.9s	8.1s
make (fuse.c)	17.5s	12.8s
make (disk.c)	19.0s	14.1s
make (ui.h)	1m 02.8s	58.2s
make clean	7.2s	4.5s

[Fredrick Meunier]

Hi Sergio,
Not sure how this one got dropped - I think we generally are keen to have a non-recursive make in Fuse, can you bring this up to date for trunk?

Thanks,
Fred

[Sergio Baldoví]

It was experimental and I thought there were contrary opinions in the past. Seeing the recent interest, I will retake it.

[Fredrick Meunier]

Re-reviewing I think that at the time there was primarily no interest.

I think you may also be able to get larger speedups with make -j<N> as the recursive version will wait until a directory is completely compiled while the non-recursive version can start compiling the next object straight away.

[Sergio Baldoví]

Thanks, I was aware of that but used one job in tests because had only 1 VCPU at the time.

[Stuart Brady]

That's not quite true: I was certainly interested, although I'll admit I didn't particularly express that interest at the time! (Apologies, when this was originally posted I was quite busy.)

[Stuart Brady]

Oh, note also the V=0 override to Make which might cut down the time taken by your terminal emulator. :-)

[Sergio Baldoví]

Oh, note also the V=0 override to Make which might cut down the time taken by your terminal emulator. :-)

Thanks. I have seen this low verbosity output before but I was not aware how to enable it. I plan to add verbosity rules for perl and windres commands after switching to the non-recursive makefile.

[Sergio Baldoví]

I've attached a patch updated to current version with some improvements:

• Using makefile variables like $(PERL) rather than direct substitutions (@PERL@) allow easier modifications and patches to the Makefile.
• Remove duplicate option.h rule
• Remove UI_LIBS, WINDRES_OBJ substitutions
• svn:ignore properties clean-up

I plan to commit it next week, barring objections.

There is still a dependency for X11 with GTK+ UI. This should be fixed later for the sake of Windows and MacOSX subsystems.

[Stuart Brady]

Instead of V=0, you can also use the --enable-silent-rules configure option. You can then enable verbosity if needed by passing V=1 to make.

For Automake 1.11 and Automake 1.12 we would need to explicitly enable support for this in configure.ac: https://autotools.io/automake/silent.html

Should we check for Automake 1.11, now that we use $(AM_V_CC)? I suppose it will just expand to nothing for old versions of Automake, so we should be okay?

AUTOMAKE_OPTIONS overrides the options passed to AM_INIT_AUTOMAKE, so I'm not sure if using AM_SILENT_RULES would have avoided needing to alter every single AUTOMAKE_OPTIONS line, but as your patch removes (almost) all of the AUTOMAKE_OPTIONS lines, it now probably makes more sense just to use the "silent-rules" option. We should maybe even add the "-Wall" Automake option:

   AM_INIT_AUTOMAKE([silent-rules subdir-objects -Wall])

The -Wall option currently causes automake to spit out a whole load of warnings such as the following, which are all fixed by your non-recursive patch:

   /usr/share/automake-1.14/am/library.am: warning: 'libz80.a': linking libraries using a non-POSIX
   /usr/share/automake-1.14/am/library.am: archiver requires 'AM_PROG_AR' in 'configure.ac'
   z80/Makefile.am:28:   while processing library 'libz80.a'

I'm concerned about the top-level AUTOMAKE_OPTIONS setting. Shouldn't we now be using:

   AM_INIT_AUTOMAKE([foreign subdir-objects])

If so, we might use some subset of the following:

   AM_INIT_AUTOMAKE([foreign silent-rules subdir-objects -Wall -Werror])

[Sergio Baldoví]

For Automake 1.11 and Automake 1.12 we would need to explicitly enable support for this in configure.ac: https://autotools.io/automake/silent.html

Good find. Thanks for the feedback.

Should we check for Automake 1.11, now that we use $(AM_V_CC)? I suppose it will just expand to nothing for old versions of Automake, so we should be okay?

This expansion should be harmless, but I will better remove the space after it.

AUTOMAKE_OPTIONS overrides the options passed to AM_INIT_AUTOMAKE, so I'm not sure if using AM_SILENT_RULES would have avoided needing to alter every single AUTOMAKE_OPTIONS line, but as your patch removes (almost) all of the AUTOMAKE_OPTIONS lines, it now probably makes more sense just to use the "silent-rules" option.

I would say AUTOMAKE_OPTIONS takes precedence rather than overwrite, otherwise subdir-objects would not work. IMO it is simpler to use one place when setting the options.

We should maybe even add the "-Wall" Automake option:

Looks good to me.

I'm concerned about the top-level AUTOMAKE_OPTIONS setting. Shouldn't we now be using:
AM_INIT_AUTOMAKE([foreign subdir-objects])
If so, we might use some subset of the following:
AM_INIT_AUTOMAKE([foreign silent-rules subdir-objects -Wall -Werror])

I would start with:

    AM_INIT_AUTOMAKE([foreign subdir-objects -Wall])
    m4_ifdef([AM_SILENT_RULES], [AM_SILENT_RULES])

unless we set 1.11 as the minimum supported version.

Patch attached with silent-rules tweaks.

[Stuart Brady]

This expansion should be harmless, but I will better remove the space after it.

The space doesn't even seem to be visible to me (I replaced AM_V_CC with AM_V_FOO to test), so perhaps there's I don't understand about this?

I would say AUTOMAKE_OPTIONS takes precedence rather than overwrite, otherwise subdir-objects would not work. IMO it is simpler to use one place when setting the options.

Ah, I see: "As a general rule, options specified in 'AUTOMAKE_OPTIONS' take precedence over those specified in 'AM_INIT_AUTOMAKE', which in turn take precedence over those specified on the command line."

Having the options all in one place certainly seems simplest. I don't get why we didn't just pass foreign to AM_INIT_AUTOMAKE in the first place? It can't be to support the old AM_INIT_AUTOMAKE syntax because as you mention, we already used subdir-objects.

We should maybe even add the "-Wall" Automake option:
Looks good to me.

Cool. Maybe also -Werror too, although perhaps we should wait?

I would start with:
AM_INIT_AUTOMAKE([foreign subdir-objects -Wall])
m4_ifdef([AM_SILENT_RULES], [AM_SILENT_RULES])
unless we set 1.11 as the minimum supported version.

Right. Old versions of Automake would otherwise complain that they don't handle the "silent-rules" option, and it'd be an error, not just a warning. Seems okay to me. I do wonder what the oldest version of Automake that will work is, though.

Patch attached with silent-rules tweaks.

Looks good. If it'd help you in getting this committed sooner, I think the inclusion changes could go in straight away. I guess you could also split out changing of the AUTOMAKE_OPTIONS options, and switch to using global options, if you'd feel happier doing it that way. The final commit would then concentrate purely on switching to non-recursive makefiles.

BTW, I'm not sure of our support for building outside of the source directory. It looks like some <config.h> vs "config.h" issues may have crept in but I'm not sure of the exact difference this makes.

[Sergio Baldoví]

This expansion should be harmless, but I will better remove the space after it.

The space doesn't even seem to be visible to me (I replaced AM_V_CC with AM_V_FOO to test), so perhaps there's I don't understand about this?

It's better to follow the syntax used in documentation and auto-generated rules, just in case there is a portability issue with some make versions.

Having the options all in one place certainly seems simplest. I don't get why we didn't just pass foreign to AM_INIT_AUTOMAKE in the first place? It can't be to support the old AM_INIT_AUTOMAKE syntax because as you mention, we already used subdir-objects.

subdir-objects is a fairly recent addition [patches:#317], the decision of using AUTOMAKE_OPTIONS is lost in time.

We should maybe even add the "-Wall" Automake option:

Looks good to me.

Cool. Maybe also -Werror too, although perhaps we should wait?

I'm afraid about non-essential warnings that might block the build. If you have a strong opinion, please go ahead.

BTW, I'm not sure of our support for building outside of the source directory. It looks like some <config.h> vs "config.h" issues may have crept in but I'm not sure of the exact difference this makes.

VPATH builds are supported, I find that useful for testing different compilers. The use of #include "config.h" is marginal (6 files) and we should probably standardize it:

"With the appropriate -I option, you can use ‘#include <config.h>’. Actually, it's a good habit to use it, because in the rare case when the source directory contains another config.h, the build directory should be searched first."

[Stuart Brady]

It's better to follow the syntax used in documentation and auto-generated rules, just in case there is a portability issue with some make versions.

It certainly can't hurt.

subdir-objects is a fairly recent addition [patches:#317], the decision of using AUTOMAKE_OPTIONS is lost in time.

Indeed. [r15] is the point when the initial bits of recursive make were added (just the libspectrum, roms and z80 directories). Probably Phil was just following whatever documentation he had to hand, which didn't document the other approach or didn't make the advantage of this sufficiently clear.

I'm afraid about non-essential warnings that might block the build. If you have a strong opinion, please go ahead.

I'm in agreement with you. We can either do this and then back the change out in the release branch, or just wait until we've released. I think it's simplest just to wait, and I think it's unlikely our use of Automake will produce any important warnings, so there'd be little real benefit.

VPATH builds are supported, I find that useful for testing different compilers. The use of #include "config.h" is marginal (6 files) and we should probably standardize it:

Okay, I'll do that. We should probably also do '#include <compat stdint.h="">' for the same reason, I think?

[Stuart Brady]

The changes look good, BTW.

Do you have an opinion on what to do about windres, though? My view is:

• We should no longer refer to ui/win32/*.rc. At the very least, we should build up a list of Windows resource files rather than using a wildcard.
• Going a little further, each resource file should be compiled into a separate object file which should be passed in separately when linking. We can rename the rc files if needed, but we should use a generic rule for compiling them into object files.
• Would have a clean separation between Win32 UI and MinGW support, allowing SDL builds to work once again on Win32. This is basically just a trivial matter of distinguishing properly between COMPAT_WIN32 and UI_WIN32.

I'm not sure how much of this it make sense to do in the initial commit, though.

[Sergio Baldoví]

Do you have an opinion on what to do about windres, though? My view is:

• We should no longer refer to ui/win32/*.rc. At the very least, we should build up a list of Windows resource files rather than using a wildcard.

Definitely. Wildcard dependencies don't work. I recall touching windres.rc to force a rebuild.

• Going a little further, each resource file should be compiled into a separate object file which should be passed in separately when linking. We can rename the rc files if needed, but we should use a generic rule for compiling them into object files.

This idea has crossed my mind, something similar to [r5216]. But I'm not sure if will be valid to link multiple resources in COFF format. I suspect there was a reason for assembling one resource file.

• Would have a clean separation between Win32 UI and MinGW support, allowing SDL builds to work once again on Win32. This is basically just a trivial matter of distinguishing properly between COMPAT_WIN32 and UI_WIN32.

Agree.

I'm not sure how much of this it make sense to do in the initial commit, though.

It's Windows specific and not related to the non-recursive approach, I would leave it out for now.

[Stuart Brady]

Definitely. Wildcard dependencies don't work. I recall touching windres.rc to force a rebuild.

Well, I guess you can do that or delete the .o!

This idea has crossed my mind, something similar to [r5216]. But I'm not sure if will be valid to link multiple resources in COFF format. I suspect there was a reason for assembling one resource file.

I'm not aware of a problem with linking multiple resource files. I must have felt it was better to pollute windres.rc with includes than to pollute the top-level makefile with a list of resource files.

The reason for the mess is that Windows resources linked into a convenience library were discarded under MinGW32, although I'm not sure if this happened when the archive was made, or during the final link.

I'm fairly sure I tried adding a single resource file in ui/win32 to include the others, so it wasn't that there were multiple objects with the same name in the convenience library. I remember running into issues regarding dependencies involving autogenerated resource files. In the end we found it turned out to be simpler to generate the resource object in the top-level makefile than to generate it in the ui/win32 makefile, and of course, there are the non-Win32-UI-specific parts of windres.rc.

It's Windows specific and not related to the non-recursive approach, I would leave it out for now.

Which parts? I think the "don't use wildcards" part is essential for non-recursive make. We can no longer rely on recursive make for ordering of operations so we need to get the Windows resource dependencies sorted out properly. If the easiest way to do this allows us to trivially make the proper distinction between COMPAT_WIN32 and UI_WIN32 as a by-product, then so be it, I would say.

I think it's fine to break the Windows build for a commit or two, maybe, if that would help, but we should try to keep it working.

[Sergio Baldoví]

This idea has crossed my mind, something similar to [r5216]. But I'm not sure if will be valid to link multiple resources in COFF format. I suspect there was a reason for assembling one resource file.

I'm not aware of a problem with linking multiple resource files. I must have felt it was better to pollute windres.rc with includes than to pollute the top-level makefile with a list of resource files.

IMO it needs proper testing. The official documentation ([1]) follow the single resource file and I have found two possible issues ([2] and [3]) with multiple resource objects.

It's Windows specific and not related to the non-recursive approach, I would leave it out for now.

Which parts? I think the "don't use wildcards" part is essential for non-recursive make. We can no longer rely on recursive make for ordering of operations so we need to get the Windows resource dependencies sorted out properly.

Dependencies apart, I'm failing to see any critical issue with ordering. Anyway, the final patch tracks windres dependencies.

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa380599%28v=vs.85%29.aspx
[2] https://bz.apache.org/ooo/show_bug.cgi?id=27702#c6
[3] https://www.redhat.com/archives/virt-tools-list/2013-January/msg00048.html

[Stuart Brady]

BTW, I'd like to generate our stdint.h wrapper/replacement as compat/stdint.h if that sounds reasonable, but I can't do this whilst "compat" is ever in the include path (or else when compat/stdint.h tries to include <stdint.h>, it will end up including itself, rather than the real stdint.h).

As such, I'd encourage you to commit this today, if you have time.

[Sergio Baldoví]

Thanks, committed in [r5302] with proper windres dependencies.

[Stuart Brady]

Thanks!

BTW, should the dependency of windres.o be $(srcdir)/windres.rc instead or is Automake smart enough to work this out? Likewise for winfuse.ico. (I suppose ui/win32/fuse.manifest is generated so I'm not sure about this.)

[Sergio Baldoví]

BTW, should the dependency of windres.o be $(srcdir)/windres.rc instead or is Automake smart enough to work this out? Likewise for winfuse.ico.

It's a makefile rule (not automake) and IIRC fist the source tree is checked and then the build tree.

(I suppose ui/win32/fuse.manifest is generated so I'm not sure about this.)

ui/win32/fuse.manifest is generated in the build tree. I have tested the dependencies and it works.

Probably I should move winfuse.ico, ui/win32/fuse.manifest.in and ui/win32/installer/fuse.nsi.in to data/win32 as it could be used without WIN32 UI.

[Stuart Brady]

It's a makefile rule (not automake) and IIRC fist the source tree is checked and then the build tree.
ui/win32/fuse.manifest is generated in the build tree. I have tested the dependencies and it works.

Okay.

Probably I should move winfuse.ico, ui/win32/fuse.manifest.in and ui/win32/installer/fuse.nsi.in to data/win32 as it could be used without WIN32 UI.

Yes, please do!

I found Windows SDL builds useful when ruling out the Win32 UI when dealing with problems prior to the old Win32 beta releases, and it's useful to be able to compare with the Widget UI when you're developing on that platform. At some point, we might aim for better file selection (allowing UNC paths to be specified, etc.) but for now I think it's good enough just as a development aid and it's something we should try to keep working (especially given the minimal effort required to do so).

The one crucial thing the SDL UI handles that the Win32 UI does not is full-screen mode. TBH, I would still prefer a choice of binaries on Windows until this is resolved, but now that we autogenerate an installer in the way that we do, I'm not sure if this is feasible any longer.