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

native gems are built with ExtensionTask.no_native=true #2125

Merged
merged 5 commits into from
Dec 3, 2020

Conversation

flavorjones
Copy link
Member

@flavorjones flavorjones commented Dec 2, 2020

What problem is this PR intended to solve?

Specifically, following the advice of @kou at rake-compiler/rake-compiler#171, we set Rake::ExtensionTask.no_native=true within the rake-compiler-dock container ("guest") so that:

This also will enable us to more easily do things like removing the C extension source code from the native gem package (#2077).

This patch set also breaks out lib/nokogiri/version.rb into two new files:

  • lib/nokogiri/version/constant.rb
  • lib/nokogiri/version/info.rb

and require_relatives them both from version.rb. This is so that Hoe doesn't pull in REQUIRED_LIBXML_VERSION from extconf.rb after 652c6fd extracted that value into a constant.

This patch set also updates how Darwin native gems are built, to mirror the same rake task structure that's used for Linux and Windows; and it renames to "builder" the rake tasks that used to be "guest".

Have you included adequate test coverage?

I'm satisfied with the level of testing we have now on different platforms, though it could always be better. I will add some testing to the packaged/installed gem when I merge #1788 which introduces that test coverage pretty nicely.

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

Should only impact how the native (precompiled) gems are built and packaged.

warnings
end

def to_hash
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method to_hash has a Cognitive Complexity of 16 (exceeds 5 allowed). Consider refactoring.

warnings
end

def to_hash
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method to_hash has 39 lines of code (exceeds 25 allowed). Consider refactoring.

@codeclimate
Copy link

codeclimate bot commented Dec 2, 2020

Code Climate has analyzed commit c50a658 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 2

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

This pull request will bring the total coverage in the repository to 94.3% (0.0% change).

View more on Code Climate.

also:
- extract VersionInfo into a separate file
- make lib/nokogiri/version.rb require_relative these two new files

so that we can easily find it from Rakefile now,
and potentially a standalone gem spec later.

(This was driven out because Hoe does a questionable thing when
inferring the gem version, which is to grep through files in order of
Manifest.txt; and this is leading to using the LIBXML_REQUIRED_VERSION
as the VERSION. ¯\_(ツ)_/¯)
and add some filtering for common temp files to reduce noise
Specifically, following the advice of @kou at

  rake-compiler/rake-compiler#171

we set `Rake::ExtensionTask.no_native=true` within the
rake-compiler-dock container ("guest") so that:

- the ExtensionTask `cross_compiling` block is called
  - so that we don't package the dependencies' tarballs in /ports
  - so that we don't have mini_portile2 as a dependency (#2078)
  - so that we don't have an extra nokogiri.so/nokogiri.bundle built
    and packaged (#2076)
- we no longer have to hotfix rake-compiler at build time

This also will enable us to more easily do things like removing the C
extension source code from the native gem package (#2077).
@flavorjones flavorjones force-pushed the 2078-fix-rake-compiler-native-things branch 2 times, most recently from c7e8633 to c91ece6 Compare December 3, 2020 05:27
That is, darwin tasks now mirror the linux and windows tasks even
though there's no guest container (only a child process). This way we
avoid having darwin-specific handling from the POV of the person
invoking rake.

The native platform tasks previously named "guest" are now named
"builder" because that's a more descriptive and more accurate name,
especially since (on darwin) there's no actual guest.
@flavorjones flavorjones force-pushed the 2078-fix-rake-compiler-native-things branch from c91ece6 to 440dc4e Compare December 3, 2020 07:41
@flavorjones flavorjones merged commit b54c3fb into master Dec 3, 2020
@flavorjones flavorjones deleted the 2078-fix-rake-compiler-native-things branch December 3, 2020 13:16
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