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.

Sign up for GitHub

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

Jump to bottom

pyenchant: Fix for darwin/macOS #78341

Merged
merged 1 commit into from Jan 31, 2020
Merged

pyenchant: Fix for darwin/macOS #78341

merged 1 commit into from Jan 31, 2020

Conversation

riannucci
Copy link
Contributor

@riannucci riannucci commented Jan 23, 2020 •
edited

Motivation for this change

This allows pyenchant to be installable on macOS again, which, in turn, will allow pylint
to also be installable.

Things done
  • Switches dependencies to enchant-2 (enchant-1 is not building on macOS). The existing $src (2.0.0) already has compatibility with enchant-2.
  • Removes patch hack in favor of explicit $PYENCHANT_LIBRARY_PATH envvar
Testing
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

(Sorry for not ticking more boxes above; I just started using nix today and was trying to replace my use of homebrew as an exercise; I'll continue iterating on the testing items here.)

@ofborg ofborg bot added 6.topic: darwin Running or building packages on Darwin 6.topic: python labels Jan 23, 2020
@riannucci
Copy link
Contributor Author

Not sure what I'm doing, but the dependency on enchant-2 looks smaller than enchant-1:

$ nix path-info -S /nix/store/24pfl0nknq9a8y68gm0a10gsl2di951z-enchant-2.2.5.tar.gz.drv \ 
   /nix/store/rkvkcl1gwpiw7dslv7kfrqb4pms6mizh-enchant-1.6.1.drv
/nix/store/24pfl0nknq9a8y68gm0a10gsl2di951z-enchant-2.2.5.tar.gz.drv	    1488280
/nix/store/rkvkcl1gwpiw7dslv7kfrqb4pms6mizh-enchant-1.6.1.drv       	    2493448

@riannucci
Copy link
Contributor Author

Hm, now I'm not sure if this will work correctly because I don't think the envvar will be set outside of setup.py; I'll restore the hack to hard-code the lookup path directly into _enchant.py

@riannucci riannucci force-pushed the patch-1 branch 2 times, most recently from 0f1dc32 to 3f33022 Compare January 23, 2020 01:51
@riannucci
Copy link
Contributor Author

Squashed and fixed commit message

@jonringer
Copy link
Contributor

an env var will only be present while the build is taking place. When consuming a package (like with nix-shell), it will not be present. If you need something to have persistence, you need to patch the source with a python package

@riannucci
Copy link
Contributor Author

Yep, see the latest patchset; it now patches the path directly into the python file.

@riannucci
pyenchant: Fix for darwin/macOS
2bf75af 
This allows pyenchant to be installable on macOS again, which, in turn, will allow pylint
to also be installable.

  * Switches dependencies to enchant-2 (enchant-1 is not building on macOS). The existing $src (2.0.0) already has compatibility with enchant-2.
  * Improves patch hack by hijacking the $PYENCHANT_LIBRARY_PATH envvar lookup
    to explicitly specify the correct library path.
@riannucci
Copy link
Contributor Author

(Just to be clear, I believe I've addressed everything; are you waiting on me for something additional?)

@riannucci
Copy link
Contributor Author

(no rush if not, just want to make sure this isn't blocked on me :))

@jonringer
Copy link
Contributor

@GrahamcOfBorg mwic paperwork python27Packages.diff_cover python27Packages.pamqp python27Packages.pyenchant python27Packages.pylint python27Packages.pyls-isort python27Packages.pyspread python27Packages.pytest-pylint python27Packages.python-language-server python27Packages.rabbitpy python27Packages.soco python27Packages.sopel python37Packages.paperwork-backend python37Packages.pyenchant python37Packages.sopel python37Packages.sphinxcontrib-spelling python38Packages.pyenchant python38Packages.sopel python38Packages.sphinxcontrib-spelling retext uberwriter

jonringer
jonringer approved these changes Jan 30, 2020
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

diff LGTM

been busy with real life

[33 built (4 failed), 1251 copied (7792.9 MiB), 1218.9 MiB DL]
error: build of '/nix/store/axgh4d9gnwak3l97jfk22qhnz940pqga-env.drv' failed
https://github.com/NixOS/nixpkgs/pull/78341
4 package failed to build:
python27Packages.sphinxcontrib-spelling python27Packages.spyder python38Packages.paperwork-backend sasview

22 package built:
mwic paperwork python27Packages.diff_cover python27Packages.pamqp python27Packages.pyenchant python27Packages.pylint python27Packages.pyls-isort python27Packages.pyspread python27Packages.pytest-pylint python27Packages.python-language-server python27Packages.rabbitpy python27Packages.soco python27Packages.sopel python37Packages.paperwork-backend python37Packages.pyenchant python37Packages.sopel python37Packages.sphinxcontrib-spelling python38Packages.pyenchant python38Packages.sopel python38Packages.sphinxcontrib-spelling retext uberwriter

@jonringer
Copy link
Contributor

@GrahamcOfBorg build mwic paperwork python27Packages.diff_cover python27Packages.pamqp python27Packages.pyenchant python27Packages.pylint python27Packages.pyls-isort python27Packages.pyspread python27Packages.pytest-pylint python27Packages.python-language-server python27Packages.rabbitpy python27Packages.soco python27Packages.sopel python37Packages.paperwork-backend python37Packages.pyenchant python37Packages.sopel python37Packages.sphinxcontrib-spelling python38Packages.pyenchant python38Packages.sopel python38Packages.sphinxcontrib-spelling retext uberwriter

@jtojnar jtojnar merged commit 29d8a1c into NixOS:master Jan 31, 2020
anna328p pushed a commit to anna328p/nixpkgs that referenced this pull request Feb 2, 2020
@jtojnar @anna328p
Merge pull request NixOS#78341 from riannucci/patch-1
1ebab73 
pyenchant: Fix for darwin/macOS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtojnar jtojnar jtojnar left review comments

@jonringer jonringer jonringer approved these changes

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
6.topic: darwin Running or building packages on Darwin 6.topic: python 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@riannucci @jonringer @jtojnar @Janik-Haag