On 2021-11-07 13:40, Tobias Geerinckx-Rice wrote:
> David,
> 
> Thank you!
> 
> (One fire's out, you'll be glad to hear, so in return here's most of a
> review.  I still reserve the right to tinker with the hunk below —
> you'll see which — later today.)
> 

Thanks for the review, it feels like I learn a lot from this :-)

Not sure if u intended to attach ur modified version of the patch when 
you replied or not. I have however made the changes you suggested in a 
version2 and attached.

[...]

>> +               (display
>> +                (string-append
>> +                 "[[ \"$_BCU_SH_LOADED\" == YES ]] || { \nexport 
>> PATH=\""
>> +                 pathline "${PATH:+:}${PATH}\"\nexport PYTHONPATH=\""
>> +                 pylibsline "${PYTHONPATH:+:}${PYTHONPATH}\"\nexport 
>> GUILE_LOAD_PATH=\""
>> +                 g-bash-lib 
>> "${GUILE_LOAD_PATH:+:}${GUILE_LOAD_PATH}\"\n"
>> +                 ;; XDG_DATA_DIRS needs set for yad to load icons 
>> properly
>> +                 "[[ -e /run/current-system/profile/share ]] && 
>> export XDG_DATA_DIRS="
>> +                 
>> "/run/current-system/profile/share${XDG_DATA_DIRS:+:}${XDG_DATA_DIRS}\n"
>> +                 ;; Ensure that the setuid version of ping is used
>> +                 "[[ -e /run/setuid-programs/ping ]] && "
>> +                 "ping(){ /run/setuid-programs/ping \"$@\" ; } && 
>> export -f ping\n"
>> +                 "[[ -e /run/setuid-programs/ping6 ]] && "
>> +                 "ping6(){ /run/setuid-programs/ping6 \"$@\" ; } && 
>> export -f ping6\n}\n")
>> +                bcu-port)
>> +               (display (string-append "source " libexec "/bcu.sh\n") 
>> bcu-port)
>> +               (close-port bcu-port)
> 
> [So this is the part I was waiting to finish :-)  I still don't have 
> time now.]
> 
> I really want to rewrite this whole block, but for now I just have 1
> question: why not simply append /run/setuid-programs to the start of
> $PATH here?  What's the difference, if any, and do we care?

Basically just to make minimal disruption of the the current environment 
when you source bcu.sh. Almost certainly a non-issue 99.9% of cases, 
i.e. I think that just prepending /run/setuid-programs to PATH would be 
fine as well.

>> +      (license license:gpl3))))
> 
> Why not lgpl3+?  I can't find the gpl3-only file(s).

I really don't know enough about licensing to answer this very well 
unfortunately.. bash-coding-utils is to a large extent a remix of 
stackoverflow contributions (CC-BY-SA-4.0) and other small libraries, 
some of which have GPLv3+ stated at the top (e.g. src/csv/csv/csv.scm). 
I want to apply whatever license must be applied based on the included 
software and SO snippets. If it needs to be lgpl3+ or GPLv3 in the end 
does not matter much to me (Im the author of bash-coding-utils).

Best regards,
David