Re: [PATCH] lib/readline/doc makefiles clean targets

[Mike Jonkmans]

On Thu, Nov 07, 2024 at 06:05:36PM -0500, Chet Ramey wrote:
> On 11/4/24 9:44 AM, Mike Jonkmans wrote:
> 
> > > > According to info (make)Standard Targets, we should have:
> > > >      mostlyclean < clean < distclean < maintainer-clean
> > > That hierarchy isn't gospel.
> > Agreed, but...
> > > The current Makefile rules are fine.
> > There are some issues though. Some less important.
> > 1) After 'make distclean', it doesn't build: some .o files need y.tab.h.
> y.tab.h isn't part of the devel branch, you need bison to build it, and
> it is included in the bash distributions. I see the problem here, though.
> I'll figure out a way to fix it that's compatible with it being included
> and not without too many release-specific changes.
> 
> The fundamental difference is that the devel branch and the master
> (distribution) branch contain different files, and serve different
> purposes from a common source base, and the existing Makefiles are
> built for the master branch. That's why, for instance, the distclean
> target in doc/Makefile.in removes the texinfo intermediate files -- they
> are not part of the distributions. They appear in the devel branch for
> convenience. I don't think their presence makes it any less easy to work
> with the devel branch, though.

I see your problem, it is not that easy to keep the Makefiles correct for
both branches on all rules. You're doing fine, the issues I bring up are
mostly cosmetical or a matter of opinion.

> If you find yourself in this sitution, it's easy enough to remove
> parser-built and y.tab.c and run make again before I fix it.

I could use a GNUMakefile to get around some of the issues.
A fix would be nice.

> > 2) The use of recursive make, makes it harder to do dependencies right;
> It's the best way to build using optional components and subdirectories.

I have my doubts here. There are potential problems with dependencies
and parallel makes wrt linking.
The top Makefile could use make's include facility instead.
Though that is a bit involved and might be better suited to GNU's make.

> If you have missing dependencies, send them along.
> > 2a) Building outside of the source tree does not have proper deps;
> As above.


> > 3) The various Makefiles disagree on 'clean: mostlyclean' versus
> >     'mostlyclean: clean';
> Is this substantive or just cosmetic?

It's a minor cognitive burden, mostly cosmetic.

> > 4) .PHONY is not used for the sub-makes *clean rules;
> You mean in the subdirectory makefiles?
Yes. Though I was focussing on the clean rules.
Only two Makefiles have (incomplete) .PHONY rules:
        # find . -name Makefile | xargs grep PHONY
        ./lib/malloc/Makefile:.PHONY:           malloc stubmalloc
        ./Makefile:.PHONY: basic-clean clean maintainer-clean distclean 
mostlyclean maybe-clean

One can argue about the substantive versus cosmetic aspect of .PHONY.
As it is highly unlikely to accidently create a file named after a target.
But they don't hurt - and FWIW may save a stat or two.

> > 5) git (status), .gitignore and make are disconnected;
> What does this mean? There are a couple of files that I need to add
> to .gitignore; is this something else?

I suggest per directory .gitignore files, that contain:
        *
        !dir1
        !dir2
        !a.c
        !b.c
        ...

Which could be generated from the existing files, after a 'make distclean'
(and so could depend on git's branch automatically).
One big .gitignore is also possible.

> > 6) There is redundancy/duplication in the actions of various rules;
> OK.
> > 6) Same for the definitions of variables.
> Examples?

There is duplication in the top Makefile on the various lib sources.
One might dedup that by creating e.g. lib/glob/Makefile.ext and include
that on the toplevel (the lib's Makefile should then use GLOB_SOURCES).

lib/glob/Makefile:
        Seems that gmisc.c is missing from CSOURCES
        CSOURCES might then be derived from OBJECTS.

The top Makefile defines GLOB_SOURCES, different from those CSOURCES.
Oh, i see, #include "glob_loop.c" etc. :(
Those *_loop.c files are not included in CSOURCES nor in HSOURCES.
Could be a problem for the what-tar target (via THINGS_TO_TAR).


Another cosmetic thingy in the top Makefile.
Various *_ABSSRC are defined, but never used.

> > > > (One could argue, whether the devel branch is a distribution.)
> > > It's not. It's a snapshot of ongoing work.
> > As such, it has the purpose of:
> > - bringing in early fixes/patches;
> > - communicating future changes, eliciting comments.
> In many cases, yes. In some cases, I solicit discussion, usually here.
> > So the devel branch should be easy to work with for outsiders.
> How would the standard make targets make it easier to work with (besides
> fixing a couple of previously-discussed issues)? What are we doing here
> beyond `configure; make' and testing out various behaviors?

Making people not chase red herrings. I mean that in the output of
'make mostlylean', seeing 'clean' called, makes me want
to look at what is wrong.
 
> > That, in my opinion, would warrant more adherence to the
> > standard make targets.
> Why?

It is about expectations. I expect to remove more with maintainer-clean,
than distclean than clean than mostlyclean.
It is partly cosmetic, partly cognitive.

> > It doesn't need to be religious,
> > perhaps stating deviations and/or documenting what the expectations
> > are on the various make-targets. INSTALL already has some text.
> Are these `standard make targets' anything other than clean and
> mostlyclean?

(From 'info (make)Standard Targets')
All GNU programs should have the following targets in their Makefiles:
        all install install-html install-dvi install-pdf install-ps
        install-strip uninstall
        clean distclean mostlyclean maintainer-clean
        TAGS info dvi html pdf ps dist check installcheck installdirs

The wording has 'should have'. But I think that if you have any of these
targets, they should be doing the expected.

> > Are you often building from devel yourself? As maybe a check, in git,
> > on a push to devel, would be nice to have.
> I always build the code I push to the devel branch from a clean tree
> before I push it.

Yes, devel always seems to be building allright.
That's probably why there are not so many 'substantive' problems.

> > Anyway, I suppose that for the clean-targets (not maintainer-clean)
> >    git restore .; configure; make `clean-target'
> > should see no deleted/modified files in 'git status'.
> > This currently is not the case for mostlyclean and distclean.
> Well, see above about the difference between the devel and master
> branches.
> There's a lot of stuff that's in the devel branch for convenience,
> and some that's the opposite (y.tab.?). You can do some things with
> .gitignore, but that's not enough for all the differences.
> I do primarily build the devel branch in build directories different
> from the source directory, and I always recommend doing that, so
> maybe that masks some of the issues you're encountering.

Indeed, building ouside of the source directories will not encounter
the .gitignore problems.
Vaguely rembering some problems with building outside the source.
I shall give that another try.

> > Misc notes
> > - examples/loadables/Makefile(.in) has no mostlyclean target.
> >    The error is ignored, but clutters the output.
> ?
> $ grep mostlyclean examples/loadables/Makefile.in
> mostlyclean:  clean

I am deluded. Can't reproduce it anymore. Sorry.

> > - 'make mostlyclean' removes doc/bashref.* and doc/Makefile.
> It shouldn't remove the Makefile, and you can make it again, but I can't
> reproduce it removing the bashref.* files.

On devel:
        make distclean
        git restore .
        ./configure
        make mostlyclean
        git status
Shows several lines of 'deleted: doc/bashref.*'
Then:
        make distclean
Shows several extra deleted items 'lib/readline/doc/*'

> >    No recovery, must run 'git restore doc; configure'.
> > - in doc/Makefile: the maybe-clean test should use realpath (cd ...; pwd -P)
> I already made that exact change, actually, to match Makefile.in.

Nice!

-- 
Regards, Mike Jonkmans