4 – Makefile doesn't work well for non-default prefix

Bug 4 - Makefile doesn't work well for non-default prefix

Summary: Makefile doesn't work well for non-default prefix

Status:	RESOLVED FIXED

Alias:	None

Product:	abcde
Classification:	Unclassified
Component:	Encoding (show other bugs)
Version:	unspecified
Hardware:	All All

Importance:	Normal normal
Assignee:	Andrew Strong

URL:	http://www.linuxquestions.org/questio...

Depends on:
Blocks:

Reported:	2015-07-05 23:33 BST by Reuben Thomas
Modified:	2015-09-16 08:47 BST (History)
CC List:	1 user (show)

See Also:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Reuben Thomas 2015-07-05 23:33:12 BST

The problem is that etcdir should be ${DESTDIR}${prefix}/etc unless ${prefix} is not given.

See e.g. the autoconf manual for more on this (and all the other paths). I'm not suggesting you use autoconf, mind (though that's what I'd do: the autotools are actually very handy for small projects like this one, precisely because they take care *and hide* of all the tedious details that otherwise outweigh the complexity of the entire build system!).

Tested with current master HEAD.

Comment 1 Steve McIntyre 2015-07-06 21:40:58 BST

Hi Reuben,

Ugh at the thought of using autotools here! :-)

It's possibly the first time I've looked at the Makefile in years.

prefix is clearly broken! If using DESTDIR=/usr/local (for example), the last thing we should be doing is adding /usr/local/usr

Comment 2 Andrew Strong 2015-09-13 02:35:35 BST

Hmmm.... I am a little new to makefiles but so far I have this:

--------------------
INSTALL = /usr/bin/install -c

# Installation directories
prefix = $(DESTDIR)/usr/local
exec_prefix = ${prefix}
datarootdir = $(prefix)/share
mandir = $(datarootdir)/man
man1dir = $(mandir)/man1
bindir = ${exec_prefix}/bin
sysconfdir = $(prefix)/etc
docdir = $(datarootdir)/doc/abcde-2.7.1

all:

clean:

install:
	$(INSTALL) -d -m 755 $(bindir)
	$(INSTALL) -m 755 abcde cddb-tool abcde-musicbrainz-tool $(bindir)
	$(INSTALL) -d -m 755 $(datarootdir)
	$(INSTALL) -d -m 755 $(man1dir)
	$(INSTALL) -m 644 abcde.1 cddb-tool.1 $(man1dir)
	$(INSTALL) -d -m 755 $(sysconfdir)
	$(INSTALL) -m 644 abcde.conf $(sysconfdir)
	$(INSTALL) -d -m 755 $(docdir)
	$(INSTALL) -m 644 changelog COPYING FAQ README $(docdir)

--------------------

which works well enough here. It needs an uninstall target which I will add. It is a little neater and more syntactically correct as per this doc:

https://www.gnu.org/prep/standards/html_node/Directory-Variables.html

but is not a final effort.

Comment 3 Andrew Strong 2015-09-13 04:01:32 BST

And certainly on my system the -c option:

INSTALL = /usr/bin/install -c

is ignored so I have removed it...

Comment 4 Andrew Strong 2015-09-13 04:17:32 BST

And with uninstall:

-----
INSTALL = /usr/bin/install

# Installation directories
prefix = $(DESTDIR)/usr/local
exec_prefix = $(prefix)
datarootdir = $(prefix)/share
mandir = $(datarootdir)/man
man1dir = $(mandir)/man1
bindir = $(exec_prefix)/bin
sysconfdir = $(prefix)/etc
docdir = $(datarootdir)/doc/abcde-2.7.1

all:

clean:

install:
	$(INSTALL) -d -m 755 $(bindir)
	$(INSTALL) -m 755 abcde cddb-tool abcde-musicbrainz-tool $(bindir)
	$(INSTALL) -d -m 755 $(datarootdir)
	$(INSTALL) -d -m 755 $(man1dir)
	$(INSTALL) -m 644 abcde.1 cddb-tool.1 $(man1dir)
	$(INSTALL) -d -m 755 $(sysconfdir)
	$(INSTALL) -m 644 abcde.conf $(sysconfdir)
	$(INSTALL) -d -m 755 $(docdir)
	$(INSTALL) -m 644 changelog COPYING FAQ README $(docdir)
	
uninstall:

	-rm -v $(bindir)/{abcde,cddb-tool,abcde-musicbrainz-tool}
	-rm -v $(man1dir)/{abcde.1,cddb-tool.1}
	-rm -v $(sysconfdir)/abcde.conf
	-rm -v $(docdir)/{changelog,COPYING,FAQ,README}
------------------

I think it is good etiquette to leave directoris in place so this does not remove the docs directory itself but this could easily be altered I guess...

Comment 5 Andrew Strong 2015-09-13 04:36:18 BST

And I have done so:

------------------
INSTALL = /usr/bin/install

# Installation directories
prefix = $(DESTDIR)/usr/local
exec_prefix = $(prefix)
datarootdir = $(prefix)/share
mandir = $(datarootdir)/man
man1dir = $(mandir)/man1
bindir = $(exec_prefix)/bin
sysconfdir = $(prefix)/etc
docdir = $(datarootdir)/doc/abcde-2.7.1

all:

clean:

install:
	$(INSTALL) -d -m 755 $(bindir)
	$(INSTALL) -m 755 abcde cddb-tool abcde-musicbrainz-tool $(bindir)
	$(INSTALL) -d -m 755 $(datarootdir)
	$(INSTALL) -d -m 755 $(man1dir)
	$(INSTALL) -m 644 abcde.1 cddb-tool.1 $(man1dir)
	$(INSTALL) -d -m 755 $(sysconfdir)
	$(INSTALL) -m 644 abcde.conf $(sysconfdir)
	$(INSTALL) -d -m 755 $(docdir)
	$(INSTALL) -m 644 changelog COPYING FAQ README $(docdir)
	
uninstall:

	-rm -v $(bindir)/{abcde,cddb-tool,abcde-musicbrainz-tool}
	-rm -v $(man1dir)/{abcde.1,cddb-tool.1}
	-rm -v $(sysconfdir)/abcde.conf
	-rm -rfv $(docdir)
-------------------

Now out to dinner, let me know if you are both happy with this version...

Comment 6 Reuben Thomas 2015-09-13 14:06:40 BST

This mostly looks good, only $(DESTDIR) should be prefixed to all the paths in install commands rather than part of the definition of prefix, so that prefix and DESTDIR can both be defined by the user and still work.

See the autoconf manual for examples.

Comment 7 Andrew Strong 2015-09-14 10:52:44 BST

Makes sense, although feel free to give a suggested patch for this sort of issue :). My Slackware colleagues have suggested the following with a small additions by myself, please test out and if all is well (and I think it looks pretty decent) I will commit this weekend:

--------------------------
abcde_version = abcde-2.7.1
INSTALL = /usr/bin/install

prefix = /usr/local
exec_prefix = $(prefix)
bindir = $(exec_prefix)/bin
sysconfdir = $(prefix)/etc
datarootdir = $(prefix)/share
docdir = $(datarootdir)/doc/$(abcde_version)
mandir = $(datarootdir)/man
DESTDIR =

all:

clean:

install:
	$(INSTALL) -d -m 755 $(DESTDIR)$(bindir)
	$(INSTALL) -m 755 abcde cddb-tool abcde-musicbrainz-tool $(DESTDIR)$(bindir)
	$(INSTALL) -d -m 755 $(DESTDIR)$(sysconfdir)
	$(INSTALL) -m 644 abcde.conf $(DESTDIR)$(sysconfdir)
	$(INSTALL) -d -m 755 $(DESTDIR)$(docdir)
	$(INSTALL) -m 644 changelog COPYING FAQ README $(DESTDIR)$(docdir)
	$(INSTALL) -d -m 755 $(DESTDIR)$(mandir)/man1
	$(INSTALL) -m 644 abcde.1 cddb-tool.1 $(DESTDIR)$(mandir)/man1

uninstall:

	-rm -v $(DESTDIR)$(bindir)/{abcde,cddb-tool,abcde-musicbrainz-tool}
	-rm -v $(DESTDIR)$(sysconfdir)/abcde.conf
	-rm -v $(DESTDIR)$(docdir)/{changelog,COPYING,FAQ,README}
	-rm -v $(DESTDIR)$(mandir)/man1/{abcde.1,cddb-tool.1}
-----------------------------

(Some did not like the addition of the version information but I have retained this and left it at the top of the makefile for easy editing.) I will add full credit to my fellow Slackers in the changelog.

And then I will return to ripping my cds to alac with abcde + qaac: I sometimes almost forget that I am doing all of this so that I can enjoy music :).

Comment 8 Reuben Thomas 2015-09-14 11:13:18 BST

This looks good, thanks!

Comment 9 Andrew Strong 2015-09-14 19:34:31 BST

Minus the bash-specific brace expansion:

---------------
abcde_version = abcde-2.7.1
INSTALL = /usr/bin/install

prefix = /usr/local
exec_prefix = $(prefix)
bindir = $(exec_prefix)/bin
sysconfdir = $(prefix)/etc
datarootdir = $(prefix)/share
docdir = $(datarootdir)/doc/$(abcde_version)
mandir = $(datarootdir)/man
DESTDIR =

all:

clean:

install:
	$(INSTALL) -d -m 755 $(DESTDIR)$(bindir)
	$(INSTALL) -m 755 abcde cddb-tool abcde-musicbrainz-tool $(DESTDIR)$(bindir)
	$(INSTALL) -d -m 755 $(DESTDIR)$(sysconfdir)
	$(INSTALL) -m 644 abcde.conf $(DESTDIR)$(sysconfdir)
	$(INSTALL) -d -m 755 $(DESTDIR)$(docdir)
	$(INSTALL) -m 644 changelog COPYING FAQ README $(DESTDIR)$(docdir)
	$(INSTALL) -d -m 755 $(DESTDIR)$(mandir)/man1
	$(INSTALL) -m 644 abcde.1 cddb-tool.1 $(DESTDIR)$(mandir)/man1

uninstall:

	-rm -v $(DESTDIR)$(bindir)/abcde \
	       $(DESTDIR)$(bindir)/cddb-tool \
	       $(DESTDIR)$(bindir)/abcde-musicbrainz-tool
	-rm -v $(DESTDIR)$(sysconfdir)/abcde.conf
	-rm -v $(DESTDIR)$(docdir)/changelog \
               $(DESTDIR)$(docdir)/COPYING \
	        $(DESTDIR)$(docdir)/FAQ \
	        $(DESTDIR)$(docdir)/README
	-rm -v $(DESTDIR)$(mandir)/man1/abcde.1 \
	       $(DESTDIR)$(mandir)/man1/cddb-tool.1
---------------------

which is a pity as the brace expansion is cool....

Comment 10 Andrew Strong 2015-09-14 20:59:30 BST

Neater:

abcde_version = abcde-2.7.1
INSTALL = /usr/bin/install

prefix = /usr/local
exec_prefix = $(prefix)
bindir = $(exec_prefix)/bin
sysconfdir = $(prefix)/etc
datarootdir = $(prefix)/share
docdir = $(datarootdir)/doc/$(abcde_version)
mandir = $(datarootdir)/man
DESTDIR =

all:

clean:

install:
	$(INSTALL) -d -m 755 $(DESTDIR)$(bindir)
	$(INSTALL) -m 755 abcde cddb-tool abcde-musicbrainz-tool $(DESTDIR)$(bindir)
	$(INSTALL) -d -m 755 $(DESTDIR)$(sysconfdir)
	$(INSTALL) -m 644 abcde.conf $(DESTDIR)$(sysconfdir)
	$(INSTALL) -d -m 755 $(DESTDIR)$(docdir)
	$(INSTALL) -m 644 changelog COPYING FAQ README $(DESTDIR)$(docdir)
	$(INSTALL) -d -m 755 $(DESTDIR)$(mandir)/man1
	$(INSTALL) -m 644 abcde.1 cddb-tool.1 $(DESTDIR)$(mandir)/man1

uninstall:

	-rm -v \
		$(DESTDIR)$(bindir)/abcde \
		$(DESTDIR)$(bindir)/cddb-tool \
		$(DESTDIR)$(bindir)/abcde-musicbrainz-tool \
		$(DESTDIR)$(sysconfdir)/abcde.conf \
		$(DESTDIR)$(docdir)/changelog \
		$(DESTDIR)$(docdir)/COPYING \
		$(DESTDIR)$(docdir)/FAQ \
		$(DESTDIR)$(docdir)/README \
		$(DESTDIR)$(mandir)/man1/abcde.1 \
		$(DESTDIR)$(mandir)/man1/cddb-tool.1

Comment 11 Reuben Thomas 2015-09-16 00:22:20 BST

I believe the "-c" flag should still be passed to install for old systems. Certainly, automake still outputs this flag, even though it's ignored on GNU and modern BSD systems.

Comment 12 Andrew Strong 2015-09-16 08:47:10 BST

I guess since abcde still supports l3enc there can be no argument with older versions of install :). I have committed the changes, thanks for highlighting this issue and being part of the change...