Re: bsd.port.mk: Simplify show(-fake)-size, run as _pbuild

On Wed, May 23 2018, Marc Espie <espie@nerim.net> wrote:
> On Tue, May 22, 2018 at 11:50:56PM +0200, Klemens Nanni wrote:
>> On Mon, May 21, 2018 at 07:22:13PM +0200, Marc Espie wrote:
>> > > @@ -3434,17 +3434,17 @@ show:
>> > > # du fails if it can't access everything
>> > > show-size:
>> > > @if du -ks ${WRKDIR} 2>/dev/null >${WRKDIR}/wrkdir-size; then \
>> > > - cat ${WRKDIR}/wrkdir-size && rm -f ${WRKDIR}/wrkdir-size; \
>> > > + cat ${WRKDIR}/wrkdir-size && ${_PBUILD} rm -f ${WRKDIR}/wrkdir-size; \
>> > > else \
>> > > - chmod -R u+rX ${WRKDIR}; \
>> > > + ${_PBUILD} chmod -R u+rX ${WRKDIR}; \
>> > > du -ks ${WRKDIR}; \
>> > > fi
>> > >
>> > > show-fake-size:
>> > > @if du -ks ${WRKINST} 2>/dev/null >${WRKINST}/wrkdir-size; then \
>> > > - cat ${WRKINST}/wrkdir-size && rm -f ${WRKINST}/wrkdir-size; \
>> > > + cat ${WRKINST}/wrkdir-size && ${_PBUILD} rm -f ${WRKINST}/wrkdir-size; \
>> > > else \
>> > > - chmod -R u+rX ${WRKINST}; \
>> > > + ${_PBUILD} chmod -R u+rX ${WRKINST}; \
>> > > du -ks ${WRKINST}; \
>> > > fi
>> > >
>> > Those should definitely be done differently... Namely adding a file under
>> > WRKDIR seems like overkill, stuff could go to a tmp file
>> > and also it's probably more useful to always go _PBUILD for the du instead of
>> > chmod'ing.
>> Here's a diff that just runs `du' as _pbuild which may fail and show
>> inaccessible files as are.
>>
>> For bad permissions in WRKDIR, espie (recently) introduced
>> FIX_EXTRACT_PERMISSIONS[0], which already does
>>
>> 2505 # run as _pbuild
>> 2506 _post-extract-finalize:
>> ...
>> 2512 .if ${FIX_EXTRACT_PERMISSIONS:L} == "yes"
>> 2513 › @chmod -R a+rX ${WRKDIR}
>>
>>
>> and for bad permissions in WRKINST, there's
>>
>> 2789 ${_FAKE_COOKIE}: ${_BUILD_COOKIE} ${_FAKESUDO_CHECK_COOKIE}
>> ...
>> 2802 .if ${FAKE_AS_ROOT:L} != "yes"
>> 2803 › @${_FAKESUDO} chmod -R a+rX ${WRKINST}
>>
>>
>> Thus, by the time `show-size' or `show-fake-size' runs,
>> `_post-extract-finalize' or `${_FAKE_COOKIE}' respectively will have
>> adjusted permissions already.
>>
>> Scenarios where permissions change in between running these targets are
>> not covered, but `show(-fake)-size' is the wrong place to take care of
>> this anyway imho.
>>
>> With this in mind, I don't see why we should still bother with stashing
>> the output and rerunning `du'.
>>
>> Feedback?
>>
>> 0: https://marc.info/?l=openbsd-ports&m=151266048117858&w=2
>>
>> Index: bsd.port.mk
>> ===================================================================
>> RCS file: /cvs/ports/infrastructure/mk/bsd.port.mk,v
>> retrieving revision 1.1403
>> diff -u -p -r1.1403 bsd.port.mk
>> --- bsd.port.mk 21 May 2018 21:26:39 -0000 1.1403
>> +++ bsd.port.mk 22 May 2018 20:32:11 -0000
>> @@ -3430,23 +3430,12 @@ show:
>> @echo ${${_s}:Q}
>> .endfor
>>
>> -# avoid sudo and avoid modifying dir (if possible):
>> # du fails if it can't access everything
>> show-size:
>> - @if du -ks ${WRKDIR} 2>/dev/null >${WRKDIR}/wrkdir-size; then \
>> - cat ${WRKDIR}/wrkdir-size && rm -f ${WRKDIR}/wrkdir-size; \
>> - else \
>> - chmod -R u+rX ${WRKDIR}; \
>> - du -ks ${WRKDIR}; \
>> - fi
>> + @${_PBUILD} du -ks ${WRKDIR}
>>
>> show-fake-size:
>> - @if du -ks ${WRKINST} 2>/dev/null >${WRKINST}/wrkdir-size; then \
>> - cat ${WRKINST}/wrkdir-size && rm -f ${WRKINST}/wrkdir-size; \
>> - else \
>> - chmod -R u+rX ${WRKINST}; \
>> - du -ks ${WRKINST}; \
>> - fi
>> + @${_PBUILD} du -ks ${WRKINST}
>>
>> verbose-show:
>> .for _s in ${verbose-show}
>
> Well, I think you make sense.

Same here.

> Better than the old code anyway.
>
> Okay

ok jca@

--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE