-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
enhance(main/python-yt-dlp): add deno & node as optional dependencies
#26714
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
packages/python-yt-dlp/build.sh
Outdated
| TERMUX_PKG_SHA256=1afacd10ec55e4d18dca1f6398823f8b642b5d1c524913a065985b482b8a73c2 | ||
| TERMUX_PKG_DEPENDS="libc++, libexpat, openssl, python, python-brotli, python-pip, python-pycryptodomex" | ||
| TERMUX_PKG_RECOMMENDS="ffmpeg" | ||
| TERMUX_PKG_RECOMMENDS="deno, ffmpeg" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't that be Node OR Deno?
| TERMUX_PKG_RECOMMENDS="deno, ffmpeg" | |
| TERMUX_PKG_RECOMMENDS="nodejs | nodejs-lts | deno, ffmpeg" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if that will work, but I can test whichever method this PR chooses and report what happens on my 32-bit device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should also be noted that the version of yt-dlp that will depend on these things is still a draft in upstream, so it's not yet possible to test the actual runtime behavior of yt-dlp when exposed to these dependencies, without also applying that branch temporarily to the package, and even then, the behavior might be unfinished and not final.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Installing the current version of this PR on my 32-bit device resulted in this printing:
Recommended packages:
deno
but apt proceeded to not attempt to install deno or mention it at all during the rest of the installation process and no errors happened, so that is as expected, while on a 64-bit device it would automatically install deno, so I believe that just confirms that it is fine to add deno to TERMUX_PKG_RECOMMENDS. I haven't tested what happens if it is changed to nodejs | nodejs-lts | deno, but I can if this PR is changed to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we have architecture dependent dependencies on any packages so far.
But in theory we should be able to do a separate TERMUX_PKG_RECOMMENDS for 64 bit and 32 bit architectures if having deno in the list on architectures that it isn't available for would cause issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in theory we should be able to do a separate
TERMUX_PKG_RECOMMENDSfor 64 bit and 32 bit architectures if havingdenoin the list on architectures that it isn't available for would cause issues.
I have just tested that and explained that does not appear to cause any issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be "nicer" to not have it there on the 32 bit architectures regardless so it doesn't lead to any potential confusion for users.
i.e "Why can't I install wine on my device, oh it's 32 bit ARM"
So something like this below might be a good idea.
TERMUX_PKG_RECOMMENDS="nodejs | nodejs-lts | deno, ffmpeg"
if (( TERMUX_ARCH_BITS == 32 )); then
TERMUX_PKG_RECOMMENDS="nodejs | nodejs-lts, ffmpeg"
fiThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
going with the oneliner since @robertkirkman didn't see any mentions of deno
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way (in the current version of this PR at time of writing) works great, when I install the 32-bit build on the 32-bit device, nodejs is installed automatically, while when I install the 64-bit build on a 64-bit device, deno is installed automatically instead.
What remains is, that we don't know yet how exactly yt-dlp will actually behave when exposed to these dependencies (for example whether it will automatically attempt to actually use the nodejs dependency in the 32-bit build or not without downstream patching or explicit arguments at runtime), so maybe we just need to wait for the yt-dlp release to find out what will happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, we will have to wait and see.
currently according to this wiki change, ejs will need to be installed and deno will work automatically
for 32bit systems we will have to manually enable nodejs support, either by conditionally adding it to system config or with patches
yt-dlp will soon have deno, node and bun as runtime dependencies. add two of them which are available in termux repos
previous discussion: #26680