[pacman-dev] [PATCH 0/5] Splitting up makepkg into libmakepkg

[Dave Reisner]

On Tue, Sep 03, 2013 at 03:40:24PM +1000, Allan McRae wrote:
> On 02/09/13 01:40, Ashley Whetter wrote:
> > On 2013-08-31 07:26, Allan McRae wrote:
> >> On 25/08/13 22:14, awhetter.2011 at my.bristol.ac.uk wrote:
> >>> From: Ashley Whetter <awhetter.2011 at my.bristol.ac.uk>
> >>>
> >>> In preparation for creating a makepkg test suite I've started
> >>> splitting makepkg
> >>> up into libmakepkg.
> >>> Currently I've only split out the downloading and extracting functions.
> >>> Everything else has gone into utils.
> >>>
> >>> I decided to keep get_url and get_downloadclient out of the downloads
> >>> library
> >>> because they depend on the format of PKGBUILDs.
> >>> So the new dependency graph looks like this:
> >>> http://files.awhetter.co.uk/permanent/makepkg_resolved_deps_v2.png
> >>> Whereas the old one looked like this:
> >>> http://files.awhetter.co.uk/permanent/makepkg_resolved_deps.png
> >>>
> >>> Allan McRae (2):
> >>>   makepkg: run locally with libtool style wrapper
> >>>   makepkg: add LIBRARY variable
> >>>
> >>> Ashley Whetter (3):
> >>>   Moved makepkg functions into a library
> >>>   Moved makepkg download functions into libmakepkg
> >>>   Moved makepkg extraction functions into libmakepkg
> >>>
> >>
> >> Given the third patch is too big to get to the mailing list (and
> >> figuring out how to approve things is too much effort for me), can you
> >> push your git repo somwhere we can have a look at things?
> >>
> >> Allan
> > 
> > Sure. You can see the changes here:
> > https://github.com/AWhetter/pacman/compare/makepkg-tests
> > 
> 
> Well...   I can see why it was rejected as too big!
> 
> I'm going to assume the first two patches are awesome (given I wrote
> them...) and focus on your patches.
> 
> Here are my opinions - I'd appreciated other comments on this too.
> 
> 1) I do not like moving everything out of makepkg immediately and the
> moving it again.   I'd much prefer to just move out sections at a time.
> So it would be great to just focus on the download functions first and
> once we get that sorted and committed move on to other sections.

Agreed.

> 2) At the end of this patch series we have ended up with this in makepkg:
> 
>  source $LIBRARY/downloads.sh
>  source $LIBRARY/extractions.sh
>  source $LIBRARY/utils.sh
> 
> I think it would be best to have a file $LIBRARY/source.sh that is
> sourced in makepkg and its role it to source all the components of the
> library.

I'm not sure how much value there is in doing this. If the idea is to
have a "toplevel" file for each directory of sublibs, then it seems like
source.sh doesn't seem like it would do much more than:

  for lib in "$LIBRARY"/*.sh; do
    . "$lib"
  done

Do you forsee anything more involved that this intermediate file would
need/want to do that would make not inlining it meaningful?

> 3) Focusing just on the downloads split, how split should we go?  It has
> download_sources which calls download_{local,file,"vcs"} as needed.  I
> was thinking:
> 
> $LIBRARY/download.sh
>         /download/local.sh
>                  /file.sh
>                  /git.sh
>                  /...
> 
> The download.sh file would source all the files in the download
> directory and carry the download_sources function.
> 
> @Dave: Your input would be particularly helpful here.

Well, it *seems* reasonable. My only concern is how to handle common
code (get_url, get_filename, get_filepath) which is called in the
individual implementations. These fragments would be incomplete if
sourced on their own -- this IMO diminishes the value of separating them
out.

Maybe-crazy idea: create a function that wraps the 'source' builtin.
Perhaps it would look like:

  makepkg_load_module() {
    local module FUNCNEST=10
    if [[ -z ${MAKEPKG_LOADED_MODULES["$1"]} ]]; then
      unset -v MAKEPKG_MODULE_DEPENDENCIES
      . "$LIBRARY/$1"
      MAKEPKG_LOADED_MODULES["$1"]=loaded
      for module in "${MAKEPKG_MODULE_DEPENDENCIES[@]}"; do
        makepkg_load_modules "$module"
      fi
    fi
  }

MAKEPKG_LOADED_MODULES would be some associative array we declare before
calling this function. modules can declare dependencies which is an
array of module names it depends on.

I'm not sure we want to go down this road, but I'm mentioning it
anyways. It would certainly, at a minimum, make testing individual
modules more straightforward.

> 4) Copyright years in new file.   I guess we should a git blame on
> makepkg and have the copyright years of the new files cover the range of
> the commits to those files.  We also need to be careful about the other
> people listed at the top of the makepkg file, but I guess there is
> almost zero contribution left by them in makepkg these days.
> 
> Allan
>