Currently libspectrum and fuse-utils use a non-recursive Makefile. I've been experimenting with a non-recursive makefile for Fuse since [#317]. Basically...
I've measured times from Linux, Cygwin and MSYS environments. Some tests do a full build or a partial build. My conclusions are:
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 |
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
It was experimental and I thought there were contrary opinions in the past. Seeing the recent interest, I will retake it.
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.
Thanks, I was aware of that but used one job in tests because had only 1 VCPU at the time.
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.)
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.
I've attached a patch updated to current version with some improvements:
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.
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:
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:
I'm concerned about the top-level AUTOMAKE_OPTIONS setting. Shouldn't we now be using:
If so, we might use some subset of the following:
Good find. Thanks for the feedback.
This expansion should be harmless, but I will better remove the space after it.
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.
Looks good to me.
I would start with:
unless we set 1.11 as the minimum supported version.
Patch attached with silent-rules tweaks.
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?
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.
Cool. Maybe also -Werror too, although perhaps we should wait?
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.
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.
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.
subdir-objects is a fairly recent addition [patches:#317], the decision of using AUTOMAKE_OPTIONS is lost in time.
I'm afraid about non-essential warnings that might block the build. If you have a strong opinion, please go ahead.
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:
Related
Patches:
#317It certainly can't hurt.
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 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.
Okay, I'll do that. We should probably also do '#include <compat stdint.h="">' for the same reason, I think?
Related
Commit: [r15]
Patches:
#317The changes look good, BTW.
Do you have an opinion on what to do about windres, though? My view is:
I'm not sure how much of this it make sense to do in the initial commit, though.
Definitely. Wildcard dependencies don't work. I recall touching windres.rc to force a rebuild.
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.
Agree.
It's Windows specific and not related to the non-recursive approach, I would leave it out for now.
Related
Commit: [r5216]
Well, I guess you can do that or delete the .o!
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.
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.
Related
Commit: [r5216]
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.
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
Related
Commit: [r5216]
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.
Thanks, committed in [r5302] with proper windres dependencies.
Related
Commit: [r5302]
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.)
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.
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.
Okay.
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.