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

speechd: introduce minimal variant, use it everywhere, inject big variant impurely #329658

Merged
merged 5 commits into from
Jul 26, 2024

Conversation

K900
Copy link
Contributor

@K900 K900 commented Jul 24, 2024

Description of changes

TODO: figure out how to default this to false without breaking people.

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.

@K900 K900 changed the base branch from master to staging-next July 26, 2024 13:07
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jul 26, 2024
@K900 K900 changed the title speechd: introduce minimal variant, use it everywhere speechd: introduce minimal variant, use it everywhere, inject big variant impurely Jul 26, 2024
mweinelt and others added 2 commits July 26, 2024 16:11
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.

Co-authored-by: Martin Weinelt <[email protected]>
@K900 K900 requested a review from davidak July 26, 2024 13:14
@K900 K900 marked this pull request as ready for review July 26, 2024 13:14
Copy link
Member

@mweinelt mweinelt left a comment

Choose a reason for hiding this comment

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

Thanks, i think that looks worth a try.

@vcunat vcunat merged commit b8688ce into NixOS:staging-next Jul 26, 2024
7 of 9 checks passed
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this pull request Jul 27, 2024
Incidentally this also fixes tests like nixosTests.installer.simple
/cc PR NixOS#329658
@h7x4 h7x4 added the 8.has: module (new) This PR adds a module in `nixos/` label Jul 27, 2024
vcunat added a commit to vcunat/nixpkgs that referenced this pull request Jul 28, 2024
Incidentally this also fixes tests like nixosTests.installer.simple
/cc PR NixOS#329658
@vcunat
Copy link
Member

vcunat commented Jul 28, 2024

Now that I think of it, I'm not sure if services.speechd is the best place, maybe programs.speechd?

I don't care really. But now when it's new (not even on master yet) such changes are easiest. So I'm just posting this note...

@K900
Copy link
Contributor Author

K900 commented Jul 28, 2024

It is conceptually a service, it's just started weird currently. The upcoming next release will be a proper systemd socket activated thing.

@vcunat
Copy link
Member

vcunat commented Jul 28, 2024

Oh, OK. BTW, another way of saving space would be to default to English only (and perhaps just a single voice) unless you override your i18n.supportedLocales or something. That looks like ~7 MB only.

@NobbZ
Copy link
Contributor

NobbZ commented Dec 12, 2024

Something in the changelog would have been nice… Also what are the consequences of force disabling it? I used my system for severel years without it, and have no clue how I had to trigger it right now…

@K900
Copy link
Contributor Author

K900 commented Dec 12, 2024

It's perfectly fine to disable it if you don't need text to speech.

@NobbZ
Copy link
Contributor

NobbZ commented Dec 12, 2024

And why did it get enabled by default without a mention in the changelog?

@tejing1
Copy link
Contributor

tejing1 commented Dec 12, 2024

Which more than 99% of people don't... so why is it enabled by default for everybody?

@K900
Copy link
Contributor Author

K900 commented Dec 12, 2024

Because accessibility is important. It should have definitely had a release note, though.

@NobbZ
Copy link
Contributor

NobbZ commented Dec 12, 2024

Yes, accessibility is important, I agree, though so far there is some random envvar set without any documentation of how to use it nor what programs can actually use it.

The closure doesn't seem to be large in its effect on my system, I still consider the way it gets introduced worth questioning.

@K900
Copy link
Contributor Author

K900 commented Dec 12, 2024

That environment variable gets picked up by libspeechd in applications. The whole thing was actually an attempt to reduce closure size - previously you'd just get speechd as a single package, including the gigabytes of voice data; now individual applications only depend on the library, and the daemon and voice data are separate and late bound, so you can disable them.

@tejing1
Copy link
Contributor

tejing1 commented Dec 12, 2024

So it's enabled for everybody because otherwise people who used it the way it worked before would have it break?

@K900
Copy link
Contributor Author

K900 commented Dec 12, 2024

Indeed, and also because people that rely on screenreaders should be able to use the stock configuration on the installer and on the default system.

@SuperSandro2000
Copy link
Member

You get rid of it with:

services.orca.enable = false;
services.speechd.enable = false;

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Dec 12, 2024

I think it would have been better to introduce an accessibility top-level option to centralize the default configuration of programs like brltty, orca or speechd. This is yet another undocumented default that's hard to track down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: qt/kde 8.has: module (new) This PR adds a module in `nixos/` 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 11-100 10.rebuild-linux: 501-1000 10.rebuild-linux: 501+
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants