Get Makefile to conform to GNU Makefile conventions, and be better

[ndim]

This fixes bugs like trying removing the wrong directory, introduces some make variables to help with DRY, stops installing cow files cowsay cannot handle and possibly non-cow files, allows installation preserving file timestamps on install, replaces the installed cowthink.1 with a symlink, etc.

This also allows packagers to preserve file timestamps at "make install" time by setting INSTALL_DATA="install -p -m 0644" and INSTALL_PROGRAM="install -p" or maybe just INSTALL="install -p", and it also stops installing files using cp which respects umask.

Overall, this would make it possible for e.g. the Fedora cowsay package to drop the custum install code.

[apjanke]

I like it!

Would you mind squashing this to a single commit? (I could do this myself, but not sure it would preserve author credit, and you can pick the commit message for the single commit.)

Also, I dunno about this extra in-line spacing here:

I don't think I've seen that before? Is this standard/conventional?

[apjanke]

it also stops installing files using cp which respects umask

Can you tell me a little more about this? I was unaware of this cp/umask issue, and this seems like something I should actually be familiar with.

[ndim]

Would you mind squashing this to a single commit? (I could do this myself, but not sure it would preserve author credit, and you can pick the commit message for the single commit.)

I made a series of commits on purpose as this makes it easy to understand and review the individual changes, and therefore will make it easier to understand things when looking back at the git history later.

Squashing this into a single commit will result in a commit with either a very long and therefore difficult to understand commit message, or with a summary commit message which does not really say much about the individual changes.

The summary commit message could be well suited for a merge commit, though.

Do you want me to create a commit message for that merge commit, or do you still prefer the one squashed commit?

Also, I dunno about this extra in-line spacing here:

I don't think I've seen that before? Is this standard/conventional?

This is mostly me finding source code easier to read when the same thing looks the same and when a similar thing looks similar.

install: man
	$(INSTALL_DIRS)           $(DESTDIR)$(bindir)
	$(INSTALL_PROGRAM) cowsay $(DESTDIR)$(bindir)/cowsay
	rm -f                     $(DESTDIR)$(bindir)/cowthink
	$(LN_S)            cowsay $(DESTDIR)$(bindir)/cowthink
	$(INSTALL_DIRS)          $(DESTDIR)$(mandir)/man1
	$(INSTALL_DATA) cowsay.1 $(DESTDIR)$(mandir)/man1
	rm -f                    $(DESTDIR)$(mandir)/man1/cowthink.1
	$(LN_S)         cowsay.1 $(DESTDIR)$(mandir)/man1/cowthink.1
	$(INSTALL_DIRS)              $(DESTDIR)$(cowsdir)
	$(INSTALL_DATA) $(COW_FILES) $(DESTDIR)$(cowsdir)
	$(INSTALL_DIRS)              $(DESTDIR)$(sitecowsdir)

If you minimize the whitespace, you end up with a Makefile few bytes smaller, but you lose the information which used to be visible at a glance that

• the first four lines of the recipe are related to $(DESTDIR)$(bindir)
• the the second set of four lines of the recipe are related to $(DESTDIR)$(mandir)/man1
• the consecutive rm -f and $(LN_S) lines use the same last arg and therefore are related
• the rm -f and $(LN_S) pair of lines is repeated later for cowthink.1
• we always first create the directory ($(INSTALL_DIRS)) and only then install files to it ($(INSTALL_DATA) and $(INSTALL_PROGRAM))
• typos in things which are supposed to be the same can be very obvious

Of course, this will be difficult to impossible to see for people using a screen reader or for people using a proportional width font or for people who just perceive text formatting differently. (Visually impaired people using a braille terminal will probably benefit from aligned line parts, even though a screen reader will probably ignore the alignment.)

I think it is a majority of people who can benefit from nicely formatted code besides myself, though.

install: man
	$(INSTALL_DIRS) $(DESTDIR)$(bindir)
	$(INSTALL_PROGRAM) cowsay $(DESTDIR)$(bindir)/cowsay
	rm -f $(DESTDIR)$(bindir)/cowthink
	$(LN_S) cowsay $(DESTDIR)$(bindir)/cowthink
	$(INSTALL_DIRS) $(DESTDIR)$(mandir)/man1
	$(INSTALL_DATA) cowsay.1 $(DESTDIR)$(mandir)/man1
	rm -f $(DESTDIR)$(mandir)/man1/cowthink.1
	$(LN_S) cowsay.1 $(DESTDIR)$(mandir)/man1/cowthink.1
	$(INSTALL_DIRS) $(DESTDIR)$(cowsdir)
	$(INSTALL_DATA) $(COW_FILES) $(DESTDIR)$(cowsdir)
	$(INSTALL_DIRS) $(DESTDIR)$(sitecowsdir)

The price to pay for the above benefits is

• file sizes are a few bytes larger (not much of an issue these days)
• there might be some arcane shell implementation which has a problem with more than one space on the command line (that is wild speculation), or with maximum allowable command line length (not much of an issue these days)
• the proportional font and screen reader people might mess up the formatting later, but the code reviewer can help there

I think the benefits outweigh the drawbacks, but your metrics may vary.

This is similar to manual code formatting versus enforcing what some clang-tidy or indent or flake8 code formatting tool says. The automatically formatted code from the tool is a lot better than code manually indented badly and inconsistently. However, you pay a price for that by not having the benefits of manually well-formatted code.

(I might be a bit old school in using a code editor with a monospace font and caring about code indented for readability.)

Do you want me to change this to the minimized whitespace variant?

[ndim]

it also stops installing files using cp which respects umask

Can you tell me a little more about this? I was unaware of this cp/umask issue, and this seems like something I should actually be familiar with.

I ran into this with the Fedora cowsay package. The Fedora package ships a separate animalsay script which is (mistakenly) stored in the git repo with mode 0664. The package then copies the files to the final installation destination using cp -p, and for the animalsay script, it does chmod +x.

This ends up with a /usr/bin/animalsay script which is mode 0775 instead of 0755. At least using some build environments (mockbuild).

OK, so cp -p is supposed to preserve the file mode. However, when I read info cp about the -p option, it says

     In the absence of this option, the permissions of existing
     destination files are unchanged.  Each new file is created with the
     mode of the corresponding source file minus the set-user-ID,
     set-group-ID, and sticky bits as the create mode; the operating
     system then applies either the umask or a default ACL, possibly
     resulting in a more restrictive file mode.  *Note File
     permissions::.

so the "operating system then applies either the umask"... but when installing files I want the installed file to have well defined permissions, and not depend on whatever umask happens to be set, or the source file's filesystem permissions.

So if there is this install tool created to install files properly... why not use that instead of cp? (Especially when we are already using the install tool for other files.)

[ndim]

I have three branches now with identical trees, any of which I can force-push to this PR's make-install branch

• https://github.com/ndim/cowsay/tree/make-install-commit-series (series of commits like the original PR)
• https://github.com/ndim/cowsay/tree/make-install-including-merge-commit (merge commit for the series of commits, not to be "merged")
• https://github.com/ndim/cowsay/tree/make-install-squashed (series of commits squashed into one commit)

I have just force-pushed the squashed variant to here, but I can force-push any of them to here.

[ndim]

I have just added commit bfb65fd to the series, and squashed it into the squashed branch and re-created the merged branch.

That commit creates an empty cowpath.d directory on make install.

[apjanke]

OK, so cp -p is supposed to preserve the file mode. However, when I read info cp about the -p option, it says...
[...]
So if there is this install tool created to install files properly... why not use that instead of cp?

Ah! I learned something today. This install vs. cp thing makes sense to me now. Yeah, let's go with install then. (Also because that's what the GNU Coding Standards recommend, right?)

IIRC, install moves files instead of copying them, which means doing a make installis a destructive operation to your local working copy. But git and other modern source control systems make that easy to manage and work around.

install it is.

Do you want me to change this to the minimized whitespace variant?

Yeah, please change it to the minimized whitespace variant. I can see why the tabular-style extra whitespace form can be nice. But it's not what I've seen in most other GNU style Makefiles. And I find it a little harder to read myself; I think I tend to scan each line as a command in turn, instead of viewing it all as a tabular structure.

Overall

The new squashed commit is looking good to me! I like that version of the commit history best of the branches you linked.

I'll test this out. Redo the whitespace please and I think we're ready to merge here.

[ndim]

IIRC, install moves files instead of copying them, which means doing a make installis a destructive operation to your local working copy. But git and other modern source control systems make that easy to manage and work around.

Negative. install does not remove or change the source file at all.

[ndim]

The new squashed commit is looking good to me! I like that version of the commit history best of the branches you linked.

I'll test this out. Redo the whitespace please and I think we're ready to merge here.

Ready on my part, then.

[apjanke]

Negative. install does not remove or change the source file at all.

Ah, I'm thinking of old behavior (that maybe only was in some *nix variants?). Found this in macOS's BSD man install:

     -c      Copy the file.  This is actually the default.  The -c option is only included for backwards compatibility.
     
 [...]
 
 COMPATIBILITY
     Historically install moved files by default.  The default was changed to copy in FreeBSD 4.4.

[ndim]

Negative. install does not remove or change the source file at all.

Ah, I'm thinking of old behavior (that maybe only was in some *nix variants?). Found this in macOS's BSD man install:

     -c      Copy the file.  This is actually the default.  The -c option is only included for backwards compatibility.
     
 [...]
 
 COMPATIBILITY
     Historically install moved files by default.  The default was changed to copy in FreeBSD 4.4.

Hmm. Change the INSTALL = install definition to INSTALL = install -c perhaps?

I vaguely remember having seen that somewhere, but I couldn't find what -c actually does (GNU coreutils install docs just say that -c is ignored). After checking, I can confirm that the default definition of INSTALL an automake generated configure script generates is INSTALL = /usr/bin/install -c.

[ndim]

I have changed the INSTALL definition to INSTALL = install -c.

If automake does that by default for wider compatibility, that looks like a very good example to follow.

[apjanke]

I have changed the INSTALL definition to INSTALL = install -c.

If automake does that by default for wider compatibility, that looks like a very good example to follow.

I agree with this reasoning.

[apjanke]

This is looking good to me now!

I added one last comment/question about the uninstallation process's handling of some directories which may interact with other packages or site-local configuration.

[apjanke]

This PR doesn't have much in the way of end-user-facing stuff, but like you say, it could be nice for downstream package maintainers, so once this hits master I'm going to cut a cowsay 3.8.0 release that includes it. That will also pick up the man page improvements, cowthink as symlink, empty-message fix, and the new "fox" cow.

It's been long enough since a new release anyway.

[apjanke]

Hi! Sorry for being so slow here.

If you still want to get this merged, would you mind rebasing on main? I recently migrated from AsciiDoc to Asciidoctor (finally figuring out #38), and that affects some of the commands here. In particular, a2x is gone, and needs to be replaced with asciidoctor calls.

[ndim]

Give me a sec.

[ndim]

Rebased, ready to be merged.

And after that merge, #45 will be waiting to be merged.

[apjanke]

Merged in b52012d.

(I did a manual cherry-pick in git, instead of merging this PR in GitHub, to do additional testing etc before merging.)

Thanks for your contribution and patience here!

I think this, in combination with other recent changes, may have fixed the "installation under /usr is broken" problem. Since pulling this in, installs to /usr seem to work fine, and I can't reproduce the earlier problems.

May also help with the Arch packaging for #49?

I'll have a look at #45 once the Arch packaging is fixed up.

[ndim]

The cowsay script contains

    if ($real_prefix_dir eq "/usr") {
        $etc_dir = "/etc";
    } else {
        $etc_dir = "$real_prefix_dir/etc";
    }

but the installation Makefile does not reflect that: The Makefile expects you to specify that exception via

make install prefix=/usr sysconfdir=/etc

As there is no "build" step in the Makefile which might substitute those values of /usr and /etc somewhere into the cowsay script (and the cowsay.1 man page) at this time, such code must be maintained separately.

As there are systems which actually use /bin together with /etc and /usr/bin together with /usr/etc, the Makefile requiring you to specify prefix=/usr together with sysconfdir=/etc if you actually want that makes sense (hello *BSD).

The cowsay script ignoring /usr/etc in favour of /etc when it finds itself installed into prefix=/usr makes less sense there.

As a workaround/hotfix/heuristic, in the absense of a way to substitute directories into the cowsay script, for prefix=="/usr", the cowsay script could try ${prefix}/etc first and check whether it exists and contains anything. If yes, it could use ${prefix}/etc, and fall back onto /etc otherwise.

[apjanke]

Oh, shoot. I missed that bit when debugging this, and yeah, special-casing /usr like this seems like a problem.

I don't think this should be breaking current functionality, though, because I think the only thing that $etc_dir is currently used for is for locating the cowpath.d directory which is for specifying supplemental cowfile collections? The main cowfiles distributed as part of the cowsay package itself (in <PREFIX>/share/cows and site-cows) are located independently of the config files in $etc_dir. Yeah?

As a workaround/hotfix/heuristic, in the absense of a way to substitute directories into the cowsay script, for prefix=="/usr", the cowsay script could try ${prefix}/etc first and check whether it exists and contains anything. If yes, it could use ${prefix}/etc, and fall back onto /etc otherwise.

My inclination is similar: treat /etc as a special global fallback location that is respected regardless of installation prefix, and have ${prefix}/etceither supersede that if present, or get merged with /etc, including treating /usr/etc that way when $prefix is /usr. (For cowpath.d in particular, I think merging probably makes sense, because that's intended to be an "additive" mechanism?) Which I think would probably work well with the scenario of running cowsay directly from a cloned repo when doing development, too.

And for the long term, remove the treatment of /usr as a special case, and add that build/munge step for supporting systems which do differentiate "/bin + /etc" vs "/usr/bin + /usr/etc". Make a separate mechanism if we need to differentiate the really special case of "running directly from the dev code/repo instead of an installation location".

I should probably do some reading about what the Linux Filesystem Hierarchy Standard and BSD docs have to say about the particulars here.

[apjanke]

I made an issue for the etc and prefix detection stuff: #55

[apjanke]

Here's a small thing. I don't know if the cowthink.1 man page should actualyl be a symlink.

The asciidoctor action for the make man step produces both cowsay.1 and cowthink.1 files. But cowthink.1 isn't a symlink, or a full man page itself.

[cowsay] $ rm man/*.1
[cowsay] $ ls -l man
total 16
-rw-r--r--@ 1 janke  staff  6063 Aug 19 14:32 cowsay.1.adoc
[cowsay] $ make man
asciidoctor -b manpage man/cowsay.1.adoc
[cowsay] $ ls -l man
total 40
-rw-r--r--  1 janke  staff  7027 Aug 20 11:09 cowsay.1
-rw-r--r--@ 1 janke  staff  6063 Aug 19 14:32 cowsay.1.adoc
-rw-r--r--  1 janke  staff    12 Aug 20 11:09 cowthink.1
[cowsay] $ file man/*
man/cowsay.1:      troff or preprocessor input text, ASCII text, with very long lines (609)
man/cowsay.1.adoc: ASCII text, with very long lines (583)
man/cowthink.1:    troff or preprocessor input text, ASCII text, with no line terminators
[cowsay] $ cat man/cowthink.1
.so cowsay.1[cowsay] $

That .so cowsay.1 looks like a man-specific reference that kind of acts like a symlink, but isn't one. Maybe it should just be installed as is? That might support some man reference-following behavior of some sort (though I don't know what it is; in my basic testing it seems to have the same behavior as a symlink). And it would reduce the differences between the in-repo and the installed-to-prefix files and behavior, which I think is generally a good thing.

I think these are man page "aliases", and this is an intentional behavior of asciidoc/asciidoctor, and should maybe stick with it. See:

• Multiple man page names don't work (title & name reversed?) asciidoctor/asciidoctor#2543
• Shadow man pages not working properly asciidoctor/asciidoctor#1811
• https://unix.stackexchange.com/questions/531213/man-page-aliases
• https://docs.asciidoctor.org/asciidoctor/latest/manpage-backend/

From the latter:

From https://unix.stackexchange.com/questions/531213/man-page-aliases, it looks like the special alias form is a man/groff macro that has it read in the main file.

I'm leaning toward sticking with the alias man page instead of a symlink, since that seems more in line with what the toolchain does.

[ndim]

So I run "make man" to rebuild the man page files

8141  man/cowsay.1
  12  man/cowthink.1

with the latter file containing

.so cowsay.1

Now I run man man/cowsay.1 or man ./man/cowsay.1 and it shows me a man page with a build date (bottom center) of 2024-08-21 which is expected. Then I run man man/cowthink.1 or man ./man/cowthink.1 and it shows me a man page with a build date (bottom center) of 2024-08-08 which is the same as the system-wide man cowsay or man cowthink.

If I cd man and repeat the experiment with man ./cowsay.1 and man ./cowthink.1, both show the 2024-08-21 date.

This shows that either the exact .so command asciidoc generates is buggy, or that how man handles a .so command is buggy. Therefore I would prefer to not use .so wherever possible and use symlinks instead.

Using a symlink on make install is easy (which is why I had it in the Makefile), while replacing the .so cowsay.1 file asciidoctor generates with a symlink is a bit more complex shell commands in the make recipe. Nothing out of this world, but non-trivial.

[apjanke]

I think that what's happening there is that the .so cowsay.1 directive is a reference to a man page by name and manual section, and man looks those up along your manpath, which acts as a database of topics in a flat namespace, like a command search path. But it doesn't seem to work well in many cases.

If I move the cowsay man pages into a man/man1 subdirectory in the repo, and get that man on the manpath by setting $PATH or $MANPATH, then it picks up the local man page for me. Or rather, on some systems, it picks up one of the local in-repo manpages, but maybe not the one I expect, depending on how I call it.

On Debian bookworm:

janke@book-test-as:~/repos/cowsay$ PATH=$PWD/path:$PATH
janke@book-test-as:~/repos/cowsay$ man -a -w cowsay
man: warning: /home/janke/repos/cowsay/man/man1/cowsay.1.adoc: ignoring bogus filename
/home/janke/repos/cowsay/man/man1/cowsay.1
janke@book-test-as:~/repos/cowsay$ man -a -w cowthink
/home/janke/repos/cowsay/man/man1/cowsay.1
janke@book-test-as:~/repos/cowsay$ ls -l man/man1/cowthink.1 
-rw-r--r-- 1 janke janke 12 Aug 21 00:34 man/man1/cowthink.1
janke@book-test-as:~/repos/cowsay$ 

Here, both man cowthink and man ./man/man1/cowthink.1 show me the cowsay man page, with a date indicating it's the local one. But based on the man -w output, I suspect while the latter is reading that actual file, the former is noticing the multiple command names in the cowsay.1 page, and going directly to that one. Looks like same behavior on Ubuntu, no surprise.

I'll clean up that "ignoring bogus filename" warning.

Similar behavior on Fedora.

janke@fed39-test-as:~/repos/cowsay$ PATH=$PWD/bin:$PATH
janke@fed39-test-as:~/repos/cowsay$ which cowsay
~/repos/cowsay/bin/cowsay
janke@fed39-test-as:~/repos/cowsay$ man -a -w cowsay
man: warning: /home/janke/repos/cowsay/man/man1/cowsay.1.adoc: ignoring bogus filename
/home/janke/repos/cowsay/man/man1/cowsay.1
janke@fed39-test-as:~/repos/cowsay$ man -a -w cowthink
/home/janke/repos/cowsay/man/man1/cowsay.1
janke@fed39-test-as:~/repos/cowsay$

And on the installed packages using the old symlink approach, both entries resolve to cowsay. But I can't tell if that's because man is symlink-aware and following it, or just seeing the "cowthink" name in the "cowsay.1" entry, and using that.

janke@fed39-test-as:~$ which cowsay
/usr/bin/cowsay
janke@fed39-test-as:~$ man -a -w cowsay
/usr/share/man/man1/cowsay.1.gz
janke@fed39-test-as:~$ man -a -w cowthink
/usr/share/man/man1/cowsay.1.gz
janke@fed39-test-as:~$ 

In Debian, if I remove the symlink entry, then it no longer resolves man cowthink, so I think it's following the symlink.

root@trix-test-as:/usr/share/man/man6# rm cowthink.6.gz

janke@trix-test-as:~$ man -a -w cowsay
/usr/share/man/man6/cowsay.6.gz
janke@trix-test-as:~$ man -a -w cowthink
No manual entry for cowthink

Meanwhile, over on macOS 14.6 (a BSD system), it doesn't seem to recognize this .so-using cowthink.1 as a man database entry at all, though it'll display the man page when I point it at the file directly.

[cowsay] $ PATH=$PWD/bin:$PATH
[cowsay] $ which cowsay
/Users/janke/repos/cowsay/bin/cowsay
[cowsay] $ man -a -w cowsay
/Users/janke/repos/cowsay/man/man1/cowsay.1
[cowsay] $ man -a -w cowthink
No manual entry for cowthink
[cowsay] $ man ./man/man1/cowthink.1.    # (this shows me the local cowsay/cowthink man page)
[cowsay] $

And if I install from main using Homebrew, man cowthink doesn't work.

[~] $ man -a -w cowsay
/opt/homebrew/share/man/man1/cowsay.1
[~] $ man -a -w cowthink
No manual entry for cowthink
[~] $

Over on FreeBSD, man cowthink doesn't find a page, and the .so-using cowthink.1 doesn't display anything even if you give it the file path. No error message either; I'm guessing

That all suggests to me that the symlink is indeed the better-working approach. I'll put the symlink back for 3.8.3, and talk to the Asciidoctor folks about this. And to get the in-repo files more like the as-installed files, I think I'll go the other way around, and have make man replace the .so-using cowthink. with a symlink. Yeah, a little more complex, but I think it's just a couple extra lines in the make man target in the Makefile.

[apjanke]

Switched back to symlink for cowthink.1 in 86fdd9b.

[apjanke]

Say... I gave you a shout-out in the 3.8.3 release notes, since you made a significant contribution here. Is there a particular way you'd like to be referred to in notes like that (i.e., nickname / email / GH username / actually-please-don't-mention-me)? And would you like to be included in the new CONTRIBUTORS.md file?

[ndim]

Say... I gave you a shout-out in the 3.8.3 release notes, since you made a significant contribution here. Is there a particular way you'd like to be referred to in notes like that (i.e., nickname / email / GH username / actually-please-don't-mention-me)? And would you like to be included in the new CONTRIBUTORS.md file?

Ah. Nice. Thank you.

My preferred way to be referenced for notes like that is what my git commit author/committer is: "Hans Ulrich Niedermann hun@n-dimensional.de". Where that is too long, please shorten to "H. U. Niedermann" or "Hans Ulrich" or "ndim", and for the CONTRIBUTORS.md file, you could add "https://n-dimensional.de/".

[apjanke]

Cool. Added you to CONTRIBUTORS.md in a squash commit here: 096afe6.