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

[Richard Hansen <rhansen%bbn.com@localhost>]

On 2014-05-06 16:21, Aleksej Saushev wrote:
>> 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?

I did this because a package is allowed to both download a tarball and
check out sources from a repository.  For example, a hypothetical
package foo might do the following:

  1. download foo-1.2.3.tar.gz
  2. extract foo-1.2.3.tar.gz to ${WRKDIR}/foo-1.2.3
  3. clone url://to/bar.git into ${WRKDIR}/bar
  4. build and install the sources in ${WRKDIR}/foo-1.2.3
  5. install ${WRKDIR}/bar/nifty-script to /usr/bin

If I change the default value of WRKSRC, then this package would try to
build the sources in ${WRKDIR}/bar instead of ${WRKDIR}/foo-1.2.3.

Maybe it's OK to require a package developers to manually reset
WRKDIR=${WRKSRC}/${DISTNAME} if they both download a tarball and check
out a repository.  If so, I think this would do what we want:

.if ${CVS_REPOSITORIES:[\#]} == 1
WRKSRC?=        ${WRKDIR}/${CVS_MODULE.${CVS_REPOSITORIES:[1]}}
.endif


BTW, I just noticed that there is a (minor) bug in my patches:  If a
package checks out two different types of repositories (e.g., it both
clones a Git repo and checks out a CVS module) and ${WRKSRC} doesn't
exist by post-extract, then it's a race to determine which symlink will
win.  I'm not sure this is worth fixing; the package developer should
set WRKSRC in this case anyway.

> 
>> 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.

Broken how?  On which 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.

Yeah, I was debating which way was better.  Because you brought it up I
think I'll add '-d ${CVS_MODULE.${repo}:Q}' for consistency (even though
it's redundant).

> 
>> --- 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.

How so?  As far as I can tell everything here is sufficiently quoted.

Note:  POSIX shells do not subject variable assignments to word
splitting.  Thus, in the following script, quotes are unnecessary around
$1 but are necessary around $2 (because bar="$2" is not a variable
assignment; it's an argument to the export command):

   foo=$1
   export bar="$2"

> 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,

If they were inlined it would be easier to read.  :)

> I suggest that you introduce
> variables in make rather than in sh.

This defeats the purpose -- the make variables and their expansion
modifiers would be very long, e.g. ${_CVS_DISTFILE.${repo}:Q} and
${_CVS_FULLTARBALL.${repo}:Q} instead of $$tarball and $$fulltarball.

I can scrap this commit, though I prefer not to; I hate working with
code that has lines longer than 80 characters.

> 
>> -            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".

That's unfortunate.  I'll switch it back and add comments to scare off
future attempts to change it.

> 
>> +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.

Note that cvs-package.mk already does:

pre-extract: do-cvs-extract
do-cvs-extract: .PHONY
        ...

I would suspect that any argument against do-cvs-post-extract also
applies to do-cvs-extract.  Given that do-cvs-extract seems to work OK,
I don't see a problem with do-cvs-post-extract.

Also note that do-cvs-post-extract is a prerequisite to post-extract, so
it is guaranteed to run before the user-definable post-extract rule.

> 
>> -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.

It's not documented -- the algorithm is only in source code.  Maybe I'll
scrap this commit to be consistent with the others.

-Richard