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

open-jtalk: init at 1.11 #203607

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

open-jtalk: init at 1.11 #203607

wants to merge 5 commits into from
+140 −0

Conversation

Yorwba
Copy link

@Yorwba Yorwba commented Nov 29, 2022

Description of changes

Open JTalk is a Japanese text-to-speech system built on top of the hidden Markov model-based HTS engine.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@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 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Nov 29, 2022
@Janik-Haag Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023
@lolbinarycat
Copy link
Contributor

looks good to me

@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict This PR has merge conflicts with the target branch labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
@donovanglover
Copy link
Member

Closing due to inactivity. Feel free to re-open and rebase or create a new PR if this is still relevant to you.

@Yorwba
Copy link
Author

Yorwba commented Feb 7, 2025

I rebased on latest master and fixed the merge conflicts, but I'm not seeing an option to re-open.

@donovanglover
Copy link
Member

You have to re-open before rebasing. No worries though, we can continue in a new PR.

@donovanglover
Copy link
Member

I think you fixed it and the PR can be re-opened now, so we can also continue here if you want.

@donovanglover donovanglover reopened this Feb 7, 2025
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Feb 7, 2025
@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Feb 7, 2025
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Feb 7, 2025
@github-actions github-actions bot added 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Feb 7, 2025
Yorwba added 4 commits February 7, 2025 19:41
@Yorwba
hts-engine: init at 1.10
9965c14
@Yorwba
open-jtalk: init at 1.11
f8cc7e4
@Yorwba
hts-voice-nitech-jp-atr503-m001: init at 1.05
a9f96e7
@Yorwba
open-jtalk: patch in default dictionary and voice
64e40e3 
Open JTalk is set up to allow the user to supply
their own dictionary and voice files, but the ones
most likely to be used are those published alongside
Open JTalk itself.

Using them as the defaults makes the program a lot
more convenient to use.
@Yorwba
Copy link
Author

Yorwba commented Feb 7, 2025

Thanks to your link, I realized that it's possible to make a rebased PR reopenable again by force-pushing the pre-rebase commit that was associated with the PR when it was closed. Interesting GitHub quirk...

I also adapted to the new formatting and directory structure requirements. Hopefully everything is all right now.

donovanglover
donovanglover reviewed Feb 7, 2025
View reviewed changes
Copy link
Member

@donovanglover donovanglover left a comment

Choose a reason for hiding this comment

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

Great work. Here's my review that focuses on code improvements and questions (I'm not familiar with open-jtalk).

pkgs/by-name/ht/hts-engine/package.nix
fetchurl,
}:

stdenv.mkDerivation rec {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
stdenv.mkDerivation rec {
stdenv.mkDerivation (finalAttrs: {

See https://discourse.nixos.org/t/avoid-rec-expresions-in-nixpkgs/8293

pkgs/by-name/ht/hts-engine/package.nix
version = "1.10";

src = fetchurl {
url = "mirror://sourceforge/hts-engine/hts_engine%20API/hts_engine_API-${version}/hts_engine_API-${version}.tar.gz";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
url = "mirror://sourceforge/hts-engine/hts_engine%20API/hts_engine_API-${version}/hts_engine_API-${version}.tar.gz";
url = "mirror://sourceforge/hts-engine/hts_engine%20API/hts_engine_API-${finalAttrs.version}/hts_engine_API-${finalAttrs.version}.tar.gz";

pkgs/by-name/ht/hts-engine/package.nix

src = fetchurl {
url = "mirror://sourceforge/hts-engine/hts_engine%20API/hts_engine_API-${version}/hts_engine_API-${version}.tar.gz";
sha256 = "sha256-4hMr5YYNj7SkYL52ZFTP18PiHPZ7UJxI4YBP6rFJaPc=";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sha256 = "sha256-4hMr5YYNj7SkYL52ZFTP18PiHPZ7UJxI4YBP6rFJaPc=";
hash = "sha256-4hMr5YYNj7SkYL52ZFTP18PiHPZ7UJxI4YBP6rFJaPc=";

#325892

pkgs/by-name/ht/hts-engine/package.nix
sha256 = "sha256-4hMr5YYNj7SkYL52ZFTP18PiHPZ7UJxI4YBP6rFJaPc=";
};

meta = with lib; {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
meta = with lib; {
meta = {

#292468 (comment)

pkgs/by-name/ht/hts-engine/package.nix
meta = with lib; {
description = "HMM-based speech synthesis engine";
homepage = "https://hts-engine.sourceforge.net/";
license = licenses.bsd3;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
license = licenses.bsd3;
license = lib.licenses.bsd3;

pkgs/by-name/op/open-jtalk/package.nix
Copy link
Member

Choose a reason for hiding this comment

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

Same suggestions apply to this file.

pkgs/by-name/op/open-jtalk/package.nix
./dic-path.patch
./voice-path.patch
];
voice = "${hts-voice-nitech-jp-atr503-m001}/share/hts-voice/nitech_jp_atr503_m001.htsvoice";
Copy link
Member

Choose a reason for hiding this comment

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

Can we support other voices as well?

pkgs/by-name/op/open-jtalk/package.nix
Comment on lines +19 to +22
patches = [
./dic-path.patch
./voice-path.patch
];
Copy link
Member

Choose a reason for hiding this comment

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

Try using substituteInPlace --replace-fail to avoid committing patch files

pkgs/by-name/ht/hts-engine/package.nix
homepage = "https://hts-engine.sourceforge.net/";
license = licenses.bsd3;
maintainers = with maintainers; [ yorwba ];
};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
};
mainProgram = "hts_engine";
};

pkgs/by-name/op/open-jtalk/package.nix
homepage = "https://open-jtalk.sourceforge.net/";
license = licenses.bsd3;
maintainers = with maintainers; [ yorwba ];
};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
};
mainProgram = "open_jtalk";
};

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

@donovanglover donovanglover donovanglover left review comments

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: 1-10 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.
5 participants
@Yorwba @lolbinarycat @donovanglover @wegank @Janik-Haag