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

MSVC should be able to compile nokogiri C extension #2016

Merged
merged 1 commit into from
Mar 30, 2020

Conversation

jmarrec
Copy link
Contributor

@jmarrec jmarrec commented Mar 26, 2020


What problem is this PR intended to solve?

Aims to partially fix the issue #2015 to allow compilation on MSVC.
This fixes the redefinition issues, but ideally the packaged dependencies (zlib, libxml, lixslt) would work out of the box (I am using conan to get the dependencies and passing some arguments such as with-xxx-dir to make it work right now), and the .cross-rubies and task/cross-ruby.rb would be modified to allow MSVC gems to be packaged.

Have you included adequate test coverage?

We have a thorough test suite that allows us to create releases confidently and prevent accidental regressions. Any proposed change in behavior must be accompanied by tests.

If possible, please try to write the tests so that they communicate intent.

Does this change affect the C or the Java implementations?

I'm adding a macro to the C nokogiri.h header which shouldn't have much effect since it's wrapped in a #if _MSC_VER block.

I currently added jaro_winkler to a patched version of mine (tonytonyjan/jaro_winkler#43)

@flavorjones
Copy link
Member

Let's discuss in #2015

@flavorjones
Copy link
Member

I'll note that jaro_winkler is only a development dependency, and not a runtime dependency. If you're having to build from source, you can safely remove rubocop from your Gemfile, no big deal.

If you'd like to remove the Gemfile patch from #2016, and let the PR pipeline run, it should go green and the I'd be happy to merge it.

…definitions

Note: jaro_winkler is a native gem dependency of rubocop that currently doesn't supports C89 (MSVC)
@jmarrec
Copy link
Contributor Author

jmarrec commented Mar 30, 2020

@flavorjones cleaned up and force pushed.

@flavorjones
Copy link
Member

OK - the remaining failure looks like it would be fixed by a rebase onto master, but that's fine, I'll merge this!

@flavorjones flavorjones merged commit aaccf5a into sparklemotion:master Mar 30, 2020
@flavorjones flavorjones changed the title Support MSVC MSVC should be able to compile nokogiri C extension Mar 30, 2020
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.

2 participants