yt-dlp / yt-dlp

A feature-rich command-line audio/video downloader
https://discord.gg/H5MNcFW63r
The Unlicense
80.09k stars 6.26k forks source link

`NO_COLOR` environment variable overrides `--color auto` #10420

Closed 0n-s closed 1 month ago

0n-s commented 1 month ago

DO NOT REMOVE OR SKIP THE ISSUE TEMPLATE

Checklist

Provide a description that is worded well enough to be understood

yt-dlp respects the NO_COLOR standard for disabling terminal color output by default. However, passing --color auto on the command line does not override it.

Passing --color auto explicitly should override NO_COLOR, which merely specifies a default. --color always is unideal since it means color codes are enabled even when stdout isn't a terminal.

Provide verbose output that clearly demonstrates the problem

Complete Verbose Output

$ yt-dlp -vU --ignore-config --color auto FOuoW68nkrw
[debug] Command-line config: ['-vU', '--ignore-config', '--color', 'auto', 'FOuoW68nkrw']
[debug] Encodings: locale UTF-8, fs utf-8, pref UTF-8, out utf-8, error utf-8, screen utf-8
[debug] yt-dlp version local@2024.07.10 [705f5b84d]
[debug] Python 3.11.9 (CPython x86_64 64bit) - Linux-6.9.8-x86_64-Intel-R-_Xeon-R-_CPU_E5-2420_0_@_1.90GHz-with-libc (OpenSSL 3.2.2  1 Jan 1970, libc)
[debug] exe versions: ffmpeg present (needs_adtstoasc,setts), ffprobe present
[debug] Optional libraries: brotli-1.0.9, certifi-3021.03.16, mutagen-1.46.0, pycrypto-3.18.0, requests-2.31.0, sqlite3-3.40.1, urllib3-2.0.3
[debug] Proxy map: {}
[debug] Request Handlers: urllib
[debug] Loaded 1834 extractors
[debug] Fetching release info: https://api.github.com/repos/yt-dlp/yt-dlp/releases/latest
Latest version: stable@2024.07.09 from yt-dlp/yt-dlp
yt-dlp is up to date (local@2024.07.10)
[youtube] Extracting URL: FOuoW68nkrw
[youtube] FOuoW68nkrw: Downloading webpage
[youtube] FOuoW68nkrw: Downloading ios player API JSON
[debug] [youtube] Extracting signature function js_9ed4a7e1_105
[debug] Loading youtube-sigfuncs.js_9ed4a7e1_105 from cache
[debug] Loading youtube-nsig.9ed4a7e1 from cache
[debug] [youtube] Decrypted nsig imA6eMryKfy9COoE => okCEH0HQm4JFOA
[debug] Loading youtube-nsig.9ed4a7e1 from cache
[debug] [youtube] Decrypted nsig ybQEeSqB93bFMfzj => Yp8_uwe72M1FeQ
[debug] [youtube] Extracting signature function js_9ed4a7e1_101
[debug] Loading youtube-sigfuncs.js_9ed4a7e1_101 from cache
[youtube] FOuoW68nkrw: Downloading m3u8 information
[debug] Sort order given by extractor: quality, res, fps, hdr:12, source, vcodec:vp9.2, channels, acodec, lang, proto
[debug] Formats sorted by: hasvid, ie_pref, quality, res, fps, hdr:12(7), source, vcodec:vp9.2(10), channels, acodec, lang, proto, size, br, asr, vext, aext, hasaud, id
[debug] Default format spec: bestvideo*+bestaudio/best
[info] FOuoW68nkrw: Downloading 1 format(s): 244+251
[debug] Invoking http downloader on "https://rr1---sn-capm-vnae.googlevideo.com/videoplayback?expire=1720667623&ei=h_mOZr7UO9yEv_IPoZK8wAQ&ip=XXX&id=o-APrqACYQsIsDhtZQiy7FA7rMrJ8dFRG6gw8jWGnHLo4D&itag=244&aitags=133%2C134%2C135%2C160%2C242%2C243%2C244%2C278&source=youtube&requiressl=yes&xpc=EgVo2aDSNQ%3D%3D&mh=44&mm=31%2C29&mn=sn-capm-vnae%2Csn-5hne6nsy&ms=au%2Crdu&mv=m&mvi=1&pl=24&gcr=no&initcwndbps=1818750&bui=AXc671IC7kEtzqHrev-bxoBTzPGHFvczgBWeEn7ZQGodWTYePI71cvU7Uflq7HZVJwB63CkzR9v2xf-4&spc=NO7bAcm1rUpzJlz6aJV3UPdr1hUqrMJhSawcTh3Iw2zMGXHM-P28HnnWuwzK&vprv=1&svpuc=1&mime=video%2Fwebm&ns=bM1a0nlKF_H90_7W2872DIAQ&rqh=1&gir=yes&clen=172309&dur=33.333&lmt=1719184131865464&mt=1720645513&fvip=4&keepalive=yes&c=WEB&sefc=1&txp=530F224&n=Yp8_uwe72M1FeQ&sparams=expire%2Cei%2Cip%2Cid%2Caitags%2Csource%2Crequiressl%2Cxpc%2Cgcr%2Cbui%2Cspc%2Cvprv%2Csvpuc%2Cmime%2Cns%2Crqh%2Cgir%2Cclen%2Cdur%2Clmt&lsparams=mh%2Cmm%2Cmn%2Cms%2Cmv%2Cmvi%2Cpl%2Cinitcwndbps&lsig=AHlkHjAwRQIgb7-r3m1q2c7QWucFfb-WG-fi_y2VaBCoNh2oUCXane8CIQCz3juvR89hlrYCJhzgKCIWf7M12qQUxzIgTWkPF_uajA%3D%3D&sig=AJfQdSswRQIhAIYF7M2BEy5v1UdIGY25bUAxzIaWiptdGF6fRSoWQFnYAiBBhMaEeh4h0uTGiiAg8m5HxJ7V1bY5FEVTvQL6xgRZZw%3D%3D"
[download] Destination: Like That but kendrick falls down the stairs and dies-FOuoW68nkrw.f244.webm
[download] 100% of  168.27KiB in 00:00:01 at 134.58KiB/s
[debug] Invoking http downloader on "https://rr1---sn-capm-vnae.googlevideo.com/videoplayback?expire=1720667623&ei=h_mOZr7UO9yEv_IPoZK8wAQ&ip=XXX&id=o-APrqACYQsIsDhtZQiy7FA7rMrJ8dFRG6gw8jWGnHLo4D&itag=251&source=youtube&requiressl=yes&xpc=EgVo2aDSNQ%3D%3D&mh=44&mm=31%2C29&mn=sn-capm-vnae%2Csn-5hne6nsy&ms=au%2Crdu&mv=m&mvi=1&pl=24&gcr=no&initcwndbps=1818750&bui=AXc671IC7kEtzqHrev-bxoBTzPGHFvczgBWeEn7ZQGodWTYePI71cvU7Uflq7HZVJwB63CkzR9v2xf-4&spc=NO7bAcm1rUpzJlz6aJV3UPdr1hUqrMJhSawcTh3Iw2zMGXHM-P28HnnWuwzK&vprv=1&svpuc=1&mime=audio%2Fwebm&ns=bM1a0nlKF_H90_7W2872DIAQ&rqh=1&gir=yes&clen=550194&dur=33.381&lmt=1719184130555699&mt=1720645513&fvip=4&keepalive=yes&c=WEB&sefc=1&txp=5308224&n=Yp8_uwe72M1FeQ&sparams=expire%2Cei%2Cip%2Cid%2Citag%2Csource%2Crequiressl%2Cxpc%2Cgcr%2Cbui%2Cspc%2Cvprv%2Csvpuc%2Cmime%2Cns%2Crqh%2Cgir%2Cclen%2Cdur%2Clmt&lsparams=mh%2Cmm%2Cmn%2Cms%2Cmv%2Cmvi%2Cpl%2Cinitcwndbps&lsig=AHlkHjAwRQIgb7-r3m1q2c7QWucFfb-WG-fi_y2VaBCoNh2oUCXane8CIQCz3juvR89hlrYCJhzgKCIWf7M12qQUxzIgTWkPF_uajA%3D%3D&sig=AJfQdSswRAIgXFcK4YEU4KOQob9EcrX2gXMsE5CW8q4PYMP7jItsRrgCIDsh8CH8Z6AafHm6tjZshMyS0w61pwYgnLw_nw5Gt2HS"
[download] Destination: Like That but kendrick falls down the stairs and dies-FOuoW68nkrw.f251.webm
[download] 100% of  537.30KiB in 00:00:02 at 226.63KiB/s
[Merger] Merging formats into "Like That but kendrick falls down the stairs and dies-FOuoW68nkrw.webm"
[debug] ffmpeg command line: ffmpeg -y -loglevel repeat+info -i 'file:Like That but kendrick falls down the stairs and dies-FOuoW68nkrw.f244.webm' -i 'file:Like That but kendrick falls down the stairs and dies-FOuoW68nkrw.f251.webm' -c copy -map 0:v:0 -map 1:a:0 -movflags +faststart 'file:Like That but kendrick falls down the stairs and dies-FOuoW68nkrw.temp.webm'
Deleting original file Like That but kendrick falls down the stairs and dies-FOuoW68nkrw.f244.webm (pass -k to keep)
Deleting original file Like That but kendrick falls down the stairs and dies-FOuoW68nkrw.f251.webm (pass -k to keep)
Grub4K commented 1 month ago

