paulrouget
commented
9 months ago Hi! This is amazing work @kuanyui ! Thank you so much for taking the time to look into this.
Here are some comments:
- please make this a Linux only thing for now. I'd argue that only Linux user need that, and it's easier to implement for Linux systems.
- Only use PATH, PATHEXT won't be nessary
- Rename the features: "--use-system-ffmpeg" and build.sh should fail if this is used on windows or mac.
- when the package is generated, add a string to the name of the package (
vdhcoapp-…-no-ffmpeg.deb). - add the file to the list of files to publish (see the
if $publish == 1function in build.sh). - Add a dependency to ffmpeg in the deb package.
Please make findexecutable more ES6 friendly:
- I don't think it needs its own file as it can be reduced to one single function
- use arrow functions instead of function() calls.
- you can skip the map:
return envPath.replace(/["]+/g, '').split(path.delimiter).map(function (chunk) {
return envExt.split(path.delimiter).map(function (ext) {
return path.join(chunk, bin + ext);
});
}).reduce(function (a, b) {
return a.concat(b);
});could be (didn't test):
return envPath.replace(/["]+/g, '')
.split(path.delimiter)
.map((chunk) => path.resolve(chunk, bin))
.find(fileExistsSync); // Make fileExistsSync inline maybe? As it doesn't need to be its own function.with all this rewrite, I don't think we need to worry about the GPL thing.
Let me know if I can assist for any of these comments.
Thank you.
kuanyui
For the users who don't want to have pre-built
ffmpegbinaries (e.g. for saving disk storage) :--no-ship-ffmpegforbuild.sh(default is 0, which means won't change existing behavior by default)vdhcoappcan searchffmpeg,ffprobein PATH, and execute the existingffmpegin system.Currently I've tested on Linux, and confirmed that
build.shcan packagevdhcoappwithout shippingffmpegbinaries, and still can correctly download video from YouTube. However Windows and MacOS are not tested because I have no systems can test.