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

feat: adds nokogiri/cppflags to VERSION_INFO #2145

Merged
merged 1 commit into from
Dec 24, 2020

Conversation

flavorjones
Copy link
Member

the presence of CPPFLAGS suitable for compilation against nokogiri and
the packaged libraries will allow us to deprecate
libxml/libxml2_path in v1.12.x

also:

  • change VERSION_INFO["nokogiri"] from a string to a hash
  • add VERSION_INFO["nokogiri"]["version"] to store the version string

What problem is this PR intended to solve?

I'd like to make sure we have a mechanism in place in v1.11.x that will replace `VERSION_INFO["libxml"]["libxml2_path"] so that we can deprecate it. With this value, another gem's extconf is as simple as:

append_cflags(Nokogiri::VERSION_INFO["nokogiri"]["cppflags"])
have_libxml2 = have_header('libxml/tree.h')
have_ng = have_header('nokogiri.h')

Have you included adequate test coverage?

Yes, test coverage added to test/test_version.rb and scripts/test-gem-installation

Does this change affect the behavior of either the C or the Java implementations?

No.

@codeclimate
Copy link

codeclimate bot commented Dec 24, 2020

Code Climate has analyzed commit ba04a58 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 72.7% (80% is the threshold).

This pull request will bring the total coverage in the repository to 94.1% (-0.1% change).

View more on Code Climate.

the presence of CPPFLAGS suitable for compilation against nokogiri and
the packaged libraries will allow us to deprecate
`libxml/libxml2_path` in v1.12.x

also:
- change VERSION_INFO["nokogiri"] from a string to a hash
- add VERSION_INFO["nokogiri"]["version"] to store the version string
@flavorjones flavorjones force-pushed the flavorjones-version-info-cppflags branch from cf92267 to ba04a58 Compare December 24, 2020 17:45
@flavorjones flavorjones merged commit f82b28f into master Dec 24, 2020
@flavorjones flavorjones deleted the flavorjones-version-info-cppflags branch December 24, 2020 21:28
flavorjones added a commit that referenced this pull request Apr 7, 2021
which are present starting in Nokogiri v1.11.0.rc4.

See #2145 for more information.

With this change, here's what compilation looks like when Nokogiri is
built with libxml2:

> /home/flavorjones/.rvm/rubies/ruby-2.7.2/bin/ruby -I. ../../../../ext/nokogumbo/extconf.rb
> checking for whether -I/home/flavorjones/.rvm/gems/ruby-2.7.2/gems/nokogiri-1.11.0.rc3/ext/nokogiri is accepted as CFLAGS... yes
> checking for whether -I/home/flavorjones/.rvm/gems/ruby-2.7.2/gems/nokogiri-1.11.0.rc3/ext/nokogiri/include is accepted as CFLAGS... yes
> checking for whether -I/home/flavorjones/.rvm/gems/ruby-2.7.2/gems/nokogiri-1.11.0.rc3/ext/nokogiri/include/libxml2 is accepted as CFLAGS... yes
> checking for libxml/tree.h... yes
> checking for nokogiri.h... yes
> creating Makefile

and here's what compilation looks like when Nokogiri is _not_ built with
libxml2:

> checking for whether -I/home/flavorjones/.rvm/gems/ruby-2.7.2/gems/nokogiri-1.11.0.rc3/ext/nokogiri is accepted as CFLAGS... yes
> checking for libxml/tree.h... no
> checking for nokogiri.h... no
> checking for xmlNewDoc() in -lxml2... yes
> checking for nokogiri.h in /home/flavorjones/.rvm/gems/ruby-2.7.2/gems/nokogiri-1.11.0.rc3/ext/nokogiri... yes
> creating Makefile

In a future update, once we've pinned the Nokogiri dependency to "~>
1.11", we should be able to remove the stanza that looks at
`libxml2_path`.
flavorjones added a commit that referenced this pull request Apr 7, 2021
This is necessary on Windows where unresolved symbols aren't
allowed. We also limit compatibility with Nokogiri's precompiled libraries
to Nokogiri >= 1.11.2 on Windows for this reason.

Related to:
- #2145
- #2167
- #2202
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.

1 participant