[bug#49238] [PATCH v1 2/2] gnu: Add ytfzf.

[Maxime Devos]

Raghav Gururajan via Guix-patches via schreef op zo 27-06-2021 om 01:47 [-0400]:
> +       (patches
> +        (search-patches
> +         ;; Prerequisite for 'patch phase.
> +         "ytfzf-programs.patch"

That's a neat trick to avoid fragile (substitute* ...).
However, as I understand it, the origin is meant to be
usable as source code (think install dependencies && guix build && tar xzf 
stuff.tgz
&& ./configure && make && try it out), such that
"guix build --source=transitive stuff" would give all the
source code for building stuff (in the sense of ‘Corresponding Source’
of the GPL), and "guix-mpv", 'guix-jq" ... don't exist anywhere.

I don't know if that has been spelled out somewhere though.

I'd suggest adding "patch" to 'native-inputs', adding the patch
to 'inputs' or 'native-inputs' (doesn't really matter which) and
doing (invoke "patch" OPTIONS "blabla.patch") before the substitute*.

Actually, myself I'm not convinced because you could consider
the package definition itself to be part of the ‘corresponding source’.

> +             (substitute* "ytfzf"
> +               (("guix-catimg")
> +                (format #f "~a/bin/catimg"
> +                        (assoc-ref inputs "catimg")))

I'm wondering if (string-append (assoc-ref inputs "catimg") "/bin/catimg")
would be better, as 'string-append' is less complex than 'format'
and "format" doesn't seem to provide any additional value here.

Now about the patch:

> + ############################
> + #       Help Texts         #
> +@@ -326,8 +326,8 @@ print_info () {
> + }
> + 
> + print_error () {
> +-    [ $ext_menu_notifs -eq 1 ] && notify-send "error" "$*" || printf 
> "\033[31m$*\033[0m" >&2
> +-    [ $ext_menu_notifs -eq 1 ] && notify-send "Check for new versions and 
> report at: https://github.com/pystardust/ytfzf\n"; || printf "Check for new 
> versions and report at: https://github.com/pystardust/ytfzf\n"; >&2
> ++    [ $ext_menu_notifs -eq 1 ] && guix-notify-send "error" "$*" || printf 
> "\033[31m$*\033[0m" >&2
> ++    [ $ext_menu_notifs -eq 1 ] && guix-notify-send "Check for new versions 
> and report at: https://github.com/pystardust/ytfzf\n"; || printf "Check for 
> new versions and report at: https://github.com/pystardust/ytfzf\n"; >&2

Maybe tell people to report issues at bug-guix@gnu.org or #guix? Dunno

> + }
> + 
> + ############################
> +@@ -398,8 +398,8 @@ format_fzf () {
> + format_menu () {
> +     if [ "$is_ext_menu" -eq 0 ]; then
> +             #dep_ck fzf here because it is only necessary to use here
> +-            dep_ck "fzf"
> +-            menu_command='column -t -s "$tab_space" | fzf -m --bind 
> change:top --tabstop=1 --layout=reverse --delimiter="$tab_space" --nth=1,2 
> --expect="$shortcuts" $FZF_DEFAULT_OPTS'
> ++            dep_ck "guix-fzf"
> ++            menu_command='column -t -s "$tab_space" | guix-fzf -m --bind 
> change:top --tabstop=1 --layout=reverse --delimiter="$tab_space" --nth=1,2 
> --expect="$shortcuts" $FZF_DEFAULT_OPTS'
> +             format_fzf

Don't forget to patch 'column'.

> +     else
> +             # Dmenu doesn't render tabs so removing it
> +@@ -462,7 +462,7 @@ WIDTH=$FZF_PREVIEW_COLUMNS
> + HEIGHT=$FZF_PREVIEW_LINES
> + start_ueberzug () {
> +     [ -e $FIFO ] || { mkfifo "$FIFO" || exit 1 ; }
> +-    ueberzug layer --parser json --silent < "$FIFO" &
> ++    guix-ueberzug layer --parser json --silent < "$FIFO" &
> +     exec 3>"$FIFO"
> + }
> + stop_ueberzug () {

Don't forget to patch "mkfifo".

> +@@ -585,17 +585,17 @@ download_thumbnails () {
> +     if [ "$thumbnail_quality" -eq 1 ]; then
> +             image_download () {
> +                     # higher quality images
> +-                    curl -s "$Url" -G --data-urlencode "sqp=" > 
> "$thumb_dir/$Name.png"
> ++                    guix-curl -s "$Url" -G --data-urlencode "sqp=" > 
> "$thumb_dir/$Name.png"
> +             }
> +     else
> +             image_download () {
> +-                    curl -s "$Url"  > "$thumb_dir/$Name.png"
> ++                    guix-curl -s "$Url"  > "$thumb_dir/$Name.png"
> +             }
> +     fi
> + 
> +     print_info "Downloading Thumbnails...\n"
> +     thumb_urls=$(printf "%s" "$*" |\
> +-            jq  -r '.[]|[.thumbs,.videoID]|@tsv' )
> ++            guix-jq  -r '.[]|[.thumbs,.videoID]|@tsv' )
> + 
> +     while IFS=$tab_space read -r Url Name; do
> +         sleep 0.001
> 

Don't forget patching 'sleep'. It is not shell a built-in
(try "type sleep" and "type [" in a terminal").


> +@@ -988,7 +988,7 @@ format_user_selection () {
> +                     11) 
> selected_urls=$selected_urls$new_line'https://www.youtube.com/watch?v='$surl 
> ;;
> +                     34) 
> selected_urls=$selected_urls$new_line'https://www.youtube.com/playlist?list='$surl
>  ;;
> +                     36)
> +-                        selected_urls=$selected_urls$new_line"$(printf "%s" 
> "$videos_json" | jq '.[].url' | grep -F "$surl" | tr -d '"')" ;;
> ++                        selected_urls=$selected_urls$new_line"$(printf "%s" 
> "$videos_json" | guix-jq '.[].url' | grep -F "$surl" | tr -d '"')

Don't forget patching 'grep' and 'tr'.

> " ;;
> +                     *) continue ;;
> +             esac
> +             refined_selected_data=$refined_selected_data$new_line$(printf 
> '%s' "$videos_data" | grep "|$surl" )
> +@@ -1014,7 +1014,7 @@ print_data () {
> + get_video_format () {
> +     # select format if flag given
> +     [ $show_format -eq 0 ] && return
> +-        formats=$(youtube-dl -F "$(printf "$selected_urls")") 
> ++        formats=$(guix-youtube-dl -F "$(printf "$selected_urls")")
> +         line_number=$(printf "$formats" | grep -n '.*extension  
> resolution.*' | cut -d: -f1)

Don't forget 'grep -> guix-grep' and 'cut -> guix-cut'

> +         quality=$(printf "$formats \n1 2 xAudio" | awk -v 
> lineno=$line_number 'FNR > lineno {print $3}' | sort -n |  awk -F"x" '{print 
> $2 "p"}' | uniq | sed -e "s/Audiop/Audio/" -e "/^p$/d" | eval "$menu_command" 
> | sed "s/p//g")

Don't forget 'awk -> guix-awk' and 'sort -> guix-sort' and 'uniq -> 'guix-uniq'

> +             [ -z "$quality"  ] && exit;
> +@@ -1026,9 +1026,9 @@ get_video_format () {
> + get_sub_lang () {
> +     if [ $auto_caption -eq 1 ]; then
> +         #Gets the auto generated subs and stores them in a file
> +-        sub_list=$(youtube-dl --list-subs  --write-auto-sub 
> "$selected_urls" | sed '/Available subtitles/,$d' | awk '{print $1}' | sed 
> '1d;2d;3d')
> ++        sub_list=$(guix-youtube-dl --list-subs  --write-auto-sub 
> "$selected_urls" | sed '/Available subtitles/,$d' | awk '{print $1}' | sed 
> '1d;2d;3d')

Don't forget 'sed -> guix-sed' and 'awk -> guix-awk'

> +         if [ -n "$sub_list" ]; then
> +-            [ -n "$selected_sub" ] ||  selected_sub=$(printf "$sub_list" | 
> eval "$menu_command") &&  youtube-dl  --sub-lang $selected_sub  
> --write-auto-sub --skip-download "$selected_urls" -o /tmp/ytfzf && 
> YTFZF_SUBT_NAME="--sub-file=/tmp/ytfzf.$selected_sub.vtt" || printf "Auto 
> generated subs not available."
> ++            [ -n "$selected_sub" ] ||  selected_sub=$(printf "$sub_list" | 
> eval "$menu_command") &&  guix-youtube-dl  --sub-lang $selected_sub  
> --write-auto-sub --skip-download "$selected_urls" -o /tmp/ytfzf && 
> YTFZF_SUBT_NAME="--sub-file=/tmp/ytfzf.$selected_sub.vtt" || printf "Auto 
> generated subs not available."

FWIW, writing to "/tmp/ytfzf" seems to be a potential security problem
and bad behaviour on multiple-user systems.

What would happen if /tmp/ytfzf is a symlink to /etc/passwd an ytfzf is run
as root (yes, running as root is not recommended)? Would that brick the system?
What if /tmp/ytfzf is a symlink to ~/.profile? Would that brick the login?

What if multiple users run ytfzf concurrently? Would they overwrite eaech
other subtitles?  Would a different user be able to see what the other
is downloading?

A relatively easy fix would be to write to, say, $HOME/.cache/ytzf-subs
instead (not sure what the proper directory would be), which is completely
under the user's control.

> +@@ -1262,7 +1262,7 @@ EOF
> + update_ytfzf () {
> +     branch="$1"
> +     updatefile="/tmp/ytfzf-update"
> +-    curl -L 
> "https://raw.githubusercontent.com/pystardust/ytfzf/$branch/ytfzf"; -o 
> "$updatefile"
> ++    guix-curl -L 
> "https://raw.githubusercontent.com/pystardust/ytfzf/$branch/ytfzf"; -o 
> "$updatefile"
> + 
> +     if sed -n '1p' < "$updatefile" | grep -q '#!/bin/sh'; then

FWIW, as update-ytfzf will be more-or-less deleted
in the next patch, don't forget to patch 'sed' and 'grep'

> +             chmod 755 "$updatefile"
> +@@ -1346,10 +1346,10 @@ create_subs () {
> +     : > "$config_dir/subscriptions"
> + 
> +     # check how many subscriptions there are in the file
> +-    sublength=$( jq '. | length' < "$yt_sub_import_file" )
> ++    sublength=$( guix-jq '. | length' < "$yt_sub_import_file" )
> + 
> +     for i in $(seq $((sublength - 1))); do
> +-        channelInfo=$(jq --argjson index ${i} '[ 
> "https://www.youtube.com/channel/"; + .[$index].snippet.resourceId.channelId + 
> "/videos", "#" + .[$index].snippet.title ]' < "$yt_sub_import_file")
> ++        channelInfo=$(guix-jq --argjson index ${i} '[ 
> "https://www.youtube.com/channel/"; + .[$index].snippet.resourceId.channelId + 
> "/videos", "#" + .[$index].snippet.title ]' < "$yt_sub_import_file")
> +     printf "%s\n" "$(printf "%s" "$channelInfo" | tr -d '[]"\n,')" >> 
> "$subscriptions_file"

And don't forget 'tr'.

Greetings,
Maxime.

signature.asc
Description: This is a digitally signed message part