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

Make builds minimal #1719

Closed
wants to merge 1 commit into from
Closed

Conversation

stevecrozz
Copy link
Contributor

This is in reference to #1711 where I suggest we package minimal builds that only include the essentials. This change saves me about 2MiB on disk which sounds like nothing, but on my alpine linux Docker images its actually significant. Since we started discussing already, its probably best to talk about the general idea of removing non-essential files from the build in #1711, but here's a concrete example of what I'm thinking.

@tenderlove
Copy link
Member

This seems alright to me. Agree with @flavorjones that it's sometimes helpful to have the tests, but I also don't feel strongly about it (especially since we tag the release version in git anyway).

@flavorjones
Copy link
Member

OK - I agree in principle with minimizing the gem; however I want to make sure I have automated testing of the generated gem to ensure that we're not omitting necessary files. This is at the top of my TODO list.

@junaruga
Copy link
Contributor

however I want to make sure I have automated testing of the generated gem to ensure that we're not omitting necessary files.

@flavorjones thank you for the working with this. How was the status?
I proposed it with another pull-request.

@stevecheckoway
Copy link
Contributor

Making gems minimal in size sounds good to me as long as not too much is removed. E.g., #1788 wants the headers installed in the extension directory or at least not removed.

Until that (or something like it) happens, Nokogumbo is actually using the dependency.yml file. See https://github.com/rubys/nokogumbo/blob/master/ext/nokogumbo/extconf.rb#L15. I'd like to remove most of that code (i.e., downloading, configuring, and installing headers for libxml2), but unless Nokogiri vends the embedded headers, having the version, and SHA-256 hash of the library source code is very helpful.

@stevecrozz stevecrozz force-pushed the minimal-build branch 3 times, most recently from 9660dff to 7462bd5 Compare October 5, 2018 21:51
@stevecrozz
Copy link
Contributor Author

I've rebased this branch onto most recent master and @stevecheckoway I have restored dependencies.yml to Manifest.txt as it sounds like gems may depend on it.

Manifest.txt Outdated
@@ -256,6 +238,7 @@ patches/libxml2/0003-Fix-infinite-loop-in-LZMA-decompression.patch
patches/sort-patches-by-date
suppressions/README.txt
suppressions/nokogiri_ruby-2.supp
<<<<<<< HEAD
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove git conflict mark here.

Manifest.txt Outdated
@@ -368,3 +351,5 @@ test/xml/test_xinclude.rb
test/xml/test_xpath.rb
test/xslt/test_custom_functions.rb
test/xslt/test_exception_handling.rb
=======
>>>>>>> Make builds minimal
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove git conflict mark here.

@stevecrozz
Copy link
Contributor Author

Thanks @junaruga. Updated.

@flavorjones
Copy link
Member

@stevecrozz Can you please rebase on master? We fixed some things in 464717b related to JRuby 9.2 backwards incompatibilities that are making this PR fail tests on JRuby 9.2.

@stevecrozz
Copy link
Contributor Author

No prob @flavorjones. I force-pushed a newly rebased branch.

@flavorjones
Copy link
Member

Woo, that went green. OK, I may try to get this into v1.9.0. Thanks!

@flavorjones flavorjones added this to the v1.9.0 milestone Dec 10, 2018
flavorjones added a commit that referenced this pull request Dec 17, 2018
for #1711 and #1719

[skip ci]
@flavorjones
Copy link
Member

I've merged this manually! Will be in v1.9.0. Thanks again for your patience and for submitting this PR!

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

Successfully merging this pull request may close these issues.

5 participants