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

fix: update automake files to allow arm64 to compile packaged libraries #2214

Merged
merged 1 commit into from Apr 4, 2021

Conversation

@flavorjones
Copy link
Member

@flavorjones flavorjones commented Apr 2, 2021

What problem is this PR intended to solve?

The packaged versions of libxml2 and libxslt contain automake files that do not support arm64/aarch64/M1 architecture, meaning that people on those systems can't compile Nokogiri from source.

We've been relying on the precompiled native gems to solve this problem, but I guess there is a need to fully support the platform.

This is an attempt at a fix; but I really need someone with an M1 to verify that this works.

Have you included adequate test coverage?

No! We unfortunately don't have arm64 coverage in the test suite. Someone with an M1 can follow these instructions to test:

  • checkout this branch (flavorjones-allow-arm64-compilation)
  • bundle exec rake clean clobber
  • bundle exec rake compile
  • bundle exec rake test

Note that the compile step should show these patches being applied:

  • patches/libxml2/0011-update-automake-files-for-arm64.patch
  • patches/libxslt/0001-update-automake-files-for-arm64.patch

I'm tagging a few people who've interacted with me about M1 support in the past few months: @milosivanovic @mengqing @rgaufman @samwich @mzagaja. Can any of you add a comment letting me know whether this works for you?

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

No.

@flavorjones flavorjones changed the title fix: update automake files to allow arm64 to compile package libs fix: update automake files to allow arm64 to compile packaged libraries Apr 2, 2021
Copy link

@codeclimate codeclimate bot left a comment

The PR diff size of 5023 lines exceeds the maximum allowed for the inline comments feature.

@codeclimate
Copy link

@codeclimate codeclimate bot commented Apr 2, 2021

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

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 94.0% (0.0% change).

View more on Code Climate.

@mzagaja
Copy link

@mzagaja mzagaja commented Apr 3, 2021

While this worked great in Rosetta 2 mode, I don't think that's what you were aiming at. Trying to debug this made me realize I've been running everything Rosetta 2 up and down to avert these sorts of challenges, so need to setup/configure an Apple Silicon environment. Going to aim to take a run at it Sunday night.

@flavorjones
Copy link
Member Author

@flavorjones flavorjones commented Apr 3, 2021

@mzagaja Can you help me understand what didn't work for you? I was fairly confident this would Just Work so please share any/all of the log files.

@mzagaja
Copy link

@mzagaja mzagaja commented Apr 4, 2021

@flavorjones I did what I needed to do (migrate my development environment to Apple Silicon native) and it's working! All good to go on this!

# Running tests with run options --seed 28684:

........................................................................................................................................................................................................................S..S..............................................................................................................................................................................................................................................................................................................S..................................................................................................................................................................................................................................................S........................................................................................................................................................................................................................................................................................................................................S...............................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................S.S.............................................................................................................

Finished tests in 32.702431s, 53.0236 tests/s, 198.5785 assertions/s.


Slowest tests:

2.939798s test_value_lookup_segfault#Nokogiri::XML::TestReaderEncoding
2.141817s test_0001_should not segfault#Nokogiri::XML::NodeSet::adding nodes from different documents to the same NodeSet
1.051172s test_reader_blocking#Nokogiri::XML::TestReader
0.498329s test_text_node_robustness_gh1426#Nokogiri::XML::Node
0.213397s test_serialize_encoding_html#Nokogiri::HTML::TestNodeEncoding

1734 tests, 6494 assertions, 0 failures, 0 errors, 7 skips
Coverage report generated for Unit Tests to /Users/mzagaja/Developer/nokogiri/coverage. 2311 / 2447 LOC (94.44%) covered.
@flavorjones
Copy link
Member Author

@flavorjones flavorjones commented Apr 4, 2021

@mzagaja Amazing! Thank you so much for your help.

@flavorjones flavorjones merged commit beb77f5 into main Apr 4, 2021
19 checks passed
19 checks passed
@github-actions
cruby-test-system-libraries (2.7)
Details
@github-actions
cruby-test-system-libraries (3.0)
Details
@github-actions
cruby-test-vendored-libraries (2.7)
Details
@github-actions
cruby-test-vendored-libraries (3.0)
Details
@nokobot
ci.nokogiri.org/cruby-2.5 Nokobot is happy with this job.
Details
@nokobot
ci.nokogiri.org/cruby-2.6 Nokobot is happy with this job.
Details
@nokobot
ci.nokogiri.org/cruby-2.7 Nokobot is happy with this job.
Details
@nokobot
ci.nokogiri.org/cruby-3.0 Nokobot is happy with this job.
Details
@nokobot
ci.nokogiri.org/cruby-gem-test Nokobot is happy with this job.
Details
@nokobot
ci.nokogiri.org/cruby-native-gem-test Nokobot is happy with this job.
Details
@nokobot
ci.nokogiri.org/cruby-native-gem-test-32bit Nokobot is happy with this job.
Details
@nokobot
ci.nokogiri.org/cruby-on-musl Nokobot is happy with this job.
Details
@nokobot
ci.nokogiri.org/cruby-on-vanilla-ubuntu Nokobot is happy with this job.
Details
@nokobot
ci.nokogiri.org/cruby-with-libxmlruby Nokobot is happy with this job.
Details
@nokobot
ci.nokogiri.org/jruby-9.2 Nokobot is happy with this job.
Details
@nokobot
ci.nokogiri.org/jruby-gem-test Nokobot is happy with this job.
Details
@nokobot
ci.nokogiri.org/rubocop Nokobot is happy with this job.
Details
codeclimate All good!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@flavorjones flavorjones deleted the flavorjones-allow-arm64-compilation branch Apr 4, 2021
@deepj
Copy link

@deepj deepj commented Apr 6, 2021

Unfortunately, this is back again with Ruby 3.0.1. I'm using Ruby 3.0.1 on Mac with M1 chip and I'm trying to install Nokogiri. It fails on

Running 'configure' for libxml2 2.9.10... ERROR, review '~/.gem/ruby/3.0.1/gems/nokogiri-1.11.2/ext/nokogiri/tmp/arm64-apple-darwin20/ports/libxml2/2.9.10/configure.log' to see what happened.
Last lines are:
========================================================================
checking whether to enable maintainer-specific portions of Makefiles... yes
checking build system type... arm-apple-darwin20.3.0
checking host system type... Invalid configuration `arm64-apple-darwin20': machine `arm64-apple' not recognized
configure: error: /bin/sh ./config.sub arm64-apple-darwin20 failed
========================================================================
*** extconf.rb failed ***

When I switch back to Ruby 3.0.0, tak there is no issue with Nokogiri installation.

@flavorjones
Copy link
Member Author

@flavorjones flavorjones commented Apr 6, 2021

@deepj I haven't made a release with this change yet.

@flavorjones
Copy link
Member Author

@flavorjones flavorjones commented Apr 6, 2021

@deepj Please use the precompiled native gem. Or open a new issue if you want to discuss.

@deepj
Copy link

@deepj deepj commented Apr 6, 2021

@flavorjones Ah, my bad. I apologize.

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

Successfully merging this pull request may close these issues.

None yet

3 participants