Skip to content

Commit

Permalink
nixos/alsa: Do not make sound.enable conditional on stateVersion.
Browse files Browse the repository at this point in the history
Eelco Dolstra wrote:

Hm, this is not really the intended use of stateVersion. From the description:

        Every once in a while, a new NixOS release may change
        configuration defaults in a way incompatible with stateful
        data. For instance, if the default version of PostgreSQL
        changes, the new version will probably be unable to read your
        existing databases. To prevent such breakage, you can set the
        value of this option to the NixOS release with which you want
        to be compatible. The effect is that NixOS will option
        defaults corresponding to the specified release (such as using
        an older version of PostgreSQL).

So this is only intended for options that have some corresponding on-disk state. AFAICT this is not the case for sound. In any case stateVersion is a necessary evil that only exists because we can't just upgrade Postgres databases or change SSH host keys. It's not necessary for things like whether sound is enabled. (If the user discovers that sound is suddenly disabled, they can just enable it.)

I had some vague recollection that we also had a configVersion option setting to control the defaults for non-state-related options, but I can't find it so maybe it was only discussed.
  • Loading branch information
aristidb committed Feb 23, 2018
1 parent a6664d8 commit e349ccc
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 12 deletions.
10 changes: 5 additions & 5 deletions nixos/doc/manual/release-notes/rl-1803.xml
Expand Up @@ -80,6 +80,11 @@ has the following highlights: </para>
following incompatible changes:</para>

<itemizedlist>
<listitem>
<para>
<literal>sound.enable</literal> now defaults to false.
</para>
</listitem>
<listitem>
<para>
Dollar signs in options under <option>services.postfix</option> are
Expand Down Expand Up @@ -209,11 +214,6 @@ following incompatible changes:</para>
For <literal>stateVersion = "17.09"</literal> or lower the old behavior is preserved.
</para>
<itemizedlist>
<listitem>
<para>
<literal>sound.enable</literal> now defaults to false.
</para>
</listitem>
<listitem>
<para>
<literal>matrix-synapse</literal> uses postgresql by default instead of sqlite.
Expand Down
10 changes: 3 additions & 7 deletions nixos/modules/services/audio/alsa.nix
Expand Up @@ -21,7 +21,7 @@ in

enable = mkOption {
type = types.bool;
defaultText = "!versionAtLeast system.stateVersion \"18.03\"";
default = false;
description = ''
Whether to enable ALSA sound.
'';
Expand Down Expand Up @@ -78,11 +78,7 @@ in

###### implementation

config = mkMerge [
({
sound.enable = mkDefault (!versionAtLeast config.system.stateVersion "18.03");
})
(mkIf config.sound.enable {
config = mkIf config.sound.enable {

environment.systemPackages = [ alsaUtils ];

Expand Down Expand Up @@ -128,6 +124,6 @@ in
];
};

})];
};

}

1 comment on commit e349ccc

@abbradar
Copy link
Member

@abbradar abbradar commented on e349ccc Feb 24, 2018

Choose a reason for hiding this comment

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

It looks very user-unfriendly to me like this -- imagine users which would update their systems and discover that sound is gone. How are they going to find out exactly what happened? Especially users on unstable -- I can see "Sound stopped working" issues in our tracker piling up.

I propose not making this change if we can't make this conditional.

Please sign in to comment.