Skip to content
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

General questions regarding vs-overlay #17

Open
sshiroi opened this issue Dec 12, 2022 · 14 comments
Open

General questions regarding vs-overlay #17

sshiroi opened this issue Dec 12, 2022 · 14 comments

Comments

@sshiroi
Copy link

sshiroi commented Dec 12, 2022

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

  • 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)
  • 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?
  • 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
  • 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
  • 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)
  • And the whole python testPhase binaryPlugin/pythonPlugin seperation problem.
    : 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 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
  • 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)
  • 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 will continue working on these problems myself, but I think it would be best if someone with more nix/vapoursynth knowledge could comment.

@sbruder
Copy link
Collaborator

sbruder commented Dec 12, 2022

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
Copy link
Author

sshiroi commented Dec 12, 2022

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
Copy link
Author

sshiroi commented Dec 14, 2022

A few more things I wanted to add

  • Something I saw on aur is that they also package the readmes, which we should also do. Some software to show them all and search through them would also be nice.
  • I also noticed that when updating some HomeofVapoursynthEvolution packages that they added some kind of lto parameter that makes newer version not show up. This weirdly only seems the case on packages that use avx or sthing like that (for example bwdif and addgrain break with lto but imwri does not). No clue how those two should related. Disabling lto seems to fix all of them though.
  • Tied to that I noticed that most meson buildscript check for x86 by hostmachine which seems to be entirely wrong but I don't think its a big issue even though it does effect alot of packages.
  • Also a big waste is that changes in binary packages forces rebuilds for all dependent packages even though their not needed at buildtime. I don't know if something like "runtime only" dependecys are possible or not in nix, quick googling indicates no. I think we could hack something together with passthru but thats probalby not worth it.

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

  • I'm currently around parity with vapoursynth-portage, which is around ~150 packages, aur is ~250 and vsdb is at 210. I skipped all packages with the source being a dropbox link or sth obscure like that. It even felt useless to package sth. that wasn't updated in like 8years, so I think I'm only going to package stuff that seems to acually still be in use.
  • I've ""cleanly"" abstracted out the most common types of packages (automake,meson,python packages, singular python files) into mK... functions, that removed alot of boilerplate
  • Don't know if my pythonTestPhase fix works (because I don't think there is a test that has a dependecy graph that would hit edgecases) but heres what I though seems to be a ok solution:
  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
Copy link
Collaborator

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
Copy link
Collaborator

sbruder commented Dec 19, 2022

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
Copy link

@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
Copy link
Author

sshiroi commented Dec 26, 2022

@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
Copy link
Collaborator

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
Copy link
Collaborator

aidalgol commented Feb 7, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

Sure!

@sshiroi
Copy link
Author

sshiroi commented Mar 18, 2023

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

@aidalgol
Copy link
Collaborator

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

@sshiroi
Copy link
Author

sshiroi commented Mar 19, 2023

I just came across this when updating my fork. Fixing build is easally achieved by putting ffmpeg and imagemagick in nativeCheckInputs.
sshiroi@6a31f0d

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants