invidious icon indicating copy to clipboard operation
invidious copied to clipboard

[Question] Skip downloading the video.js tarballs if they are already in `assets/npm-tarballs/`

Open mk-pmb opened this issue 6 months ago • 7 comments

Is your enhancement request related to a problem? Please describe. I want to make it easier for people on exotic networks to develop and contribute here. Currently, one deterrent is how to circumvent the hard-coded download mechanism in fetch-player-dependencies.cr line 86.

Describe the solution you'd like Devs can download the required tarballs themself, put them in the assets/npm-tarballs/ folder, and fetch-player-dependencies.cr will accept them there. It would just skip the download step, but still verify them and unpack them as normal.

Describe alternatives you've considered Modify download_path = … and delete the code for the download command and its callback function. Works as a one-off fix but I'd prefer a way without hotpatching so I can rebase directly without having to remove and re-apply my hotfix each time.

Additional context

[Outdated original feature request]

Is your feature request related to a problem? Please describe. (Alternative solution to #4360 ) In my network setup, the docker container cannot directly download from https://registry.npmjs.org/ , so I'd like to use proper npm to download the tarballs outside of docker.

Describe the solution you'd like I've started making a script that successfully determines deps and uses npm to download them. Now it's about unpacking the files:

$ ls -1 assets/videojs/
video.js-7.12.1.tgz
videojs-contrib-quality-levels-2.1.0.tgz
videojs-http-source-selector-1.1.6.tgz
videojs-markers-1.0.1.tgz
videojs-mobile-ui-0.6.1.tgz
videojs-overlay-2.1.4.tgz
videojs-share-3.2.1.tgz
videojs-vr-1.8.0.tgz
videojs-vtt-thumbnails-0.0.13.tgz

Now how should I handle unpacking? Can I downlaod them to a directory where the docker installer will accept them and do its thing? Should I unpack them myself? Could someone provide paths for some of the files expected to be created by proper unpacking? (Edit: I figured I can make the download step work by just editing the hard-coded URL to point to my local mirror, so I can now see what is expected to be unpacked.) Some of the packages have dist/ directories, so do we need just them somewhere?

Describe alternatives you've considered

Additional context

mk-pmb avatar Dec 29 '23 05:12 mk-pmb

It seems that the tarballs are expected in /tmp/invidious-videojs-dep-install and if I understand the crystal downloader correctly it will not currently accept any pre-existing file. I suggest we

  • skip the download attempt if the tarball exists in assets/videojs/ assets/npm-tarballs/
  • move the sha1sum check to after the download, rather than in what seems to be a callback.
  • add the -b flag to sha1sum to properly convey our intent of a binary comparison even though it's the default on linux.

mk-pmb avatar Dec 29 '23 06:12 mk-pmb

Changing from a feature request to a question because we are not going to complicate the building code just for some niche use cases.

If you can't build invidious in local then use our official docker image https://quay.io/invidious/invidious or just build the image on a separate computer that have full internet access.

Invidious hasn't been designed for complicated network setups.

unixfox avatar Dec 29 '23 07:12 unixfox

Invidious hasn't been designed for complicated network setups.

Any file retrieval mechanism can fail for lots of reasons, so it's always good to have a way for users to provide those files by some other means. If we do the right thing here, it yields support for niche use cases as merely a side effect.

use our official docker image

Wouldn't work in my case because I'm trying to fix #2837 which requires recompile.

we are not going to complicate the building code just for some niche use cases.

Unfortunately it seems that no-one with a less-niche setup had time to tackle the issue in almost 2 years. Who knows how many people could have solved it already if it were easier to develop on strange networks. (school, university, corporate, chinese, …)

mk-pmb avatar Dec 29 '23 20:12 mk-pmb

Unfortunately it seems that no-one with a less-niche setup had time to tackle the issue in almost 2 years. Who knows how many people could have solved it already if it were easier to develop on strange networks. (school, university, corporate, chinese, …)

Feel free to submit code change for that then!

The code is fully open source, available for everyone to submit new changes in the code.

Meanwhile we as core developers of invidious are not going to invest in these niches setup. That's part of developing an open source product we can't make everyone happy.

unixfox avatar Dec 29 '23 22:12 unixfox

I updated the opening post to reflect my current understanding of what would be a good approach. I'll try and make a PR that separates downloading from the checkusm validation, then learn more crystal in order to make it use the existing files if there are any.

Edit: I'll use assets/npm-tarballs/ to be future-proof in case we might some day need other JS packages.

mk-pmb avatar Dec 31 '23 07:12 mk-pmb

This feature should be available once #4438 is merged though you'll have to rename the downloaded tarballs. Let me know how well it works!

syeopite avatar Feb 19 '24 23:02 syeopite

Thanks! I wasn't quickly able to merge it with the proxy support branch, but I'll try again later.

mk-pmb avatar Feb 20 '24 05:02 mk-pmb