atom / node-spellchecker

SpellChecker Node Module
http://atom.github.io/node-spellchecker
MIT License
300 stars 108 forks source link

Enable building on FreeBSD #125

Closed vrachnis closed 5 years ago

vrachnis commented 5 years ago

Requirements for Contributing a Bug Fix

Identify the Bug

Similar to #87, building is broken in FreeBSD for 2 reasons:

  1. The OS is not recognized by the build system.
  2. hunspell won't build cleanly under LLVM/Clang.

Description of the Change

I would really like to make this change support OpenBSD as #87 reports, but given that I don't have a VM available at the moment, I limited my changes to FreeBSD (that I can test).

This PR includes 2 changes:

  1. Make freebsd a recognizable platform alongside mac, linux and win. The logic is identical to Linux, since nothing special is required.
  2. Partially backport https://github.com/hunspell/hunspell/commit/be3b8d945a2577fb714fee95a6499f22a7e2cef6#diff-0dd29bfd51e6c6fdadebc9b96923c5c6 and https://github.com/hunspell/hunspell/commit/dd3a71c38b614adc88d232f16fb1b199c97cf28f#diff-0dd29bfd51e6c6fdadebc9b96923c5c6. Hunspell have already fixed the build issue upstream, but the version included with spellchecker is not up to date. Instead of pulling in all the changes from upstream, include the must-haves that unbreak the build.

Alternate Designs

Ideally, we could update hunspell to the latest version. However that might be a big change, given that the last time that the vendor directory was updated was 6 years ago.

With this in mind, I tried to pick the minimum amount of changes that will unbreak the build and minimize the risk of breakage.

Possible Drawbacks

I am not aware of any drawbacks. Given that this is just following the upstream commits, this shouldn't have any negative impact.

Verification Process

Verified that the build works, and that the unit tests still pass.

$ yarn test
yarn run v1.16.0
$ jasmine-focused --captureExceptions --coffee spec/
..........................................................................................................................................

Finished in 13.619 seconds
138 tests, 184 assertions, 0 failures, 0 skipped

Done in 14.37s.

Release Notes

rsese commented 5 years ago

Thanks for the contribution! We took a look at this today and it seems like this change isn't necessary for Atom specifically in that it's not fixing a problem/bug with Atom's use of this module for spellchecking (please let me know if I'm misunderstanding however).

With our current resources and priorities, as much as possible we're unfortunately not taking on changes that are for use cases outside of Atom and don't directly address some issue or bug in Atom itself. Since this isn't something the team will take on I'm going to close this out but thank you again for your contribution.