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.

Sign up for GitHub

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

Jump to bottom

fix nokogumbo linking on windows #2202

Merged
merged 5 commits into from Mar 8, 2021
Merged

fix nokogumbo linking on windows #2202

merged 5 commits into from Mar 8, 2021

Conversation

flavorjones
Copy link
Member

What problem is this PR intended to solve?

Closes #2167 which relates to an issue building Nokogumbo on Windows against the precompiled libraries.

This PR does the following:

  • ensures that libxml2 symbols are exported when built on windows
  • ensures that nokogiri symbols are exported when built on windows
  • includes LDFLAGS in Nokogiri::VERSION_INFO to allow the windows linker to resolve all symbols

Have you included adequate test coverage?

There's pretty good test coverage of this test case in the Nokogumbo CI suite; and I've added some light tests here in scripts/test-gem-installation though I'll note that we're not testing gem installation on Windows in the Nokogiri CI suite at the moment (though, see #2153).

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

This should only impact downstream gems like Nokogumbo who are trying to link against the precompiled Nokogiri libraries.

@flavorjones
fix: ensure_func fails properly if it fails during build
b99ecb8
@flavorjones
fix(windows): ensure libxml2 symbols are exported from nokogiri.so
7ace791 
Related to #2167
Related to rubys/nokogumbo#163
@flavorjones
fix(windows): ensure nokogiri symbols are exported from nokogiri.so
bb7cab7 
Related to #2167
Related to rubys/nokogumbo#163
@flavorjones
fix(windows): version info contains ldflags on windows
1b448d7 
to allow downstream gems like nokogumbo to build correctly simply by
requiring nokogiri in the extconf and running:

  append_cflags(Nokogiri::VERSION_INFO["nokogiri"]["cppflags"])
  append_ldflags(Nokogiri::VERSION_INFO["nokogiri"]["ldflags"])

this is needed because unresolved symbols aren't allowed on windows.

Related to #2167
@flavorjones
doc: update CHANGELOG and add a VersionInfo comment
3cff820
@flavorjones flavorjones force-pushed the 2167-fix-nokogumbo-linking-on-windows branch from 2ea48e8 to 3cff820 Compare March 7, 2021 21:03
@codeclimate
Copy link

codeclimate bot commented Mar 7, 2021

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

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

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

View more on Code Climate.

@larskanis
Copy link
Member

@flavorjones I'm sad, I wasn't able to help you, but I'm glad, you could solve this issue!

I like the idea of passing cflags and ldflags by the VERSION_INFO. And the proposed solution makes sense to me. I only wonder why it worked with previous nokogiri versions. And I'm pretty sure, this will not work with MSWIN alias MSVC compilers.

Unfortunately I'm still suffered by pandemic restrictions. Schools are closed since months and working as a substitute teacher while having a lot of work on the daily job is exhausting. I have to take my holiday to do any serious OSS development.

@flavorjones
Copy link
Member Author

@larskanis I don't think it ever worked on Windows ... previous versions of Nokogiri didn't package the header files as expected/promised, and so Nokogumbo will gracefully "fall back" to using Nokogiri's Ruby API (rather than libxml2's C API).

@flavorjones flavorjones merged commit 4830749 into main Mar 8, 2021
@flavorjones flavorjones deleted the 2167-fix-nokogumbo-linking-on-windows branch March 8, 2021 13:29
@flavorjones flavorjones added this to the v1.11.x patch releases milestone Mar 8, 2021
flavorjones added a commit that referenced this pull request Apr 7, 2021
@flavorjones
fix: use Nokogiri's LDFLAGS to link against precompiled libs
e0db2f7 
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
flavorjones added a commit that referenced this pull request Nov 16, 2022
@flavorjones
ci: update test-gem-installation to work on windows
b79f989 
Also modify test-nokogumbo-compatibility to skip 2.0.4 on Windows
because it's missing the LDFLAGS fix from #2167, #2202, and
nokogumbo#163.
flavorjones added a commit that referenced this pull request Nov 16, 2022
@flavorjones
ci: update test-gem-installation to work on windows
f461f9e 
Also modify test-nokogumbo-compatibility to skip 2.0.4 on Windows
because it's missing the LDFLAGS fix from #2167, #2202, and
nokogumbo#163.
flavorjones added a commit that referenced this pull request Nov 17, 2022
@flavorjones
ci: update test-gem-installation to work on windows
bec9da5 
Also modify test-nokogumbo-compatibility to skip 2.0.4 on Windows
because it's missing the LDFLAGS fix from #2167, #2202, and
nokogumbo#163.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.11.x patch releases
Development

Successfully merging this pull request may close these issues.

[bug] v1.11.0 native Windows gem causes nokogumbo installation to fail
2 participants
@flavorjones @larskanis