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

mbrola-voices: extract from mbrola, allow language selection #329650

Closed
wants to merge 4 commits into from

Conversation

mweinelt
Copy link
Member

@mweinelt mweinelt commented Jul 24, 2024

Allow filtering voices based on any number of extglob patterns.

Description of changes

Restricting mbrola-voices to only one voice per language saves 387M, and is what I recommend we should do on the ISO, because it likely doesn't compromise accessbility too much.

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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

@vcunat
Copy link
Member

vcunat commented Jul 24, 2024

kept as a fixed-output derivation to limit "duplicates"

I wonder if it's worth the effort to make the default one a FOD again.

For the files that often remain in people's closures. I think it could make a significant difference for some people, unless they have nix.settings.auto-optimise-store = true; or filesystem-level deduplication.

@mweinelt
Copy link
Member Author

I wonder if it's worth the effort to make the default one a FOD again.

Defaulting to the derivation source now for mbrola, so the default is effectively that again.

@vcunat
Copy link
Member

vcunat commented Jul 24, 2024

I think that piece of code doesn't work as intended? Maybe because of the self-reference. I still see this in the default closure

/nix/store/8ja11xcr6dbvh1bfc0p3ny79jdwwgvmg-mbrola-voices-0-unstable-2020-03-30

@vcunat
Copy link
Member

vcunat commented Jul 24, 2024

Maybe we could write mbrola-voices like this?

if languages == [ ] then src // { inherit meta; } else stdenv.mkDerivation { inherit src meta; /*...*/ }

Allow filtering voices based on any number of extglob patterns.
This reduces the size of mbrola-voices by 387M (647M -> 260M) for all
installers that ship with speechd, to make sure that they fit the output
limits of hydra while not compromising too much on accessbility.
Otherwise we got just "source" as the name, which was ugly.
@vcunat
Copy link
Member

vcunat commented Jul 25, 2024

🤔 the overlay approach won't be a good longer-term solution, as it really needs lots of rebuild work to get the ISOs (every time a rebuild happens). Users would be slightly impacted on installation from that media, as hashes of those packages would differ, so they couldn't be pulled from the ISO.

Using the reduced set of voices by default... maybe, I'm no good with accessibility. Decoupling the data and plugging it by some environment variable is probably the best shot for longer-term solutions, though it's "less pure". Maybe this WIP PR: #329658

@vcunat
Copy link
Member

vcunat commented Jul 25, 2024

For the immediate unblocking I increased the limit on Hydra a bit, so that we have some time to improve the closure sizes.

EDIT: here https://hydra.nixos.org/build/267286447#tabs-buildsteps

@vcunat vcunat removed the 1.severity: channel blocker Blocks a channel label Jul 25, 2024
vcunat added a commit to NixOS/infra that referenced this pull request Jul 25, 2024
This is primarily a temporary measure; see here and around:
NixOS/nixpkgs#329650
@vcunat
Copy link
Member

vcunat commented Jul 26, 2024

Superseded by #329658

@vcunat vcunat closed this Jul 26, 2024
@vcunat
Copy link
Member

vcunat commented Jul 26, 2024

At least I think we're not missing anything now.

@mweinelt mweinelt deleted the mbrola-diet branch July 26, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants