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

aspell: add all missing official dictionaries #53143

Merged
merged 6 commits into from Feb 24, 2019

Conversation

liff
Copy link
Contributor

@liff liff commented Jan 1, 2019

From:

ftp://ftp.gnu.org/gnu/aspell/dict/0index.html

Also sets the default homepage of the dictionaries to
the above URL.

The Marathi and Hindi dictionaries both contain u-deva.{cmap,cset} files, which cause a conflict. This patch adds those files from the Marathi dictionary into the main aspell package like Debian does.

Motivation for this change

I intended to add the Finnish dictionary to aspellDicts but noticed there’s a fairly large bunch of official ones not included so why not add them all in one go?

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@liff
Copy link
Contributor Author

liff commented Jan 1, 2019

Running nox-review wip results in a build failure in haskellPackages.djinn but I’m fairly certain it’s not related.

@FRidh
Copy link
Member

FRidh commented Jan 1, 2019

It's good to add dictionaries for other languages, however, we need a way to maintain these expressions. Basically, we need a script that can generate the data set.

@liff
Copy link
Contributor Author

liff commented Jan 1, 2019

I’ll see if I can come up with a robust script for keeping these up to date.

From:

  https://ftp.gnu.org/gnu/aspell/dict/0index.html

Use a specialized function, buildOfficialDict to specialize builds of
official aspell dictionaries and add an update script.
@liff
Copy link
Contributor Author

liff commented Jan 1, 2019

Added a buildOfficialDict function for official dictionaries with an updateScript.

The name should include the version number and this is handled just fine by
buildDict.
Copy link
Member

@Ekleog Ekleog left a comment

Choose a reason for hiding this comment

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

@liff This looks good, thanks! Just one point: here, it looks like a MitM to someone who runs the update script can execute arbitrary code on the computer running the update script (via the $versions substitution), so it's I think a bit too permissive.

Maybe it'd be possible to write it in a more defensive way, to prevent that from happening?


meta = {
homepage = "http://ftp.gnu.org/gnu/aspell/dict/0index.html";
} // (args.meta or {});
Copy link
Member

Choose a reason for hiding this comment

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

This line is useless with the // removeAttrs args [ ... ] below: args.meta would override the whole meta anyway. I guess you forgot "meta" in the list below?

@liff
Copy link
Contributor Author

liff commented Feb 17, 2019

Would it be sufficient to restrict the version string to [A-Za-z0-9_+.-] instead of .+ to keep the generated version entries within quotes?

@Ekleog
Copy link
Member

Ekleog commented Feb 17, 2019

It should do it indeed!

Now, I've just noticed another issue: the nix executable interface is not stable yet, so we're not allowed to rely on it yet in nixpkgs.

 - Use HTTPS and tighter version regexp to guard against MitM
 - Use nix-instantiate instead of experimental nix eval
 - Handle dictionary-specific meta overrides
@liff
Copy link
Contributor Author

liff commented Feb 18, 2019

Thanks for the review.

Changed from FTP to HTTPS since apparently they are going to disable FTP at some unspecified time anyway, adjusted the regexp, changed from nix to nix-instantiate and fixed the meta thing.

@Ekleog
Copy link
Member

Ekleog commented Feb 19, 2019

@GrahamcOfBorg eval
@GrahamcOfBorg build aspellDicts.bn

@oxij
Copy link
Member

oxij commented Feb 22, 2019 via email

@Ekleog
Copy link
Member

Ekleog commented Feb 24, 2019

@liff Do you think you could replace your usage of lynx with curl and some ad-hoc matching on html elements? If you don't, as it's for an update script it's likely not that bad, but it'd for sure be cleaner to.

@7c6f434c
Copy link
Member

In practice hacks like those in my updater nixpkgs/pkgs/build-support/upstream-updater/urls-from-page.sh are almost always sufficient; not sure if I would call them cleaner than actually using an HTML parser.

@liff
Copy link
Contributor Author

liff commented Feb 24, 2019

Changed to curl.

@Ekleog
Copy link
Member

Ekleog commented Feb 24, 2019

@GrahamcOfBorg eval
@GrahamcOfBorg build aspellDicts.bn

@Ekleog
Copy link
Member

Ekleog commented Feb 24, 2019

Looks like everything is good! Thank you, @liff :) Merged!

@Ekleog
Copy link
Member

Ekleog commented Feb 24, 2019

… And noticed just a bit too late to stop the push: The last 3 commits' messages are not following nixpkgs' convention (while the first 3 were). Mea culpa for not having noticed this.

@Ekleog Ekleog merged commit 098c442 into NixOS:master Feb 24, 2019
@liff
Copy link
Contributor Author

liff commented Feb 24, 2019

Oops, sorry. Seems I forgot about that half way when working on the requested changes.

@liff liff deleted the aspell/all-dicts branch November 27, 2023 09:42
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.

None yet

6 participants