Codecov Report

Merging #2505 into master will decrease coverage by 0.06%.
The diff coverage is 30%.

@@            Coverage Diff             @@
##           master    #2505      +/-   ##
==========================================
- Coverage   52.59%   52.52%   -0.07%     
==========================================
  Files         239      239              
  Lines       15073    15121      +48     
==========================================
+ Hits         7927     7942      +15     
- Misses       7146     7179      +33

Why is esprima being added here? We don't add additional dependencies like this without some sort of discussion and you've already been informed this is not acceptable. The decision regarding any sort of javascript parser has not been made by the maintainers or contributors. If you want to start a discussion on it feel free to do so, but this PR can't be merged with this change. This PR is in relation to this comment: #2178 (comment) and they claim it is simply a URL change.

This is surprise. What do You mean? I thought there was discussion containing some pros and contra. If You think it is not enough You can add more arguments letting it continue. Finally You can make decision either accept or deny and close the issue like You did with foxnews plugin when You wanted to stop discussion. By the way, there was question asked about esprima but it got no answer.

As for "it is simply a URL change" - it is not quite true to say the least. There are some more. And some changes happened right when I was writing the code. Worse of that the site code is slightly differ for domains. And these are all more causes to use parser.

@vstavrinov, you assume that the full python support is available on all devices, but unfortunately that is not the case and one depends on the manufacturer of the devices / firmware and the hardware limitations.

@Billy2011 give me example of the device where streamlink is running without problem, while esprima causes the problem and what that problem is.

@vstavrinov I tried it and it won't play any stream.

streamlink -l debug https://vaughn.live/atlantis6 worst
[cli][debug] OS:         Linux-4.19.56-1-clear-lts2018-ucode-20171117.native-x86_64-with-arch
[cli][debug] Python:     3.7.3
[cli][debug] Streamlink: 1.1.1+30.g209e431.dirty
[cli][debug] Requests(2.22.0), Socks(1.7.0), Websocket(0.56.0)
[cli][info] Found matching plugin vaughnlive for URL https://vaughn.live/atlantis6
[plugin.vaughnlive][debug] Stream URL: https://v4pm2.vaughnsoft.net/play/live_atlantis6.flv?&secure=gejb0yFPGJPqmA6RH7vnKA&expires=1561508421&ip=46.123.254.60
[cli][info] Available streams: live (worst, best)
[cli][info] Opening stream: live (http)
[cli][error] Try 1/1: Could not open stream <HTTPStream('https://v4pm2.vaughnsoft.net/play/live_atlantis6.flv?&secure=gejb0yFPGJPqmA6RH7vnKA&expires=1561508421&ip=46.123.254.60')> (Could not open stream: Unable to open URL: https://v4pm2.vaughnsoft.net/play/live_atlantis6.flv?&secure=gejb0yFPGJPqmA6RH7vnKA&expires=1561508421&ip=46.123.254.60 (403 Client Error: Forbidden for url: https://v4pm2.vaughnsoft.net/play/live_atlantis6.flv?&secure=gejb0yFPGJPqmA6RH7vnKA&expires=1561508421&ip=46.123.254.60))
error: Could not open stream <HTTPStream('https://v4pm2.vaughnsoft.net/play/live_atlantis6.flv?&secure=gejb0yFPGJPqmA6RH7vnKA&expires=1561508421&ip=46.123.254.60')>, tried 1 times, exiting

@mmetak Yes, I know. The site code has changed again after I committed this pull request. As I mentioned earlier this is already second time the changes happens while I am working on this plugin. But this time the things get worse, because secret key is not exposed now and should be calculated in some way. This key generated by javascript in browser and this functionality should be re-implemented in external environment with python. Recently I have solved similar task when was working here on foxnews plugin that was rejected, so I hope on success in this case too.

@vstavrinov

• I did not specifically refer to esprima

• On most linux set top boxes, there is no pip but opkg to install additional packages and that is unfortunately very limited

• I do not like creating extra dependencies so a few plugins will work again, especially if it were possible with the resources available

• many of the embedded linux devices only have limited hardware resources (memory, cpu ...)

@Billy2011

• The matter is about js parser only, nothing else.
• Did You try:
  opkg install python-pip
  What this about:
  https://openwrt.org/packages/pkgdata/python-pip
• Parser is not specific requirement for some plugins, but could be used in any one, either new, or existing. And yes, You may prefer walk instead of driving car. There are many other things in our life that we can do without, but we prefer to have them rather than not to have them.
• I am running streamlink with esprima on poor android device without problems.

I have to agree with @gravyboat. I don't see any merits of using a JS parser here and it's just unnecessary bloat which causes packaging problems for Streamlink.

After taking a quick look at the changes of the pull request, it looks like you are using the generated AST of the JS parser to find a certain variable declaration in the utter mess of the vaughnlive JS code. This could also be done with a regex, although not as nicely as with an AST, but it should still work. If they would change the name of the variable you're searching for in the future, then both methods will fail, so using a JS parser is not a solid solution for implementing this.
If the code becomes even more complex, then parsing an AST becomes even less reliable.

@bastimeyer,

• Package maintainer could always use pip in post installation script to satisfy any dependencies. This is regular work and not a problem.
• As for finding variables and other aspects - it is not what the issue is about, again: You shouldn't consider this as specific case - it is rather general issue.
• regex is great tool almost for everything, but in loose and messy html / js contents it is vulnerable for casual, but not logical changes and could produce ambiguous results. It is good for
  one-off application where we need empirical approach. But for regular tasks a parser provides system and logical solution.

I think that having a JS parser would actually be quite useful for several plugin, where, as @vstavrinov pointed out, regex is clunky. Which package is most useful and if that package is feasible to add is another thing.

@vstavrinov pip as a post-install step is generally not a good idea, state is not well defined, failures are more complex, removal even more so.

@beardypig

Which package is most useful

My quick lookup makes me think that esprima is most widely used "classic" tool originally written in js, used under nodejs and then ported to python. May be I am wrong, but I satisfied with my short experience with it. Fill free to probe somewhat other if it will better.

pip as a post-install step is generally not a good idea, state is not well defined, failures are more complex, removal even more so.

I have referenced postinst script as example only of possible solution. If You wish You can use preinst or even Makefile. You can do this at any stage of installing or packaging. Every package management system has their own means to solve such standard tasks. Eventually You can create separate package for it. Whatever way You choose in any case there are no problems as long as the package available in pip, or even without it this task looks relatively simpler in comparable with big complex compiled software with lot of library dependencies.

@vstavrinov I fully accept that it is possible, and each distro will have their own packaging guidelines and requirements - eg. I imagine Debian would reject any package that used pip to install a dependency. If you use pip to install a dependency the package manager cannot track it, so I cannot be removed correctly (what if another package is using it?). It also means you might break something else if the version requirements are different (eg. 1.0 vs. 2.0). The only way to do it is to build a package for that dependency, which is an extra burden on the package maintainer - something which should be avoided. Not everyone has unlimited time to spend on their hobby projects, and if we make it harder for people to maintain they will be discouraged from continuing to maintain it and everyone loses out.

The comments by @mmetak and @Billy2011, should not be taken lightly as they both fall in to the category of package maintainer and their contributions are important to the Streamlink community. We should strive to make Streamlink better for everyone, and losing support for a distro's package would not make it better. Of course, I don't want to discourage you from continuing the contribute, as losing a contributor would be equally bad for the project. But, we all have to work together - we are a team, and the plight of your fellow teammates should not be overlooked :)

