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

build hunspell-sys also on Windows #2

Merged
merged 7 commits into from
Jul 9, 2020
Merged

build hunspell-sys also on Windows #2

merged 7 commits into from
Jul 9, 2020

Conversation

msuesskraut
Copy link
Contributor

  • msvc does not support autotools &
    hunspell's build scripts itself do not play well with msvc
  • just compile the hunspell-1.7.lib with cc "by hand"
  • keep build without msvc as it is

- msvc does not support autotools &
  hunspell's build scripts itself do not play well with msvc
- just compile the hunspell-1.7.lib with cc "by hand"
- keep build without msvc as it is
@drahnr
Copy link
Contributor

drahnr commented Jun 19, 2020

Very nice work!

@msuesskraut
Copy link
Contributor Author

It could also be a fallback if autotools is not installed.

It would be nice to autodetect, if autotools are available, but the autotools crate does not provide a check. It plainly assumes, that autotools are on PATH.

- build-autotools to build with autotools
- build-cc to build with cc
- default is build-autotools
- added documentation into Readme.md to show
  how to enable build-cc
@msuesskraut
Copy link
Contributor Author

I hid the build with cc behind a feature build-cc. Default is still to use autotools (with the new build-autotools feature). I added a Building section into Readme.md to explain this.

Cargo seems not to allow target specific default features. However, dependent crates can specify targets specific features for hunspell-sys.

[target.'cfg(target_env = "msvc")'.dependencies]
hunspell-sys = { version = "0.1.3", default-features = false, features = ["build-cc"] }

[target.'cfg(not(target_env = "msvc"))'.dependencies]
hunspell-sys = "0.1.3"

- added information about target specific
@drahnr
Copy link
Contributor

drahnr commented Jun 22, 2020

@euclio anything left to do here? This is blocking cargo-spellcheck from being installed on windows, so I'd see this merged rather sooner than later :)

@euclio
Copy link
Owner

euclio commented Jun 22, 2020

I don't think a feature flag is the right solution here. I would prefer either using cc as a fallback if autotools is not installed, or switching the build to use cc entirely.

@drahnr
Copy link
Contributor

drahnr commented Jun 22, 2020

Using cc and subsequent static linkage would cause some license friction. Frankly right now I am not sure if this is even the right place to impl the feature flag, or if it might be better suited for hunspell-rs rather than the generated bindings.
What speaks against a feature flag?

@euclio
Copy link
Owner

euclio commented Jun 22, 2020

Cargo features are not mutually exclusive. If I try to build this PR with cargo build --all-features it will not work.

I'm unfamiliar with the linking impact on licensing here. Why is statically linking not an issue for MSVC but it is for other platforms?

@drahnr
Copy link
Contributor

drahnr commented Jun 22, 2020

It's not about msvc, it's about static linkage with libhunspell

@euclio
Copy link
Owner

euclio commented Jun 22, 2020

We can use libhunspell under the MPL license, which has no linking restrictions. In my understanding, since we haven't modified the hunspell source, we're free to statically link and distribute it as long as it's clear where to find the source.

MPL FAQ: https://www.mozilla.org/en-US/MPL/2.0/FAQ/

@drahnr
Copy link
Contributor

drahnr commented Jun 22, 2020

Huh, mea culpa, I had something GPL-ish in the back of my head. Discard my previous comments then.

@euclio
Copy link
Owner

euclio commented Jul 7, 2020

Ping @msuesskraut, any interest in finishing this?

@msuesskraut
Copy link
Contributor Author

The question is on how to rewrite it. We could orient ourselves on libsqlite3-sys:

Hence, that would mean to search for the hunspell library by default via pkg-config.

However, with the bundled feature the hunspell library will be compiled from source by cc.

Is this the way to move forward? If yes, I could adapt my pull request.

@drahnr
Copy link
Contributor

drahnr commented Jul 8, 2020

I would very much appreciate that approach.

@euclio
Copy link
Owner

euclio commented Jul 8, 2020

Yeah, that sounds great to me.

- by default hunspell is searched by pkg-config
- optionally - with feature bundled - hunspell is build from
  directory vendor by crate cc
- documented this in README.md
- extended .travis.yml to test building the bundled hunspell
- for testing feature bundled
- for testing all features in all rust versions
@msuesskraut
Copy link
Contributor Author

So I updated the pull request as discussed. Sorry for the many commits. I'm not so fluent with travis. I copied the approach to test the bundled feature from rusqlite.

I also updated the build dependencies to the most current versions.

Please have a look.

Copy link
Owner

@euclio euclio left a comment

Choose a reason for hiding this comment

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

Looks great! Just one suggestion.

Don't worry too much about travis, I'll have to change it anyways to get it to work with Windows. (I'll probably migrate to GHA too).

README.md Outdated Show resolved Hide resolved
@euclio euclio merged commit d49c3bf into euclio:master Jul 9, 2020
@euclio
Copy link
Owner

euclio commented Jul 9, 2020

Published v0.2.0.

@drahnr
Copy link
Contributor

drahnr commented Jul 10, 2020

Thank you!

@msuesskraut
Copy link
Contributor Author

msuesskraut commented Jul 10, 2020 via email

@msuesskraut msuesskraut deleted the build_with_msvc branch July 10, 2020 07:54
@drahnr
Copy link
Contributor

drahnr commented Jul 10, 2020

Just for sake of completeness:

https://hunspell.github.io/ states the following regarding licensing:

C++ library under GPL/LGPL/MPL tri-license.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants