-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[RFC] script: add quickjs as javascript runtime #17179
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
5779044 to
9c42955
Compare
|
Thanks. This looks interesting. You should have read contribute.md and followed it, however, at the very least, this PR is missing:
So please read the contribution guidelines fully and carefully, and follow them as best as you can. Preferably, please also split it into 2 commits: one which only renames the current js (c) files to Also, the PR currently disables almost all of the CI builds - don't do that. Please restore them, and in general don't make changes outside the scope of the PR. Build/deps questions:
Some questions regarding the actual code and behavior:
Integration: As far as I can tell, the build replaces mujs with quickjs by default, and if we keep it as either mujs or quickjs, then at least for now mujs should remain the default. We'd need a lot of feedback and experience before we can replace the engine, because backward compatibility is very important. However, I did have another idea, and I'd appreciate some feedback on it by others: what if we allow both to be built together, and use the mujs backend with This way people could experiment with quckjs with the standard build (assuming both are enabled), without interfering with any existing scripts? I think that technically it should be pretty easy to do. Thoughts? |
|
Thansk for your quick reply! I'll read contributing.md again.
I do these because this PR is yet in developing, I disable build ci jobs and set quickjs on default to do a quick testing on ci (in my GitHub fork), I'll enable them before it's finally ready.
The building/ci config part is the minimal change to make it work.
the upstream quickjs is a makefile only project, it doesn't have meson or cmake build file, make it very hard to use as a library in our project. So I don't know if it works with upstream quickjs, someone may need to add a cmake file or meson.build file first.
With LLM, this code complies with our license. And no.
I would expect it, but I haven't run any tests yet, this is still WIP.
OK I'll add them later. |
Thanks. We can keep it like that for now. However, I'd want you to enable also the x86-64 msvc build, and one of the mingw build, so that windows users would be able to download and play locally with the resulting CI builds.
OK, we can keep quickjs-ng for now, but if this ever gets merged, we'd need to consider this more carefully, because generally upstream projects should be preferred, unless there are good reasons to not use them, and I don't know yet that "being a makefile only project" is a good enough reason.
But you did explicitly mark it as "ready for review", right? A PR which was marked as ready for review should be considered at least reasonably ready, even if not fully ready. And this means that at the very least the author tested that it generally works. And this is a lot of code - 1500 lines. A lot can go wrong with 1500 lines of C code which no one tested... If it was not tested at all, then this should have been stated clearly. The contribution guidelines state clearly that the author should test it before submitting it. We can't guess what the author did or think, and your PR has literally zero info which you wrote about it. We simply can't guess it. This is information which you need to provide. Before we continue, a reminder that there are few subjects which you didn't respond to, specifically, for instance splitting it into two commits, and supporting both backends together - according to the filename extension, so I'd appreciate if you could fill the missing replies to my original questions. Thanks.
Thanks. Since we don't yet have a policy for contributions using LLMs, we'd have to figure it out as we go, so I'd appreciate your patience in answering the following questions. Feel free to also volunteer interesting relevant info also beyond the questions. How much experience do you have with C? did you write or contribute meaningful C code which you wrote on your own? if yes, mind giving some examples? What was the process of opening this PR? how did you start? how did you iterate? what kinds of problems came up during the development? how were these problems solved? Feel free to also provide additional info on the process which might interest others. How much of the code was written by an LLM, and how much of it is your own? how much code did you touch/fixed yourself? To what degree do you understand this code? (for instance "not at all", or "only the overall structure", or "absolutely every bit of it", or anything else in between). Did you go over every line of this code and confirmed to yourself it looks good? Or in other words, did you review all this code carefully yourself? If I asked you questions about the code - approaches, structures, construct, integrations with mpv code, and anything else really, do you think you'd be able to answer them reasonably good without the help of an LLM? I know you said "I haven't run any tests yet", but does it mean you really didn't even run it? never tried even a single one-line js script? If you did run it and did some tests, please do elaborate what you tested and how. Thanks. |
Yeah, that's a mistake, so I change it back to draft. This is my first PR, so I'm also a little bit confusing with the |
Thanks.
No worries, and really thanks for the PR. I do find it very interesting. Also, don't worry about the exact procedure with "[RFC]" and draft status etc. The most important thing is that you explain clearly where it stands, what's the status, what was tested, what's still to do, etc, and of course any questions which you might have and which we might be able to help with. |
Ididn't write much pure-c, I did use pure-c in some my toy project. But I do write c++ ( and some c when it's necessary, for example, exporting a so ) for a living, and many works on binding language A to lanaguge B.
I already have write some js script in mpv so I know what the js runtime need to provide, for example, the Then I read the docs and example of quickjs-ng Then find out the exported API (the
I'd call 80% llM and 20% me.
I think it's between "only the overall structure" and "absolutely every bit of it". I understand the overall calling structure.
yes.
yeah, I didn't write much c but have no difficulty understanding them.
I only run the tests that what we have in meson.build. didn't run the mpv executable with js script. that's the next move I was planning, after we put the "rename" in a seprated |
|
Also, since this is your first PR, let me answer one question which you didn't ask: If you want to make changes, don't open a new PR. Just keep changing this PR (splitting it to several commits, or adding commits, etc). Thanks. |
|
Thanks. Good answers. I think we can continue ;)
OK. Once you test it a bit, please post some info about what you tested and what were the results (before you make further changes to the code).
As noted - don't open a new PR. just split this one into two commits. Thanks. |
Eh, do use underscore ( |
So I guess you are worried that I submit some LLM generated code but doesn't have the ability to address the code review? |
Yes. But you seems to have done at least some work on your own, so hopefully we're good. I would have preferred that you'd know every last bit of all the code as if you wrote it yourself, instead of reading and understanding it to answer questions which come up, but for now, let's continue and see how it goes. Also, to help us (and you) keep track of things, I'd appreciate if you could maintain a todo list at the first message. It doesn't have to be fancy, just a list of items and their status in plain text, which you'll keep up to date ("not done yet", "done but not tested", "finished and should be ready", or any other descriptions which would describe the items status). It's also fine to add items over time, and it doesn't have to be in any particular order, but it should be clear what's the status of each item, and what remains to do. Some things which come to mind, and which might fit in such list (feel free to add items now or over time, and generally write and maintain it yourself):
etc. Also, one question I had which hopefully you'd be able to answer: mujs doesn't have But quickjs does have all three (and also How do these work with quickjs in general? How do these work with quickjs in mpv? (if you're not sure about the answers right now, feel free to add it to your todo list). |
5278d2a to
24cd172
Compare
I need to do some testing. if these API from quickjs has same behavoir we can remove them from shim to provide same API to users, or fix the behavoir in another shim. |
Right. For what it's worth, these are the kinds of questions about the code which I was referring to, which I hoped you'd already know the answers pretty much on the spot, because regardless of who wrote it - you are submitting it as your code, and as the author, these are things which you should have known and figured out already (and addressed where applicable) while writing the code. So FYI. You need to really know the code well before we can discuss it. So if you need to take some time to go over every bit again to understand it fully - take your time, but please do understand it well - as if you wrote it yourself and came up with all the solutions yourself.
I'm not sure about that, and there should first be some way to tell if they really do behave the same. For instance, the Also the fact that it looks like something behaves correctly, for instance with the timers API, doesn't mean that it's also necessarily correct. Timer related things typically depend on some kind of event loop or threading or system timers APIs, and the way it's integrated with the mpv model is important to get right. With mpv it's tied to mpv's event loop, and it's important to keep it integrated well with this event loop. So once you get to it, I suggest you follow the questions I asked in that order (and explain it to me too). I.e. first how does quickjs handle "require" and timer APIs on its own, then understand how the current mujs (and lua) code works with timers in mpv, and then how it works with quickjs in the context of mpv, and which changes are required, if any. For what it's worth, at least for starters, the simplest approach would likely be to ensure that the existing implementations at |
|
Download the artifacts for this pull request: |
add quickjs runtime support based on https://github.com/quickjs-ng/quickjs
use option
-Djavascript=quickjsmeson wrap install quickjs-ngcan't be found by meson.build