[pacman-dev] [PATCH v3 3/4] Moved dependencies of download functions into a library

[Allan McRae]

On 09/10/13 14:46, Allan McRae wrote:
> On 09/10/13 14:40, Allan McRae wrote:
>> On 22/09/13 20:25, Ashley Whetter wrote:
>>> Moved functions out in preparation for splitting out download functions.
>>> scripts/libmakepkg/*.sh files only import the files from their relevant
>>> directory.
>>> All libmakepkg files have an inclusion guard.
>>> Also added libmakepkg targets to Makefile.am.
>>>
>>> Signed-off-by: Ashley Whetter <ashley at awhetter.co.uk>
>>> ---
>>
>> Patch is fine but copyright years are excessive in some cases.  As far
>> as I can tell, no line has been left untouch by contributors not covered
>> by the "Pacman Development Team" banner (apart form function names,
>> brackets and whitespace.  So we don not need to propegate those to the
>> new files.   Changes I will make while pulling this are noted below.
>>
>>>  scripts/.gitignore                 |   4 +
>>>  scripts/Makefile.am                |  38 +++++++-
>>>  scripts/libmakepkg/util.sh.in      |  30 +++++++
>>>  scripts/libmakepkg/util/message.sh |  55 ++++++++++++
>>>  scripts/libmakepkg/util/url.sh.in  | 145 ++++++++++++++++++++++++++++++
>>>  scripts/libmakepkg/util/util.sh.in |  58 ++++++++++++
>>>  scripts/makepkg.sh.in              | 180 ++-----------------------------------
>>>  7 files changed, 333 insertions(+), 177 deletions(-)
>>>  create mode 100644 scripts/libmakepkg/util.sh.in
>>>  create mode 100644 scripts/libmakepkg/util/message.sh
>>>  create mode 100644 scripts/libmakepkg/util/url.sh.in
>>>  create mode 100644 scripts/libmakepkg/util/util.sh.in
>>>
> 
> Final note.   New files needed to be added to scripts/po/POTFILES.in.
> Done on my working branch.
> 

Final, final note.   There are issues in the Makefile that need fixed
before I can pull this.  Fix that, and address the comments in my
previous replies, and I will merge.


> diff --git a/scripts/Makefile.am b/scripts/Makefile.am
> index 8130704..4c52bd4 100644
> --- a/scripts/Makefile.am
> +++ b/scripts/Makefile.am
> @@ -36,13 +36,22 @@ LIBRARY = \
>  	library/size_to_human.sh \
>  	library/term_colors.sh
>
> +LIBMAKEPKG = \
> +	$(LIBMAKEPKG_INC) \
> +	util/message.sh
> +
> +LIBMAKEPKG_INC = \
> +	util.sh \
> +	util/url.sh \
> +	util/util.sh
> +

message.sh does not have the library include field, but still needs
modified via the $(edit) below.  Join these together.

Also, I guess this needs added to EXTRA_DIST to stop "make dist"
breaking.  Please check.

>  # Files that should be removed, but which Automake does not know.
>  MOSTLYCLEANFILES = $(bin_SCRIPTS)
>
>  libmakepkgdir = $(libdir)/makepkg
>
>  clean-local:
> -	$(AM_V_at)$(RM) -r .lib
> +	$(AM_V_at)$(RM) -r .lib $(addprefix libmakepkg/,$(LIBMAKEPKG_INC))
>
>  if USE_GIT_VERSION
>  GIT_VERSION := $(shell sh -c 'git describe --abbrev=4 --dirty | sed
s/^v//')
> @@ -83,7 +92,20 @@ $(OURSCRIPTS): Makefile
>  	$(AM_V_at)chmod +x,a-w $@
>  	@$(BASH_SHELL) -O extglob -n $@
>
> +$(addprefix libmakepkg/,$(LIBMAKEPKG_INC)): Makefile
> +	$(AM_V_at)$(RM) $@
> +	$(AM_V_GEN)test -f $(srcdir)/$@.in && m4 -P -I $(srcdir)
$(srcdir)/$@.in | $(edit) >$@

Currently, we do not need to run these through m4.  So:

$(AM_V_GEN)test -f $(srcdir)/$@.in && $(edit) $(srcdir)/$@.in >$@

> +	$(AM_V_at)chmod a-w $@
> +	@$(BASH_SHELL) -O extglob -n $@
> +
> +libmakepkg: \
> +	$(srcdir)/libmakepkg/util.sh \
> +	$(srcdir)/libmakepkg/util/message.sh \
> +	$(srcdir)/libmakepkg/util/url.sh \
> +	$(srcdir)/libmakepkg/util/util.sh
> +
>  makepkg: \
> +	libmakepkg \
>  	$(srcdir)/makepkg.sh.in \
>  	$(srcdir)/makepkg-wrapper.sh.in \
>  	$(srcdir)/library/parseopts.sh
> @@ -131,7 +153,12 @@ makepkg-wrapper: \
>  	$(srcdir)/makepkg-wrapper.sh.in \
>  	$(srcdir)/makepkg.sh.in \
>  	$(srcdir)/library/parseopts.sh \
> -	| makepkg
> +	$(srcdir)/libmakepkg/util.sh \
> +	$(srcdir)/libmakepkg/util/message.sh \
> +	$(srcdir)/libmakepkg/util/url.sh \
> +	$(srcdir)/libmakepkg/util/util.sh \
> +	| libmakepkg \
> +	makepkg

You have added libmakepkg as a dependency and an order-only dependency
here.   Only the latter is needed.  And there is no need to add the
files from libmakepkg, as they are covered already.

>  	$(AM_V_at)$(MKDIR_P) .lib
>  	$(AM_V_at)mv -f makepkg .lib
>  	$(AM_V_at)$(RM) $@
> @@ -146,6 +173,10 @@ install-data-hook:
>  	cd $(DESTDIR)$(bindir) && \
>  		$(RM) makepkg makepkg-wrapper
>  	$(INSTALL) .lib/makepkg $(DESTDIR)$(bindir)/makepkg
> +	$(AM_V_at)$(MKDIR_P) $(DESTDIR)$(libmakepkgdir)/util
> +	for lib in $(LIBMAKEPKG); do \
> +		$(INSTALL) libmakepkg/$$lib $(DESTDIR)$(libmakepkgdir)/$$lib; \
> +	done
>  	cd $(DESTDIR)$(bindir) && \
>  		$(RM) repo-elephant && \
>  		( $(LN_S) repo-add repo-elephant || \
> @@ -160,5 +191,8 @@ install-data-hook:
>  uninstall-hook:
>  	cd $(DESTDIR)$(bindir) && \
>  		$(RM) repo-remove repo-elephant
> +	for lib in $(LIBMAKEPKG); do \
> +		$(RM) -r $(DESTDIR)$(libmakepkgdir)/$$lib; \

What is the -r doing.   The directories are not in $(LIBMAKEPKG) so they
will not be removed.

> +	done
>
>  # vim:set ts=2 sw=2 noet: