-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
davinci-resolve and davinci-resolve-studio: fix panel support and add missing applications #370719
base: master
Are you sure you want to change the base?
Conversation
This all seems sane and we greatly appreciate the work @httpdev I'll have to do some testing when I have time but I'm glad there is someone with the physical davinci panels and knows nix. That vent diagram is quite small haha |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution. Please read the contributing guide, in particular the section on commit conventions. The commits here need to be renamed to match convention, as well as be atomic and semantic. The PR is a bit difficult to review as-is, as there are a lot of redundant commits, and not all commits are explained by their commit message, so it’s challenging to understand why some things are being changed.
I added some inline comments but stopped quickly as it was, as above, challenging to follow along.
TL;DR: Please reorganize so commits are smaller, discrete changes that are independently useful, rather than having commits make many unrelated changes. Also avoid having commits that only fix issues with earlier ones—instead amend-in the changes. There’s a lot of detail in the PR description—instead much of that should be in the discrete commit messages so it’s easy to follow and link the why to the what.
Thanks for the review. I understand, this is my first contribution to nixpkgs, so I'm not used to the conventions. I'm not sure how to split into independent commits. There are basically two parts: supporting the other utilities, and fixing the panel udev issues. Supporting the other utilities requires separating out the FHS environment into a separate derivation, which needs some reorganization. Supporting the panel setup tool without fixing the panel udev issues doesn't make sense, and fixing the udev issues and then reorganizing them anyway in the next commit also doesn't seem useful. |
715ac9a
to
05812da
Compare
Hello, I have restructured the pull request. When requesting further changes, I kindly ask to consider that debugging the hardware issues and missing dependencies alone was more than 30 hours of work. |
42fdf68
to
5808012
Compare
Refactors the FHS environment into a reusable wrapper to prepare for supporting the currently unavailable tools supplied in the binary distribution file. Adds a davinci-resolve-*-shell command which wraps a shell inside the FHS for scripting and debugging. Adds corresponding documentation in longDescription.
fixes NixOS#366384 fixes NixOS#364991 possible fix for NixOS#364646 and NixOS#168789 (untested) The udev rules now follow the same naming and priority as when installing Resolve on Non-NixOS. These files were formerly mapped from the unpacked archive (99-*.rules), but in the Resolve install script they are actually modified and a new file is added for the dongle, which results in 75-davincipanel.rules, 75-davincikb.rules, and 75-sdx.rules (new). Rules are in etc/udev/rules.d. Expanded longDescription to hint users to the fact that the package needs to be added to services.udev.packages in order for the panels to work, which was not documented.
This adds the mimeTypes, startupNotify and terminal settings to the desktop item as in the original binary distribution.
This adds the control panels setup dependencies, wrapper, and desktop item.
This adds the remote monitor wrapper and desktop item.
This adds the speed test application wrapper and desktop item.
This commit improves code readability by replacing expressions involving optionalString with ${davinci.pname} and ${product}.
The applications shipped in the binary distribution come with their own sets of dynamic libraries. This commit introduces two additional options in mkWrapper that allow to specify the library path and QT plugin directory for each application individually instead of using the libraries provided by the main executable.
This commit adds a wrapper and desktop items to support the Blackmagic Design RAW Player.
This commit adds the .xml files with the MIME type definitions and corresponding icons for DaVinci Resolve and Blackmagic RAW files.
880cddd
to
eb5569d
Compare
Nevermind, I have found some more time to work on this. It was a bit tricky because the binary distribution is a mess - it consists of several binaries which each come with their own (incompatible) .so libraries. This required a slight modification to the wrapper so they can each live in their own happy space. The RAW player is now working and all MIME types are correctly added with their icons. I have also added a command I have edited the top comment to reflect the changes. |
eb5569d
to
99686da
Compare
This commit adds Python scripting support. It sets the required paths in the FHS environment and creates a "davinci-resolve-*-python" wrapper. It also adds an additional "pythonPackage" input which can be used to override the Python package and documentation. Note that external scripting appears to be supported only by the non-free Studio version.
99686da
to
ac07922
Compare
This commit fixes #366384, #364991, #364646, and possibly (untested) #168789.
It also adds several applications that are bundled with Resolve but were not supported yet:
This required some refactoring, so I am submitting this PR for discussion and expect that it would require some changes before accepting.
Changes:
share/panels/dvpanel-framework-linux-x86_64.tgz
intolib
so that they can be found from within the FHS environment.bin/resolve
viarunScript
(discarding command line arguments). This did not allow to run the other supplied applications in the same FHS, therefore it now functions as a wrapper that can be called with the command+arguments to run. For debugging, the package also has abin/davinci-resolve-*-fhs
command, which starts a shell in the FHS environment.symlinkJoin
to have better control of which files are linked where. The main package only includes the application links inbin/
, udev rules inetc/udev/rules.d/
, and desktop items inshare/applications/
.udev
rules now follow the same naming and priority as when installing Resolve on Non-NixOS. These files were formerly only mapped from the unpacked archive (99-*.rules
), but in the Resolve install script they are actually modified and a new file is added for the dongle, which results in75-davincipanel.rules
,75-davincikb.rules
, and75-sdx.rules
(new).mimeTypes
set.longDescription
is added to hint to the fact that the package needs to be added toservices.udev.packages
in order for the panels to work, which was not documented, and includes comments about scripting.I have tested using a DaVinci Resolve Micro Panel and DaVinci Resolve Speed Editor, which work including updating the firmware using the Control Panels Setup application. I do not have access to panels with a display, so I am not sure if they require the supplied daemon, which would require additional setup.
External scripting works with DaVinci Resolve Studio. Icons are correctly displayed on GNOME, Resolve project files (.drp) are correctly opened by Resolve, and RAW file (.braw) open the RAW player (see the three clips that can be downloaded on this page for testing).
I have tested the automatic update script and it still works as far as I can tell.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.