-
-
Notifications
You must be signed in to change notification settings - Fork 899
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
get Nokogumbo's test suite green for basic cases #2222
Conversation
1eea62b
to
0f22de1
Compare
Code Climate has analyzed commit ebd76e7 and detected 14 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 100.0% (80% is the threshold). This pull request will bring the total coverage in the repository to 93.2% (-0.7% change). View more on Code Climate. |
3882b66
to
d4ab401
Compare
See rubys/nokogumbo#171 which introduced forward-looking behavior to integrate with a future Nokogiri::HTML5 after the Nokogumbo merger. These tests, which are failing right now, will drive much of the rest of the integration work.
and remove some files which are not going to be imported: - nokogumbo.rb - nokogumbo/version.rb - nokogumbo.gemspec
This is a straight lift-and-shift of the files, we'll need to do more work to get this to compile and run.
- still treating nokogumbo as a second extension - stripped down ext/nokogumbo/extconf.rb to the basics - renamed Nokogumbo to Nokogiri::Gumbo
- all HTML5 test classes inherit from Nokogiri::TestCase - some rubocop formatting applied to HTML5 test files - frozen_string_literal set on HTML5 test files - omit HTML5 tests if we're not using gumbo (i.e., jruby) - minor code changes to work with frozen strings - avoid circular requires
we're all in on the libxml2 C API now.
- move nokogumbo.c to ext/nokogiri/gumbo.c - remove the nokogumbo extension - treat the gumbo parser as a mini_portile recipe I'm not 100% sure this is the right thing to do, but let's see how far I get on cross-compiling before I give up.
to use the environment variable feature of MiniPortile#execute to compile the gumbo parser.
Note that we're using a new feature in mini_portile 2.5.1 to pass environment variables to `#execute` for the subprocess. This allows us to pass a single command string to `Process.spawn` and take advantage of the subshell's command substitution (e.g., rake-compiler's "MAKE=nice make -j`nproc`"). This also allows us to have environment variables with embedded spaces without worrying about cross-platform escaping (e.g., "CFLAGS=-fPIC -g").
and also do the right thing for all cross-compilations
and sort the ignore_files
and make sure it's included in the vanilla gem.
d4ab401
to
ebd76e7
Compare
I think this is ready to merge now. If everything goes green (and I think it should, but it will take another hour to finish CI), I'll merge. |
Note that this draft PR will be a relatively long-lived feature branch.
What problem is this PR intended to solve?
Starting functional integration of Nokogumbo from #2204:
rake test
lib/nokogiri
and C file moved intoext/nokogiri
nokogiri.so
Have you included adequate test coverage?
Indeed. The goal of this PR is to pull in Nokogumbo's test suite and code and get a basic green.
Does this change affect the behavior of either the C or the Java implementations?
Nokogiri::HTML5 will notably only be available for CRuby (and possibly other rubies that support C extensions). See #2204 for more details.