Re: wip/mk/git-package.mk cleanups

[Aleksej Saushev <asau%inbox.ru@localhost>]

  Hello!

Richard Hansen <rhansen%bbn.com@localhost> writes:

> Attached is a re-roll (v2) of the patches.  Changes from v1:
>
>   * create NO_CVS_CACHE, NO_GIT_CACHE, and NO_HG_CACHE for consistency
>     with NO_SVN_CACHE (so users can easily disable caching)
>
>   * create NO_{CVS,GIT,HG,SVN}_CACHE.${id} so that packages can
>     disable caching
>
>   * don't inline _GIT_EXTRACT_CACHED.${repo} and
>     _GIT_CREATE_CACHE.${repo} in the do-git-extract target
>
>   * extend the git-package.mk changes to the other <vc>-package.mk
>     files (where applicable) to keep them consistent
>
>   * fix the CVS checkout directory (it was checking out to ${repo}
>     instead of ${CVS_MODULE.${repo}}, although these two are the same
>     for all buildable packages anyway so it wasn't actually causing any
>     problems)
>
>   * other minor cleanups

>>>> +#         If ${WRKSRC} doesn't exist by the time post-extract is
>>>> +#         run, ${WRKSRC} will be created as a symlink pointing
>>>> +#         to ${GIT_MODULE.${GIT_REPOSITORIES:[1]}}.
>>>> +#
>>>>
>>>> I'm afraid, it doesn't clean anything up,
>>>
>>> Sure it does:  It eliminates the need to set
>>> WRKSRC=${WRKDIR}/${GIT_MODULE.foo} in the package Makefile.
>> 
>> Symlinks don't clean anything up, they complicate things rather,
>> even though it isn't clear from experience on platforms like NetBSD.
>> In particular, some FreeBSD tools (e.g. pax that is actively promoted by
>> POSIX and pkgsrc) behave subtly differently in presense of symlinks.
>> Please, don't introduce them here. At least not until consequences are
>> well understood.
>
> I believe that the risks associated with creating the symlinks are lower
> than the risks associated with overriding WRKSRC in every package that
> uses {cvs,git,hg,svn}-package.mk.  Note that the symlink and its
> referent are siblings in the filesystem hierarchy, so any relative paths
> spanning both sides of WRKSRC shouldn't be a problem.

So far the only reason for symlink is setting WRKSRC.
Why don't you want to provide default value to it instead?

> If a symlink causes a problem in a particular package, the package
> maintainer can still set WRKSRC (as required without these patches) to
> prevent the symlink from being created.
>
> I'd like to hear more from the community about this issue (especially
> alternative suggestions) before I exclude the symlink feature.

It's a hack that is potentially broken in general on at least one major 
platform.
We don't do such a thing in general case.
As for me, these are enough reasons not to do this.

> @@ -214,7 +216,7 @@ do-cvs-extract: .PHONY
>       ${SETENV} ${_CVS_ENV}                                           \
>         ${_CVS_CMD} ${_CVS_FLAGS} -d ${CVS_ROOT.${repo}:Q}            \
>           checkout ${_CVS_CHECKOUT_FLAGS} ${_CVS_TAG_FLAG.${repo}}    \
> -           -d ${repo} ${CVS_MODULE.${repo}:Q};                               
> \
> +           ${CVS_MODULE.${repo}:Q};                                  \
>       ${_CVS_CREATE_CACHE.${repo}}
>  .endfor

This goes against your style of providing explicit destination directory,
but that's alright to me.

> --- a/mk/cvs-package.mk
> +++ b/mk/cvs-package.mk
> @@ -178,17 +178,19 @@ _CVS_DISTFILE.${repo}=  
> ${PKGBASE}-${CVS_MODULE.${repo}}-${_CVS_TAG.${repo}}.tar.
>  
>  #   command to extract cache file
>  _CVS_EXTRACT_CACHED.${repo}= \
> -     if [ -f ${_CVS_DISTDIR:Q}/${_CVS_DISTFILE.${repo}:Q} ]; then    \
> -       ${STEP_MSG} "Extracting cached CVS archive 
> "${_CVS_DISTFILE.${repo}:Q}"."; \
> -       gzip -d -c ${_CVS_DISTDIR:Q}/${_CVS_DISTFILE.${repo}:Q} | pax -r;     
> \
> -       exit 0;                                                       \
> +     tarball=${_CVS_DISTFILE.${repo}:Q};                             \
> +     fulltarball=${_CVS_DISTDIR:Q}/$$tarball;                        \
> +     if [ -f "$$fulltarball" ]; then                                 \
> +             ${STEP_MSG} "Extracting cached CVS archive $$tarball."; \
> +             gzip -d -c "$$fulltarball" | pax -r;                    \
> +             exit 0;                                                 \
>       fi

I'm afraid that this change mostly undoes your efforts on proper quoting.
Anyway, I find this quoting wars against make and sh not productive enough.
This is less an issue than what is described below.

>  #   create cache archive
>  _CVS_CREATE_CACHE.${repo}=   \
> -     ${STEP_MSG} "Creating cached CVS archive 
> "${_CVS_DISTFILE.${repo}:Q}"."; \
> +     ${STEP_MSG} "Creating cached CVS archive $$tarball.";           \
>       ${MKDIR} ${_CVS_DISTDIR:Q};                                     \
> -     pax -w ${CVS_MODULE.${repo}:Q} | gzip > 
> ${_CVS_DISTDIR:Q}/${_CVS_DISTFILE.${repo}:Q}
> +     pax -w ${CVS_MODULE.${repo}:Q} | gzip >$$fulltarball
>  .endfor

Because this makes the code hard to track, I suggest that you introduce
variables in make rather than in sh.

> -             gzip -d -c "$$fulltarball" | pax -r;                    \
> +             pax -r -z -f "$$fulltarball";                           \

This is wrong. The code was changed from "pax -zrf" to "gzip -dc | pax -r"
for a reason: not all platforms support "-z".

> +post-extract: do-git-post-extract

...and similar.

Because "post-extract" is user-definable, it needs more consideration.
It may make the code essentially non-deterministic in the sense
that it may break for some but not for others. It is unclear whether
this is theoretical or practical as of now. I'd say that you shouldn't use it.

> -GIT_MODULE.${repo}?= ${repo}
> +# follow the same algorithm that 'git clone' does when "humanizing"
> +# the clone URL.  first, start with the URL
> +_GIT_MODULE_D_0.${repo}=     ${GIT_REPO.${repo}}

I don't like it, but alright. Still, because this code is really unclear
and messy, please, provide a reference to git documentation where this
algorithm is described.

Looks fine with exception of wrong pax usage, symlinks, and shell variables.


-- 
HE CE3OH...