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.

Sign up for GitHub

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

Jump to bottom

NDI6 SDK + DistroAV obs-studio plugin #368924

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

NDI6 SDK + DistroAV obs-studio plugin #368924

wants to merge 3 commits into from
+251 −0

Conversation

globule655
Copy link

ndi-6: https://ndi.video/tech/ndi6/
DistroAV: https://github.com/DistroAV/DistroAV

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Dec 28, 2024
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Dec 28, 2024
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Dec 28, 2024
@SigmaSquadron
Copy link
Contributor

Welcome to Nixpkgs! The official formatter for Nix code in Nixpkgs is nixfmt. You can format your files by running nix-shell in the Nixpkgs root directory and running nixfmt /path/to/your/changed/files

SigmaSquadron
SigmaSquadron reviewed Dec 29, 2024
View reviewed changes
Copy link
Contributor

@SigmaSquadron SigmaSquadron left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall! Don't forget that the commits need to be squashed per the Commit Conventions.

pkgs/applications/video/obs-studio/plugins/distroav/default.nix Outdated Show resolved Hide resolved
pkgs/applications/video/obs-studio/plugins/distroav/default.nix Outdated Show resolved Hide resolved
pkgs/applications/video/obs-studio/plugins/distroav/default.nix Show resolved Hide resolved
pkgs/applications/video/obs-studio/plugins/distroav/default.nix Outdated Show resolved Hide resolved
pkgs/applications/video/obs-studio/plugins/distroav/default.nix Outdated Show resolved Hide resolved
pkgs/by-name/nd/ndi-6/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/nd/ndi-6/package.nix Show resolved Hide resolved
pkgs/by-name/nd/ndi-6/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/nd/ndi-6/package.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
@github-actions github-actions bot added 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Dec 29, 2024
@globule655
Copy link
Author

Thanks a lot for the review and great suggestions !
I think I fixed all the things pointed out and squashed the commits.
Let me know if there is anything else that needs to be changed

@SigmaSquadron
Copy link
Contributor

SigmaSquadron commented Dec 29, 2024 •
edited
Loading

Good work! Just one thing: the commits were not fully squashed (you need to force push your branch after squashing). There should be three commits:

  1. maintainers: add globule655 which only adds your maintainer entry to maintainer-list.nix.
  2. obs-studio-plugins.distroav: init at 6.0.0 which only adds the distroav package.
  3. ndi-6: init at 6.1.1 which only adds the ndi-6 package.

All of the fixes should be squashed into the two latter commits.

@globule655
Copy link
Author

globule655 commented Dec 29, 2024 •
edited
Loading

Good work! Just one thing: the commits were not fully squashed (you need to force push your branch after squashing). There should be three commits:

  1. maintainers: add globule655 which only adds your maintainer entry to maintainer-list.nix.
  2. obs-studio-plugins.distroav: init at 6.0.0 which only adds the distroav package.
  3. ndi-6: init at 6.1.1 which only adds the ndi-6 package.

All of the fixes should be squashed into the two latter commits.

Whoops my bad. Fixed now, only 3 commits.
Thanks again

@SigmaSquadron SigmaSquadron added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 29, 2024
@globule655
Copy link
Author

I just changed a little thing with the substitueInPlace command.
After thinking about it I thought it best to use --replace-fail as the plugin won't work if no changes are made by the command

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes labels Dec 29, 2024
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 30, 2024
@globule655
Copy link
Author

Question to anybody in the know :
When checking the ofborg tests logs I see the following error
Cannot nix-instantiate `obs-studio-plugins.distroav.passthru.tests` because: error: attribute 'tests' in selection path 'obs-studio-plugins.distroav.passthru.tests' not found

My understanding of the passthru.tests function is that it checks that the other package can be built with the current one as a dependency or something like that.
e.g : Can I build obs-studio-plugins.distroav which depends on ndi-6 ?

So here as it's failing I'm wondering 2 things :

  • Did I understand incorrectly how I should write this ? I saw this test written exactly like that for other packages so I figured it was okay
  • Is that test really necessary in the end ? I guess it's nice to have but can be omitted without much risk

Cheers for the new year's eve 🍻

@globule655
maintainers: add globule655
eba050b
@globule655
obs-studio-plugins.distroav: init at 6.0.0
24ff7ae 
This Version has major & large code changes.Starting v6 the plugin has
been re-branded DistroAV from OBS-NDI.
https://github.com/DistroAV/DistroAV/releases/tag/6.0.0

"obs-studio-plugins.distroav: correct file formatting"

"obs-studio-plugins.distroav: fixed fmt issue by adding tag"

obs-studio-plugins.distroav: run nixfmt on the file to fix formatting

osb-studio-plugins.distroav: changed lib reference to ndi-6

obs-studio-plugins.distroav: fix ndi-6 reference
@globule655
Copy link
Author

Removed the passthru.tests directive for now as after reading the docs I didn't think it was something absolutely necessary and caused the pipelines check to hang

@SigmaSquadron
Copy link
Contributor

Removed the passthru.tests directive for now as after reading the docs I didn't think it was something absolutely necessary and caused the pipelines check to hang

Don't worry about CI. OfBorg is dead, and it won't be running any tests anytime soon. Please re-add the tests if they weren't broken.

@globule655
ndi-6: init at 6.1.1
92c9f25 
Major update from existing 4.x package.
Compatible with DistroAV obs-studio plugin
https://docs.ndi.video/all/getting-started/release-notes#ndi-6.1.1
@globule655
Copy link
Author

Removed the passthru.tests directive for now as after reading the docs I didn't think it was something absolutely necessary and caused the pipelines check to hang

Don't worry about CI. OfBorg is dead, and it won't be running any tests anytime soon. Please re-add the tests if they weren't broken.

Hey and thanks for the clarification !
I put back the test then as it was all fine, only the pipeline hanging made me doubt

Cheers

@globule655
Copy link
Author

Hi there,
Anything else needed from me for this to be merged ?
What are the next steps ?

Thanks a lot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SigmaSquadron SigmaSquadron SigmaSquadron approved these changes

Assignees
No one assigned
Labels
8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@globule655 @SigmaSquadron @wegank @NixOSInfra