[apparmor] [patch] fun with the toplevel Makefile

[Christian Boltz]

Hello,

Am Freitag, 23. Januar 2015 schrieb Steve Beattie:
> On Fri, Jan 16, 2015 at 06:23:29PM +0100, Christian Boltz wrote:

> So, a bit of history. When we originally organized things, it was
> with the intention that individual subdirectories (parser, profiles,
> utils, etc.) would get individual releases so that they would not
> be tightly coupled and not require updating all pieces to get, for
> example, an update to one of the shipped profiles.

Yes, I remember - even if it was before I became maintainer of the 
openSUSE AppArmor package.

> > - oh, and all this means "make rpm" is broken ;-)
> 
> Yes.

> > It does _not_ fix "make rpm" target. With *.spec moved to
> > deprecated/, we should probably just drop that part from the
> > Makefile. (Volunteers welcome - I'd have to read through the
> > Makefile to find out if there are additional sections that become
> > superfluous when deleting the rpm target.)
> Yeah, I'm hoping to get around to it before we release 2.10/3.0.
> 
> Another result of moving to the single tarball release process is that
> the funny game we play to symlink in the common/ directory so that
> the bits in Make.rules and Make-po.rules were available when building
> from the individual released tarballs, is no longer necessary.

Ah, that's the reason for the "common" symlink? Good to know (and a 
totally valid reason while building separate tarballs to avoid out-of-
tarball includes in the Makefile).

> The following is patch that gets rid of the symlink make trickery and
> just includes things via relative paths.
> 
> Signed-off-by: Steve Beattie <steve at nxnw.org>
> ---
>  changehat/mod_apparmor/Makefile               |   10 ++--------
>  changehat/pam_apparmor/Makefile               |   13 +++----------
>  changehat/tomcat_apparmor/tomcat_5_0/Makefile |   10 ++--------
>  changehat/tomcat_apparmor/tomcat_5_5/Makefile |   10 ++--------
>  common/Make.rules                             |   10 +++++-----
>  deprecated/utils/Makefile                     |    9 +--------
>  parser/Makefile                               |   11 +++--------
>  parser/po/Makefile                            |    5 ++---
>  profiles/Makefile                             |   12 +++---------
>  utils/Makefile                                |   10 +---------
>  utils/po/Makefile                             |    5 ++---
>  utils/test/Makefile                           |   10 ++--------
>  utils/vim/Makefile                            |   10 ++--------
>  13 files changed, 30 insertions(+), 95 deletions(-)
> 
> Index: b/common/Make.rules
> ===================================================================
> --- a/common/Make.rules
> +++ b/common/Make.rules
> @@ -19,13 +19,11 @@
>  # product.
>  #
>  # NOTES:
> -#   - must define the package NAME before including this file.
> -#   - After checking in to cvs, you'll need to delele the hardlinked
> -#     Make.rules files that already exist in the individual
> application -#     directories
> +#   - must define COMMONDIR (the location of the common/ directory)
> +#     before including this file.

While on it, please also add a comment saying something like

# - make sure to have the first (default) target defined before 
#   including this file

With my patch applied, we at least have a check that avoids funny 
behaviour, but it's better to document it.

[...]
>  .PHONY: _clean
> +ifndef VERBOSE
>  .SILENT: _clean
> +endif

No objections, but please mention this (unrelated) change in the commit 
message.

>  _clean:
>  	-[ -z "${NAME}" ] || rm -f ${NAME}-${VERSION}-*.tar.gz

This rm looks like a relict from the separate tarballs - it would delete 
apparmor-2.9.1-*.tar.gz, but the tarball name is just 
apparmor-2.9.1.tar.gz (without the -*).

Shorter: please drop that line.

>  	-rm -f ${MANPAGES} *.[0-9].gz ${HTMLMANPAGES} pod2htm*.tmp

With only this line remaining for _clean, I wonder if we should
a) rename the target to "clean_pod" or
b) just copy that line into every Makefile that needs it and totally 
   drop _clean (code duplication vs. having to check another file for 
   one line - choose your poison ;-)
but that's something for a separate patch.

