nix-community / vs-overlay

Overlay with VapourSynth plugins packaged for Nix
MIT License
12 stars 6 forks source link

General questions regarding vs-overlay #17

Open sshiroi opened 1 year ago

sshiroi commented 1 year ago

Since I saw activity on this repo again I though I should write about a few things that in my are relevant for discussing.

I will continue working on these problems myself, but I think it would be best if someone with more nix/vapoursynth knowledge could comment.

sbruder commented 1 year ago

Thanks for opening this, I wanted to say something about this, but never came around to actually do it. First, I’m sorry for not being able to maintain this and leaving this in the state it is in currently. I added many plugins when I used vapoursynth quite a bit but now I sadly don’t have much time for vapoursynth and maintaining this overlay.

As I introduced quite a few of the packages in this overlay, it’s my “responsibility” (at least I feel like it is, because I wrote that code in the first place) to take care of them, so if you have any questions about a specific thing, don’t hesitate to reach out to me. However, as you can probably see from my low activity (not only) in this project, I don’t know if I’ll be able to help you quickly (because most things in here are quite complex, so I have to think about them for some time).

I am definitely interested in making sure that vapoursynth in Nix has good infrastructure because its sheer amount of plugins with different variants makes it a prime candidate for packaging with Nix.

I’ll quickly address your points, though that is by no means an exhaustive discussion, just what I think of it right now:

The biggest problem I noticed while trying to use the overlay is that alot of stuff is outdated or missing and probably needs active maintaining to not break when you want to update a single component (especially for stuff from Irrational-Encoding-Wizardry)

In general that is what we want, though many plugin updates have breaking changes, so a way to use older plugin versions would be great (though probably not that easy without evaluating multiple versions of the overlay).

There is seems to be no proper care for versioning in the vapoursynth space i.e alot of packages just have fixes in master but there is no release, but stuff depends on them. : Whats the best course of action ignore release and always take a unstable commit?

I think addressing this requires cooperation from upstream (maybe some aren’t aware that not versioning causes problems?). Until upstream versions packages, the only thing we can do (AFAIK) is using the latest commit.

There should also be a proper checkPhase for binary plugins that checks if the plugin shows up. : I have alread programmed this but I dont't like the implementation

That’s right, ideally this could be done as a hook (like how pythonImportsCheck works).

There is also the chance for some random breakage due to nixos updating (for example vapoursynth updates, I had alot of trouble with ncnn and glslang) : Maybe some hydra thing sbruder mentioned, or just someone that checks every few week if everything still builds on nixos-unstable

Yes, Hydra would be nice for ensuring compatibility with the latest nixpkgs version, though I don’t know that much about hydra and if the “legacy” (non-flakes) method will still be supported (or how to implement that with flakes, which normally pin the inputs).

There is also alot of redundant copy paste in the .nix package files, should these be refactored out or would that lead to even more problems (python checkPhase, meson install dir, requirements.txt vapoursynth removal)

I also though about this but because of the different ways the plugins can be built, I couldn’t figure out how to abstract this (some are plain python, some are python modules with native code, some are just native code (C/C++), some are written in Rust, …).

And the whole python testPhase binaryPlugin/pythonPlugin seperation problem.

That’s a very complex problem indeed. Every possible way I could think of (as it is right now, multiple outputs, multiple packages, differentiation in the wrapper) has some downsides, especially because python plugins can depend on native plugins.

: In my opinion it would be best to transition to a model where all vapoursynth binaryPlugins are collected in some env variable (like PATH or PYTHONPATH) instead of having to build a extra withPlugins derivation with python and vapoursynth stuff combined.

I think the withPlugins mechanism is a good way in principle (many packages in nixpkgs are doing it that way), though a separation is probably necessary. But if there is a way to avoid the problems by not using the withPlugins mechanism, I won’t be opposed to it.

I would also propose some external "integration?" test than just checks if filters run and dont't throw errors. We currently can't catch missing dependencies if their just used in a function or from binary plugin

That could be done by adding a derivation that runs the tests to passthru.tests (the testing mechanism could be abstracted).

I have wrote quite a few packages and utilities now, would anyone be interested in me cleaning my stuff up and trying to upstream them here? (My plan is to be on parity with vapoursynth-portage and AUR)

As already said, I probably won’t have the time to thoroughly review them, though it definitely would be great to have an alternative vapoursynth distribution to the de facto standard (AUR).

There were also a few things that I wasn't able to package that whould be nice to have like (https://github.com/Irrational-Encoding-Wizardry/yuuno)

I also tried packaging that once, but gave up because I didn’t fully understand jupyter (and to an extent python) infrastructure in nixpkgs.

sshiroi commented 1 year ago

Thanks for the quick response, I'm definitely gonna look into how the pythonImportsCheck works. Writing test with passthru.tests also seems like something that I'd be able to just make work, but I don't think that would that be useful at this stage. I will report back once I made meaningful progress on something.

sshiroi commented 1 year ago

A few more things I wanted to add

To address a few things I mentioned in my original message:

  1. first split up derivations that are binary and python in one into two and make the python one depend on the binary one
  2. and in checkPhase recurse into every propagatedBuildInput and remove everything python releated passing that to a custom withPlugins function that only takes those as is
tadeokondrak commented 1 year ago

Hi - I merged a few PRs recently since they seemed unlikely to break anything, and I couldn't find any issues just reading through the diff. Ideally, it'd be nice to build and test them before merging, but my thinking was that a more up-to-date but possibly more unstable repo would be more useful than a dead one, especially considering flake lockfiles.

What do you think about that, @sbruder?

Also, since I don't use vapoursynth much anymore, maybe this repository is a good candidate to try to transfer to the nix-community organization. I could look into that.

sbruder commented 1 year ago

Yes, I also think that because users of the overlay can always pin it to a certain revision and should do that anyway (because of breaking changes in vapoursynth plugins).

compguy284 commented 1 year ago

@sshiroi since you have disabled issues on your fork I figured I would message you here. Since you are overriding glslang to version 1.3.216.0 it now fails to compile now that NixOS/nixpkgs has added patches for 1.3.231.0

sshiroi commented 1 year ago

@compguy284 I only infrequently update nixos-unstable so thanks for pointing it out. I have removed the overriden version and tested rife and w2x and they seem to work. I have also enabled issues (github apparently disables them by default on forks?) so please feel free to report any future problems.

I also noticed that vsgan broke and fixed that too, but I'm not sure becauase I don't know poetry. This breakage also seems to affect this repo. @aidalgol

aidalgol commented 1 year ago

I also noticed that vsgan broke and fixed that too, but I'm not sure becauase I don't know poetry. This breakage also seems to affect this repo.

I have hit this breakage myself, but I have not yet had the time to properly investigate.

aidalgol commented 1 year ago

I also noticed that vsgan broke and fixed that too, but I'm not sure becauase I don't know poetry. This breakage also seems to affect this repo.

@sshiroi I have a fix for vsgan in #18.

tadeokondrak commented 1 year ago

Regarding the repo/permissions: I've transferred vs-overlay to nix-community. @sbruder, you should have an invite pending. @aidalgol, would you like to be added as a committer here?

aidalgol commented 1 year ago

Sure!

sshiroi commented 1 year ago

Latest nixpkgs has checkInputs -> nativeCheckInputs. In this repo atleast acsuite and getnative is affected.

aidalgol commented 1 year ago

@sshiroi That sounds significant enough to warrant its own issue.

sshiroi commented 1 year ago

I just came across this when updating my fork. Fixing build is easally achieved by putting ffmpeg and imagemagick in nativeCheckInputs. https://github.com/sshiroi/vs-overlay/commit/6a31f0daf7792ee2faada8d276a0bada00403f1a

I now also noticed that even if thats fixed this getnative version will still be broken here because latest vapoursynth r60 removed deprecated functions (which is already fixed in upstream getnative). You can create an issue if you want but, I didn't because I'm in a weird situation where I mainly use my fork which has drifted far from upstream that I can't event PR stuff back, so I just though I notify when stuff breaks for me that I know also breaks here.

checkInputs have been renamed to nativeCheckInputs, because they behave the same as nativeBuildInputs when doCheck is set. checkInputs now denote a new type of dependencies, added to buildInputs when doCheck is set. As a rule of thumb, nativeCheckInputs are tools on $PATH used during the tests, and checkInputs are libraries which are linked to executables built as part of the tests. Similarly, installCheckInputs are renamed to nativeInstallCheckInputs, corresponding to nativeBuildInputs, and installCheckInputs are a new type of dependencies added to buildInputs when doInstallCheck is set. (Note that this change will not cause breakage to derivations with strictDeps unset, which are most packages except python, rust and go packages).