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

mpvScripts.autosub: init at 2021-06-29 #356158

Merged
merged 2 commits into from
Nov 27, 2024
Merged

Conversation

octvs
Copy link
Member

@octvs octvs commented Nov 15, 2024

Add mpvScripts.autosub, resolves #336986.

Took the discussion over at the linked issue, and other mpvScripts as basis.
Couple of questions to whom might review this:

  • There are usage of passthru.updateScript on other scripts, what is the best
    practice for this?
  • In a similar manner I saw uses of lib.getExe for dependencies on
    substituteInPlace like here. What is the point of using it rather than a
    direct path?
    • The latter seems more safe (or exact) to me, therefore I opted for using
      it. But I would like to know if there is a best practice and/or the logic
      behind using lib.getExe.

Cheers!

Credits to @jcaesar for supplying an initial version at the discussion

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.

@octvs
Copy link
Member Author

octvs commented Nov 16, 2024

Currently failing test is due to my entry from lib.maintainers missing, which
I expected to be present before this one with this PR already merging.

@AndersonTorres
Copy link
Member

What is the point of using it rather than a
direct path?

Encapsulation.
lib.getExe will pick the executable from outputBin. And outputBin can assume different paths according to the derivation, especially in split outputs.

@octvs
Copy link
Member Author

octvs commented Nov 17, 2024 via email

@octvs
Copy link
Member Author

octvs commented Nov 17, 2024

I've found the answer to the other question here.

pkgs/applications/video/mpv/scripts/autosub.nix Outdated Show resolved Hide resolved
pkgs/applications/video/mpv/scripts/autosub.nix Outdated Show resolved Hide resolved
pkgs/applications/video/mpv/scripts/autosub.nix Outdated Show resolved Hide resolved
@octvs
Copy link
Member Author

octvs commented Nov 17, 2024 via email

@jcaesar
Copy link
Contributor

jcaesar commented Nov 18, 2024

Currently failing test is due to my entry from lib.maintainers missing

Depending on your progress with #354823, you could also cherry-pick the maintainer add commit onto here. It won't cause merge conflicts, and if you rebase either PR after the other is merged, the duplicate commit will silently disappear.

@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Nov 18, 2024
@octvs
Copy link
Member Author

octvs commented Nov 18, 2024 via email

@uninsane uninsane added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 23, 2024
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 23, 2024
@nbraud nbraud added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Nov 27, 2024
@nbraud nbraud merged commit a3a6786 into NixOS:master Nov 27, 2024
36 of 37 checks passed
@octvs octvs deleted the initMpvAutosub branch December 2, 2024 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Package request: mpvScripts.autosub
6 participants