aspell: added new patch data-dirs-from-nix-profiles.patch #30234

roastiek · 2017-10-08T20:45:13Z

Aspell will search for dictionaries in all nix profiles even when used as library.
Setting data-dir or dict-dir in ASPELL_CONF will disable this behavior.

Motivation for this change

The ASPELL_CONF variable is used only by aspell binary so any application using aspell API will never see any dictionaries (like in #28815). Internal code of aspell allows multiple data/dict search paths so is possible to add all lib/aspell subdirectories from all nix profiles, unlike when using ASPELL_CONF.
Any application using libaspell will be able use any dictionary installed without any modification.

I have looked on aspellWithDicts derivation, but I don't think it could solve this problem.

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
- Linux
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

michalrus · 2017-10-08T21:27:37Z

@deedrah, how about this part?

nixpkgs/nixos/modules/programs/environment.nix

Lines 52 to 57 in eb21d19

    
                    unset ASPELL_CONF 
        
                    for i in ${concatStringsSep " " (reverseList cfg.profiles)} ; do 
        
                      if [ -d "$i/lib/aspell" ]; then 
        
                        export ASPELL_CONF="dict-dir $i/lib/aspell" 
        
                      fi 
        
                    done

michalrus · 2017-10-08T21:34:53Z

@deedrah, could you check languages list in gnome3.evolution as reported in #28815?

This PR doesn’t fix it for me. =( Still only Hebrew available there.

FRidh

I have looked on aspellWithDicts derivation, but I don't think it could solve this problem.

It may be good to add a line in the header of the aspellWithDicts expression explaining this limitation.

FRidh · 2017-10-09T16:33:34Z

pkgs/development/libraries/aspell/default.nix

@@ -10,6 +10,7 @@ stdenv.mkDerivation rec {

  patchPhase = ''
    patch interfaces/cc/aspell.h < ${./clang.patch}
+    patch -p1 < ${./data-dirs-from-nix-profiles.patch}


This needs to be an optional patch, because looking for NIX_PROFILES is basically an impurity. See the if [ -z "\$ASPELL_CONF" ]; then in the code that is removed. This is important for aspellWithPackages.

For build-time purity, I agree. For run-time purity, I think it's a bit of a gray area. Taking this purity argument to the extreme would be that e.g. "bash" on NixOS would not see "git" unless you have a "bash = bashWithPackages [ git ]" thing. And I don't think we want that :-)

An alternative could be to modify the patch so that it only reads NIX_PROFILES if NIX_ASPELL_USE_PROFILES is set (or something). Then the patch could be applied unconditionally. (No recompilation needed.)

For build-time purity, I agree. For run-time purity, I think it's a bit of a gray area. Taking this purity argument to the extreme would be that e.g. "bash" on NixOS would not see "git" unless you have a "bash = bashWithPackages [ git ]" thing. And I don't think we want that :-)

That's an impurity that exists by default, and one which is quite useful. Yet, if you want to, you could prevent it by creating a buildEnv and creating wrappers setting e.g PATH.

In this case, the impurity can also be useful, however it is now forced, thus requiring a wrapper unsetting NIX_PROFILES.

If I wanted these kind of impurities, I could have stayed with Debian or any other distro.

This condition ensure that whenever ASPELL_CONF contains data-dir or dict-dir, NIX_PROFILES is ignored.

I have tested that aspellWithDicts behaves as it should with this patch.

I don't think another variable is needed, but adding new parameter for the derivation like searchNixProfiles for safety would be OK.

roastiek · 2017-10-09T17:56:07Z

@michalrus I have tested it with evolution from 17.03, not from this branch.

I have tried build gedit from this branch (it is faster). I have 'aspellDicts.cs' and 'aspellDicts.en' in 'systemPackages' and installed another dict via nix-env -iA nixos.aspellDicts.de and all of them where available (prefs -> tools -> set language).

And about nixpkgs/nixos/modules/programs/environment.nix, I am just not sure, if removing that code should be part of this PR. I think it is useless even with current aspell derivation and cloud be removed regardless whether this PR gets accepted or not.

FRidh · 2017-10-09T18:28:03Z

how about this part?

I agree with @deedrah , that's for another PR.

roastiek · 2017-10-11T17:18:02Z

I have update my branch.

@michalrus evolution sometimes keeps background running process and maybe the new binary was not used (maybe log out/in is needed). Could you test it with gedit? Or describe more detailed, how did you tried?

michalrus · 2017-10-21T21:09:30Z

@deedrah, I think I just used:

$ nix-shell -I nixpkgs=https://github.com/NixOS/nixpkgs/archive/86c5bfcdc610b06d4c943de9438148cb422e528e.tar.gz -p gnome3.evolution

… but with the previous commit SHA1.

But now when I tried that, with the current one:

builder for ‘/nix/store/nqirmq896dyngcj4ir1c4x9pllcb3han-liboauth-1.0.3’ failed previously (cached)

The error was:

/nix/store/xmfz6cv13ippmg425rar74nygmlj06jl-bash-4.4-p12/bin/bash ../libtool  --tag=CC   --mode=compile gcc -DHAVE_CON
FIG_H -I.    -Wall -I/nix/store/agdpcdar9fmjcd4cm3algmjzpxsa5bny-nss-3.32.1/include/nss -I/nix/store/smhc7vh4a57j08j9k
nrc73v6vpnybmj9-nspr-4.16-dev/include  -g -O2 -c -o liboauth_la-oauth.lo `test -f 'oauth.c' || echo './'`oauth.c
libtool: compile:  gcc -DHAVE_CONFIG_H -I. -Wall -I/nix/store/agdpcdar9fmjcd4cm3algmjzpxsa5bny-nss-3.32.1/include/nss 
-I/nix/store/smhc7vh4a57j08j9knrc73v6vpnybmj9-nspr-4.16-dev/include -g -O2 -c oauth.c  -fPIC -DPIC -o .libs/liboauth_l
a-oauth.o

No such file or directory
 #  include "pk11pub.h"
                       ^
compilation terminated.
make[2]: *** [Makefile:377: liboauth_la-oauth.lo] Error 1

So this is unrelated to your PR. Could you give me a command that works for you w/ gedit?

Thank you for your effort! ♥

michalrus · 2017-10-21T21:10:53Z

Possibly just rebase this commit onto somewhere later, where that pk11pub.h bug has been fixed. :)

michalrus · 2017-10-21T21:58:20Z

This patch starts to compile on current nixos-unstable, 0ce09af.

But it’s currently building webkit. :/

michalrus · 2017-10-22T00:55:14Z

It compiled well on nixos-unstable, however, when running, fails with:

The name org.gnome.evolution.dataserver.Sources5 was not provided by any .service files

I do confirm gedit works, though.

michalrus · 2017-10-22T01:52:12Z

@deedrah, @FRidh, I managed to get it working, and the dictionaries are available.

Everything seems to be working fine. ✅

michalrus · 2017-10-22T02:04:51Z

@deedrah, so it would be wonderful if you rebased this commit onto nixos-unstable, so that people can use this directly giving SHA1 to nix-shell, until it’s merged/discussed/etc. 🙏

Aspell will search for dictionaries in all nix profiles even when used as library. Setting data-dir or dict-dir in ASPELL_CONF will disable this behavior.

roastiek · 2017-10-22T21:00:39Z

@michalrus it's rebased

michalrus · 2017-10-22T21:42:40Z

Cool, thank you! (ﾉ^_^)ﾉ

@FRidh, WDYT?

michalrus · 2017-10-23T10:49:37Z

Awesome, thank you, all! 🤗

aspell does not find the dictionary installed in config.nix after the following change: NixOS/nixpkgs#30234

We set[1] ASPELL_CONF to the last nix profile containing lib/aspell in 2013. In 2017, aspell is patched[2] to search NIX_PROFILES, which makes [1] not needed any more. Deleting it is also agreed in this discussion[3]. [1]: 0192c02 [2]: ba4cefe [3]: NixOS#30234

FRidh requested changes Oct 9, 2017

View reviewed changes

roastiek force-pushed the aspell-data-dirs-from-nix-profiles branch from f3859bf to 86c5bfc Compare October 11, 2017 17:03

aspell: added new patch data-dirs-from-nix-profiles.patch

ba4cefe

Aspell will search for dictionaries in all nix profiles even when used as library. Setting data-dir or dict-dir in ASPELL_CONF will disable this behavior.

roastiek force-pushed the aspell-data-dirs-from-nix-profiles branch from 86c5bfc to ba4cefe Compare October 22, 2017 19:02

bjornfor approved these changes Oct 23, 2017

View reviewed changes

FRidh merged commit 891c372 into NixOS:master Oct 23, 2017

michalrus mentioned this pull request Oct 29, 2017

LibreOffice doesn’t see installed Aspell dictionaries #30920

Closed

svend added a commit to svend/dotfiles that referenced this pull request Oct 30, 2017

Create custom aspell with en dictionary

a03c3a7

aspell does not find the dictionary installed in config.nix after the following change: NixOS/nixpkgs#30234

roastiek deleted the aspell-data-dirs-from-nix-profiles branch November 6, 2017 16:44

jian-lin mentioned this pull request Aug 16, 2023

nixos/environment: stop setting ASPELL_CONF #249432

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aspell: added new patch data-dirs-from-nix-profiles.patch #30234

aspell: added new patch data-dirs-from-nix-profiles.patch #30234

roastiek commented Oct 8, 2017

michalrus commented Oct 8, 2017

michalrus commented Oct 8, 2017

FRidh left a comment

FRidh Oct 9, 2017

bjornfor Oct 9, 2017

FRidh Oct 9, 2017

roastiek Oct 9, 2017 •

edited

roastiek commented Oct 9, 2017

FRidh commented Oct 9, 2017

roastiek commented Oct 11, 2017

michalrus commented Oct 21, 2017

michalrus commented Oct 21, 2017

michalrus commented Oct 21, 2017

michalrus commented Oct 22, 2017 •

edited

michalrus commented Oct 22, 2017

michalrus commented Oct 22, 2017

roastiek commented Oct 22, 2017

michalrus commented Oct 22, 2017

michalrus commented Oct 23, 2017

aspell: added new patch data-dirs-from-nix-profiles.patch #30234

aspell: added new patch data-dirs-from-nix-profiles.patch #30234

Conversation

roastiek commented Oct 8, 2017

Motivation for this change

Things done

michalrus commented Oct 8, 2017

michalrus commented Oct 8, 2017

FRidh left a comment

Choose a reason for hiding this comment

FRidh Oct 9, 2017

Choose a reason for hiding this comment

bjornfor Oct 9, 2017

Choose a reason for hiding this comment

FRidh Oct 9, 2017

Choose a reason for hiding this comment

roastiek Oct 9, 2017 • edited

Choose a reason for hiding this comment

roastiek commented Oct 9, 2017

FRidh commented Oct 9, 2017

roastiek commented Oct 11, 2017

michalrus commented Oct 21, 2017

michalrus commented Oct 21, 2017

michalrus commented Oct 21, 2017

michalrus commented Oct 22, 2017 • edited

michalrus commented Oct 22, 2017

michalrus commented Oct 22, 2017

roastiek commented Oct 22, 2017

michalrus commented Oct 22, 2017

michalrus commented Oct 23, 2017

roastiek Oct 9, 2017 •

edited

michalrus commented Oct 22, 2017 •

edited