Speaking about it, we should also
- drop _clean from utils/test/Makefile which doesn't need it
- add it for libraries/libapparmor/doc where it's missing ;-)
  (hidden from bzr status - try bzr ignored...)

> Index: b/parser/Makefile
> ===================================================================
> --- a/parser/Makefile
> +++ b/parser/Makefile
...
> +ifndef VERBOSE
>  .SILENT: clean
> +endif

Again no objections, but please mention this unrelated change in the 
commit message.

> Index: b/utils/test/Makefile
> ===================================================================
> --- a/utils/test/Makefile
> +++ b/utils/test/Makefile
> @@ -37,7 +31,7 @@ ifndef VERBOSE
>  endif
> 
>  clean: _clean

Calling _clean is not needed here since the tests don't contain any 
manpages ;-)

> Index: b/profiles/Makefile
> ===================================================================
> --- a/profiles/Makefile
> +++ b/profiles/Makefile
...
> @@ -63,7 +57,7 @@ install: local
>  LOCAL_ADDITIONS=$(filter-out ${PROFILES_SOURCE}/local/README,
> $(wildcard ${PROFILES_SOURCE}/local/*)) .
> PHONY: clean
>  clean:
> -	-rm -f $(NAME)-$(VERSION)*.tar.gz Make.rules ${LOCAL_ADDITIONS}
> common 
> +	-rm -f $(NAME)-$(VERSION)*.tar.gz ${LOCAL_ADDITIONS}

No need to delete a ghost (the no longer generated tarball) - please 
remove the tar.gz part of the line.

> Index: b/changehat/pam_apparmor/Makefile
> ===================================================================
> --- a/changehat/pam_apparmor/Makefile
> +++ b/changehat/pam_apparmor/Makefile
...
> -clean: ${MAKE_RULES}
> +clean:
>  	rm -f core core.* *.so *.o *.s *.a *~
> -	rm -f ${NAME}-*.tar.gz Make.rules
> +	rm -f ${NAME}-*.tar.gz

Another ghost tarball we no longer need to delete.

> Index: b/changehat/tomcat_apparmor/tomcat_5_0/Makefile
> ===================================================================
> --- a/changehat/tomcat_apparmor/tomcat_5_0/Makefile
> +++ b/changehat/tomcat_apparmor/tomcat_5_0/Makefile
...
>  clean:
>  	ant clean
> -	rm -f tomcat_apparmor.spec ${NAME}-*.tar.gz Make.rules
> +	rm -f tomcat_apparmor.spec ${NAME}-*.tar.gz

One more ghost.tar.gz to remove from the rm command.

Besides that, I'd guess that you can delete the whole line - I doubt 
something will generate tomcat_apparmor.spec out of nothing ;-)

(Please add something like "remove outdated tarball and spec deletions 
from 'clean' target" to the commit message ;-)

> Index: b/changehat/tomcat_apparmor/tomcat_5_5/Makefile
> ===================================================================
> --- a/changehat/tomcat_apparmor/tomcat_5_5/Makefile
> +++ b/changehat/tomcat_apparmor/tomcat_5_5/Makefile
...
>  clean:
>  	ant clean
> -	rm -f tomcat_apparmor.spec ${NAME}-*.tar.gz Make.rules
> +	rm -f tomcat_apparmor.spec ${NAME}-*.tar.gz

The comments for tomcat_5_0 also apply here.


I'd also propose to move this part of your "drop make rpm" patch here 
because it fits better:

Index: b/Makefile
===================================================================
--- a/Makefile
+++ b/Makefile
@@ -1,8 +1,7 @@
 #
 #
-OVERRIDE_TARBALL=yes
-
-include common/Make.rules
+COMMONDIR=common
+include ${COMMONDIR}/Make.rules
 
 DIRS=parser \
      profiles \



With the above changes applied and all those little ghosts chased away,
Acked-by: Christian "Ghostbuster" Boltz <apparmor at cboltz.de>


Regards,

Christian Boltz
-- 
And if the majority here feels mlmmj should respond in Klingon,
that's what we should consider.  As long as it uses proper MIME
headers, of course. ;-)    [Gerald Pfeifer in opensuse-project]