I'm not saying it's impossible to add a dependency, I have personally added a couple of dependencies and probably caused @mmetak some headache... Given the size of the project now I would not add another without serious consideration and discussion.

I would like to propose that create a guideline for adding a dependency to the project which should outline the possible issues, packaging requirements, licenses requirements, and request a short explanation of why we should add that package and how the benefits outweigh the costs. If we have a clearly thought out and well written argument for adding a dependency, we can better review its impact and consider it for inclusion.

I think it's clear that it will be a fairly lengthly process to add a a new dependency, so with that in mind, is there another way to resolve the vaughnlive issue using existing tools? :)

@beardypig:

• I totally agree we should discuss dependency and that is exactly what we are doing here.
• I don't agree that dependency is a problem. As I stated higher, it is not a problem - it is regular task. This is something differ. And it is not issue of free time. It is issue of demand and supply balance. If someone have no time, somebody else will do this, if demand exists. And that is what we are discussing here - if this demand exists. Is it demand or need - whatever word fits more.
• As for Debian, there already exists node-esprima package and few other esprima related or compatible packages. So I hope this fact lifts chances for python-esprima package to be created. More over, adding this dependency to streamlink could encourage them to do this rather sooner than later. I guess there are similar situation in other distros.
• As for vaughnlive, I have to repeat again: They all are not about certain things like this plugin or finding variables. Using JS parser is general issue applicable to almost any plugin in different ways.
• But in case of vaughnlive, dependency is not main obstacle preventing us to bring it alive. There are real problem, I think. The fact is that they are using google recaptcha based on private / public keys peer. I don't see any way to overcome this defense. And if nobody could find solution, this plugin should be removed. But there are other pull request plugins.ovvatv: fix site changes #2425 that is waiting for approval of using esprima.
• And finally few words about "We should strive to make Streamlink better for everyone." I am interested in vaughnlive for the sake of some USA channels. Fortunately I have found replacement for it, that is arconai that You have removed from streamllink because of requirements You impose on plugins with most of which, except one, I don't agree. I have recovered this stale plugin to working state and I am happy using it in my fork of streamlink. This is my own example of how excessive restrictions push users away from streamlink.

Hi everyone. As far as I'm concerned it's not a problem at all to add python-esprima as a dependency if that would mean a working vaughnlive and arconai plugins. It's already packaged in the aur and those two plugins would be very useful to me.

How about adding python-esprima as an extra requirement?

I maintain streamlink-git package in aur which tracks the master branch and I could easily adjust it for this change and make it possible to test it before it gets to release? There are a few arch+vaughnlive+streamlink users in here.

Seems like vaughnlive will be dead in the water if they are using Google Captcha, and arconai was removed in #2003. Adding this dependency wouldn't fixed either directly. You might be able to workaround the captcha for vaughnlive by including some cookies...

@vstavrinov As for getting the new dependency added, could you create a separate PR just adding the dependency so we don't have plugin issues mixed in with dependency issues. If you could also add some utility functions that make use of esprima, to demonstrate its general usefulness. A prime candidate for this is the common js_to_json regex based transformation.

_js_to_json = partial(re.compile(r"(\w+):\s").sub, r'"\1":')

Where it takes a JavaScript object and tries to fix it to make it parsable as regular JSON.

eg.

{ foo: "bar" } => { "foo": "bar" }

This sort of function would be useful in a lot of plugins, a function that takes the JavaScript and turns it in to a dict would be ideal. Obviously, there will be cases where this is not possible and it should fail appropriately :)

closing, currently not possible with this solution.

#2178