aspellDicts: add more dictionaries and some documentation #34798

oxij · 2018-02-10T02:32:37Z

Motivation for this change

Jargon files.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Fits CONTRIBUTING.md.

grahamc · 2018-02-10T02:34:43Z

pkgs/development/libraries/aspell/dictionaries.nix

+    fullName = "English Computer Jargon";
+
+    src = fetchurl {
+      url = https://mrsatterly.com/spelling_computer.txt;


this redirects

oxij · 2018-02-10T03:02:03Z

this redirects

Fixed, I hope.

grahamc · 2018-02-10T03:13:05Z

@GrahamcOfBorg eval

FRidh · 2018-02-10T08:28:08Z

pkgs/development/libraries/aspell/dictionaries.nix

+
+    langInputs = [ en ];
+
+    phases = [ "preBuild" "buildPhase" "installPhase" ];


In both cases you set phases, why? Should this be improved in buildDict?

oxij · 2018-02-10T14:11:26Z

In both cases you set `phases`, why? Should this be improved in `buildDict`?

s/you/the author/ I wondered myself, but now it seems that it is by design. `preBuild` simply defines a bunch of functions, `buildPhase`s and `installPhases`s are different for different derivations, other default phases would either fail or are useless. But its true I can just move it to `buildDict`. Done. Also made another trivial change.

FRidh · 2018-02-10T14:15:07Z

pkgs/development/libraries/aspell/dictionaries.nix

+  buildDict =
+    {langInputs ? [], ...}@args:
+    buildLang ({
+      propagatedUserEnvPackages = langInputs;


We should not be using propagatedUserEnvPackages

FRidh · 2018-02-10T14:15:55Z

pkgs/development/libraries/aspell/dictionaries.nix

+
+     ~~~~
+     environment.systemPackages = [
+       aspellDicts.en


and aspell itself?

FRidh · 2018-02-10T14:16:40Z

pkgs/development/libraries/aspell/dictionaries.nix

+
+with lib;
+
+/* HOWTO:


This only considers the method where dicts are installed separate from aspell, and not aspellWithDicts. Perhaps there is a better place for this comment? Otherwise, it's good to have the explanation!

FRidh · 2018-02-10T14:17:00Z

pkgs/development/libraries/aspell/dictionaries.nix

+     ~~~~
+     master en_US
+     extra-dicts en-computers.rws
+     add-extra-dicts en_US-science.rws


Why are these needed? Should it not detect them?

FRidh · 2018-02-10T14:20:41Z

pkgs/development/libraries/aspell/dictionaries.nix

    shortName = "ca-2.1.5-1";
    fullName = "Catalan";
    src = fetchurl {
-      url = mirror://gnu/aspell/dict/ca/aspell6-ca-2.1.5-1.tar.bz2;
+      url = "mirror://gnu/aspell/dict/ca/aspell6-ca-2.1.5-1.tar.bz2";


Quotes are not needed, and until the policy changes, they should not be included either.

That's super annoying when upgrading a literal URL to a ${version} or ${name} one. Has this policy finally been written down somewhere?

FRidh · 2018-02-10T14:26:34Z

pkgs/development/libraries/aspell/dictionaries.nix

-
-  ca = buildDict {
+  /* Function to compile txt dict files into Aspell dictionaries. */
+  buildDict =


How about creating instead a src that can be consumed by the old buildDict? Any reason for not doing that?

FRidh · 2018-02-10T14:27:20Z

pkgs/development/libraries/aspell/dictionaries.nix

     build in the exact same way. */
-  buildDict =


Any motivation for changing the name?

oxij · 2018-02-10T15:26:38Z

We should not be using `propagatedUserEnvPackages`

Any suggestions with what to replace it with? You should have `en_US` in the profile when installing `en_US-science`.

and aspell itself?

Fixed.

This only considers the method where dicts are installed separate from aspell, and not `aspellWithDicts`. Perhaps there is a better place for this comment? Otherwise, it's good to have the explanation!

The problem with `aspellWithDicts` is that it does `--set ASPELL_CONF` which breaks the above explanation. You would need to implement `aspellWithDictsAndConfig` to make this work. This is outside of scope of this PR.

Why are these needed? Should it not detect them?

They are "extra", opt-in.

Quotes are not needed, and until the policy changes, they should not be included either.

Official source on that claim? I'm motivated by #27809.

How about creating instead a `src` that can be consumed by the old `buildDict? Any reason for not doing that?

Pack files into an archive just to unpack them at the next beta-reduction? Seems crazy to me.

Any motivation for changing the name?

Right. Fixed.

oxij · 2018-02-11T15:54:05Z

Well whatever, the last patch has gone to SLNOS.

To make my position clear: I see no policy behind #27809. What I see is somebody making a huge change to nixpkgs that goes backwards and you merging it without discussion.

FRidh · 2018-02-11T20:20:41Z

Any suggestions with what to replace it with? You should have en_US in the
profile when installing en_US-science.

The closest would be aspellWithDicts, for installing individual packages there is nothing else.

The problem with aspellWithDicts is that it does --set ASPELL_CONF which breaks the above explanation.

See #34856.

You would need to implement aspellWithDictsAndConfig to make this work. This is outside of scope of this PR.

Agreed.

oxij · 2018-02-12T01:10:44Z

The closest would be `aspellWithDicts`, for installing individual packages there is nothing else.

Right. But if I removed the propagation now it would break plain `systemPackages` installs. Unless you put a check into `systemPackages` that rejects dictionaries I think the current impl is the correct one.

> The problem with `aspellWithDicts` is that it does `--set ASPELL_CONF` which breaks the above explanation. See #34856.

Looks ok to me, needs extensive testing. I propose we merge this first as it is well-tested (a whole year of fermentation in SLNOS) and makes things strictly better. You can always change the docstrings later when `aspellWithDicts` becomes awesome.

7c6f434c · 2018-02-19T14:24:43Z

OK, looks like this version is pure addition.

grahamc reviewed Feb 10, 2018

View reviewed changes

GrahamcOfBorg added 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 labels Feb 10, 2018

oxij force-pushed the pkgs/aspell-dicts branch from d04939a to 7c79305 Compare February 10, 2018 02:58

GrahamcOfBorg added the 8.has: package (new) label Feb 10, 2018

grahamc mentioned this pull request Feb 10, 2018

Tags: support 8.has: package (new) NixOS/ofborg#63

Closed

FRidh reviewed Feb 10, 2018

View reviewed changes

oxij force-pushed the pkgs/aspell-dicts branch from 9d0f039 to 10a3068 Compare February 10, 2018 14:06

FRidh requested changes Feb 10, 2018

View reviewed changes

oxij force-pushed the pkgs/aspell-dicts branch from 10a3068 to fbb9d9b Compare February 10, 2018 15:25

oxij force-pushed the pkgs/aspell-dicts branch 2 times, most recently from fe141f7 to 0f8fe1a Compare February 11, 2018 15:50

oxij mentioned this pull request Feb 11, 2018

pkgs: refactor needless quoting of homepage meta attribute #27809

Merged

8 tasks

aspellDicts: add more dictionaries and some documentation

3f9ada0

oxij force-pushed the pkgs/aspell-dicts branch from 0f8fe1a to 3f9ada0 Compare February 18, 2018 12:59

7c6f434c merged commit cfa3e7e into NixOS:master Feb 19, 2018

FRidh mentioned this pull request Jul 10, 2018

Deprecate propagatedUserEnvPkgs #43049

Closed

oxij deleted the pkgs/aspell-dicts branch September 8, 2018 22:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aspellDicts: add more dictionaries and some documentation #34798

aspellDicts: add more dictionaries and some documentation #34798

oxij commented Feb 10, 2018

grahamc Feb 10, 2018

oxij commented Feb 10, 2018 via email

grahamc commented Feb 10, 2018

FRidh Feb 10, 2018

oxij commented Feb 10, 2018 via email

FRidh Feb 10, 2018

FRidh Feb 10, 2018

FRidh Feb 10, 2018

FRidh Feb 10, 2018

FRidh Feb 10, 2018

7c6f434c Feb 19, 2018

FRidh Feb 10, 2018 •

edited

FRidh Feb 10, 2018

oxij commented Feb 10, 2018 via email

oxij commented Feb 11, 2018

FRidh commented Feb 11, 2018

oxij commented Feb 12, 2018 via email

7c6f434c commented Feb 19, 2018


		langInputs = [ en ];

		phases = [ "preBuild" "buildPhase" "installPhase" ];

aspellDicts: add more dictionaries and some documentation #34798

aspellDicts: add more dictionaries and some documentation #34798

Conversation

oxij commented Feb 10, 2018

Motivation for this change

Things done

Choose a reason for hiding this comment

oxij commented Feb 10, 2018 via email

grahamc commented Feb 10, 2018

Choose a reason for hiding this comment

oxij commented Feb 10, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FRidh Feb 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxij commented Feb 10, 2018 via email

oxij commented Feb 11, 2018

FRidh commented Feb 11, 2018

oxij commented Feb 12, 2018 via email

7c6f434c commented Feb 19, 2018

FRidh Feb 10, 2018 •

edited