Yes, that is expected behavior. --color auto is the default value, so the absence of the flag is the same thing as passing --color auto.

There would have to be two different values for --color, one which supports env variables and one which ignores them. Otherwise, if you were to supply any --color flat in a config file, you could never make it respect the env vars again from within cli.

I would propose a new option --color auto_no_env for this

Grub4K commented 1 month ago

Also note that theres a difference between --color always and --color no_color. In both cases it will use terminal sequences, but only in the first will it output color sequences as well...

when using auto, if TERM is set to dumb, it will not use any terminal seqnences, then if NO_COLOR is set it will use terminal sequences but output no color, and otherwise use all sequences.

Should a new option like that respect TERM or not in this case? I think this is quite a difficult thing to decide

0n-s commented 1 month ago

Should a new option like that respect TERM or not in this case? I think this is quite a difficult thing to decide

AFAIK TERM should always be respected if the output is a terminal (i.e. isatty() or equivalent). TERM is just not a preference of the user, it is simply a pointer to the capabilities of the terminal set up by the terminal itself. It's not really meant to be overridden.

EDIT: for this reason:

I would propose a new option --color auto_no_env for this

I would suggest naming the value to be something like tty or the like. It clearly specifies how it works: color if outputting to a terminal, no color if not. Environment variables are not necessarily user configuration, & TERM most certainly is not. :)

Alternatively, most programs often initialize parameters like this to a NULL or -1 value, with a separate auto value that has the behavior described above (i.e. just isatty()). This essentially differentiates between "respect system defaults & use in appropriate contexts if allowed by them" & "user told us to use color for this program if appropriate" (again, this means passing the isatty() & ncurses has_colors()[1] checks). But I can see that breaking backwards compatibility; not sure how much of a concern that is for...color output.

[1]: this is basically the more correct equivalent to the TERM=dumb check that handles a few corner cases.

Grub4K commented 1 month ago

So I now propose the following change:

Let me know what you think, I have a PR ready to go

0n-s commented 1 month ago

That seems sound, yes. Thanks!

Grub4K commented 1 month ago

For anyone wondering how the policies work in detail now, heres a guide. TERM is True if the TERM variable is not set to dumb, the stream is not piped and, on Windows only, VT sequences could be activated. NO_COLOR is True if the variable is nonempty

image