GNU bug report logs - #49238
Ytfzf

Previous Next

Package: guix-patches;

Reported by: Raghav Gururajan <rg <at> raghavgururajan.name>

Date: Sun, 27 Jun 2021 05:45:02 UTC

Severity: normal

Done: Raghav Gururajan <rg <at> raghavgururajan.name>

Bug is archived. No further changes may be made.

Full log

View this message in rfc822 format

From: Maxime Devos <maximedevos <at> telenet.be>
To: Raghav Gururajan <rg <at> raghavgururajan.name>, 49238 <at> debbugs.gnu.org
Cc: jgart <at> dismail.de, LibreMiami <packaging-guix <at> libremiami.org>
Subject: [bug#49238] [PATCH v1 2/2] gnu: Add ytfzf.
Date: Wed, 30 Jun 2021 21:38:15 +0200

[Message part 1 (text/plain, inline)]
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 <at> 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 (application/pgp-signature, inline)]
This bug report was last modified 3 years and 320 days ago.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.