#110 Fix PKGBUILD for non-i686 builds
Closed by bernies. Opened by bernies.
bernies/abslibre bernie/mplayer-38542-i686  into  pull-requests

Download 110.patch

Non-i686 builds fail with a failure in prepare() because the function returns the result of the test

thanks for noticing - that bug was probably my doing - i usually try i686 last,
and dont always notice if i added a regression until next the release

i will accept the patch but just to explain - dont be alarmed if i re-work
away the exact change for sake of a minimal diff - consider it as a "coding
style" guide for PKGBUILDs - for PKGBUILDs, i tend to prefer minimal diffs - i
propose an alternate fix:

  • [[ "${CARCH}" = "i686" ]] && sed -i '/vf_gradfun.c/d' Makefile
  • [[ "${CARCH}" = "i686" ]] && sed -i '/vf_gradfun.c/d' Makefile || :

the main thing is that your patch relates to 12 LOCs (most due to white-space) -
the change-set above relates to only 2 LOC - 2 LOCs is always easier than 12
to read, understand, or maintain, fewer bytes to store and transmit, and so on

for example, these 3 LOC:

[[ "${CARCH}" = "i686" ]] && do_comething || :
[[ "${CARCH}" = "i686" ]] && do_comething_similar || :
[[ "${CARCH}" = "x86_64" ]] && do_comething || :

or these equivalent 2 LOC:

case "${CARCH}" in i686|x86_64) do_comething ;; esac ;
case "${CARCH}" in i686 ) do_comething_similar ;; esac ;

although somewhat ugly, either of the above make a better patch/PR for PKGBUILDs
than the conventional 6 LOC equivalent:

if [[ "${CARCH}" = "i686" ]]; then
do_comething
do_comething_similar
elif [[ "${CARCH}" = "x86_64" ]]; then
do_comething
fi

FWIW, i never put then on same line with if, so that is 8 LOC vs 2 LOC if
i wrote them - i usually use a case in $CARCH block for larger change-sets -
if that entire block fits a <80 character one-liner, all the better for
grokability and maintainability of the diff IMHO - after-all ideally, most such
LOCs are bugfixes like this issue, to be deleted next release (less is best for
PKGBUILDs)

Thanks for the explanation, my bash scripting skills are very basic, so I appreciate the insights.

I acknowledge the desire to reduce the LOC, however I also feel that readability and maintainability are important to consider. To you the one line change is likely plenty readable, but with my level of experience it is less so. My intent was also to reduce the risk of accidentally affecting the return value of the function.

Regarding then on the same line as if, I have no opinion on this, but followed the style elsewhere in the same file (in the build() function, around line 74).

Nevertheless, I respect you as a significantly more active developer than I (and I am very thankful for your efforts here), so I have no objections to you modifying the changeset as you see fit.

I've created another PR (#112) to cleanly incorporate your feedback above. I elected to use the case instead of the test builtin as it's behaviour is more clear to me, and, should the sed command ever fail, the error should be thrown out of the prepare() function instead of being swallowed by || :. I think this is a desirable feature.

Pull-Request has been closed by bernies

Metadata