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

please ship specs in the gem #1855

Closed
boutil opened this issue Jan 8, 2019 · 22 comments
Closed

please ship specs in the gem #1855

boutil opened this issue Jan 8, 2019 · 22 comments

Comments

@boutil
Copy link
Contributor

boutil commented Jan 8, 2019

Hi

Until 1.8, nokogiri gem contained the spec files. It was useful for Debian (and certainly for other GNU/Linux distributions) to be able to run the test suite when building packages for their archive.

Since 1.9, the spec/ subdir is not included anymore. In Debian, we thought about using the github repository as the new source, but since it has no gemspec file (on purpose, as you explained already), byt has jar files, which we need to filter out, it makes things a bit tricky for packagers.

Therefore, we would like to kindly ask you if you could continue to ship the specs in the gem file.

Thank you in advance.

@flavorjones
Copy link
Member

Hi,

Thanks for bringing this up, and I apologize if this change caught you off-guard.

We did make reasonable efforts to discuss this publicly and get input into the decision. There was a public discussion on the mailing list and in a couple of different github issues about removing specs from the gem, and was met with general approval from users and downstream library maintainers. I released an RC with this change before shipping the final version. Apologies again if this was surprising, but then I'd encourage you to participate in the mailing list and/or github issues if you're repackaging Nokogiri.

Some history: this change was originally proposed on the mailing list in January 2018. That discussion resulted in #1711 and a PR at #1719. I invite you to read all those threads for context.

Nokogiri has been downloaded over 202 million times, so there is a real cost to shipping files in the gem that nobody uses (those costs are borne by the users of the gem as well as rubygems.org). I think at this point I would need to hear very compelling reasons for putting files in the gem that aren't necessary for installation, runtime, or licensing/legal reasons.

I don't know much about what's done by Debian and other distros to repackage Nokogiri, but I imagine that it wouldn't be too troublesome to either download the source tarball from the Github Releases page or to use git to access the tagged release in the repo, and run those tests.

Can you help me understand why it's not reasonable for re-packagers of Nokogiri to grab tests from the source repository? This is actually exactly what Nokogiri co-maintainer @larskanis does when he tests the pre-compiled binary releases of Windows, and what I'm in the middle of implementing for testing installed gems in our automated pipelines, so I'm not asking you to do anything that we're not already doing ourselves.

@flavorjones
Copy link
Member

One more note: I'd be happy to pair with you on updating your build scripts to merge the gem files of a certain version with the spec files from the same tagged version of the git repo. I'd like to learn more about how repackaging is being done by distro maintainers, so this might be a good way for us to build empathy with each other!

@flavorjones
Copy link
Member

I'd also be open to PRs that would make the above workflow easier to manage! For example, @larskanis proposed at one point that we decouple the compile and test rake tasks to make it easier to run tests against an installed gem; and I think that's a very reasonable idea.

@pravi
Copy link

pravi commented Jan 8, 2019

@flavorjones , @boutil wrote this in his request "In Debian, we thought about using the github repository as the new source, but since it has no gemspec file (on purpose, as you explained already), byt has jar files, which we need to filter out, it makes things a bit tricky for packagers." So if you can provide gemspec file in release tarballs, that will suffice our use case.

@pravi
Copy link

pravi commented Jan 8, 2019

We use gem2deb https://salsa.debian.org/ruby-team/gem2deb for building the deb file and till 1.8 version we did not have to do anything special for nokogiri.

@boutil
Copy link
Contributor Author

boutil commented Jan 8, 2019

@flavorjones thanks for your comments. We understand indeed the additional burden it was to provide the spec, and understand that going back is not a good idea from this point of view.
I completely missed this discussion, and will definitely join the mailing list to avoid other important issues.

As @pravi mentioned, we would be happy to use the github repository as the source. Filtering out the jar files can be done automatically. We would have the files under spec/ to run the test suite. The only missing bit for us would be a way to generate a simple gemspec (as we understand you don't want to include one #1471).

Do you confirm we could use the gem:spec target of the Rakefile to produce a viable gemspec? It is needed to ensure integration between rubygems and the Debian package system.

@pravi
Copy link

pravi commented Jan 8, 2019

@boutil I tried rake gem:spec and at least 'concourse' is not packaged, may be we can patch Rakefile to use only the libraries that are really required to generate the gemspec file. Alternatively if the gemspec from .gem is combined with the tar file from repo either by upstream or by modifying debian's fake upstream service, that could work as well.

@boutil
Copy link
Contributor Author

boutil commented Jan 8, 2019

We can comment out the part about concourse, and we would need then to package ruby-hoe-gemspec. This approach would be simpler than trying to combine two sources...

@junaruga
Copy link
Contributor

junaruga commented Jan 9, 2019

Since 1.9, the spec/ subdir is not included anymore. In Debian, we thought about using the github repository as the new source, but since it has no gemspec file (on purpose, as you explained already), byt has jar files, which we need to filter out, it makes things a bit tricky for packagers.

@boutil You can create the gemspec file by yourself.

$ gem fetch nokogiri
...
Downloaded nokogiri-1.10.0

$ ls nokogiri-1.10.0.gem
nokogiri-1.10.0.gem

$ gem specification nokogiri-1.10.0.gem --ruby > nokogiri.gemspec

@junaruga
Copy link
Contributor

junaruga commented Jan 9, 2019

Since 1.9, the spec/ subdir is not included anymore. In Debian, we thought about using the github repository as the new source, but since it has no gemspec file (on purpose, as you explained already), byt has jar files, which we need to filter out, it makes things a bit tricky for packagers.

In case of Fedora (RPM base Linux distribution), we are planning to use

  • 1st source: the gem file (nokogiri-*.gem)
  • 2nd source: tar.gz file including the spec/ (test/ ?) sub directory. nokogiri-1.10.0-tests.tgz That is prepared by below steps in advance.
$ git clone https://github.com/sparklemotion/nokogiri.git
$ cd nokogiri
$ git checkout v1.10.0
$ tar czvf nokogiri-1.10.0-tests.tgz test/

This is a common approach on Fedora project, because many gem packages including Rails does not include the test files in the gem file. And that makes sense.
Here are the cases of the RPM package config file.

I think you might be able to refer your deb package config files for Rails or some gem packages.

@junaruga
Copy link
Contributor

junaruga commented Jan 9, 2019

byt has jar files, which we need to filter out, it makes things a bit tricky for packagers." So if you can provide gemspec file in release tarballs, that will suffice our use case.

It might be tricky. But it can sometimes happen for some gem packages' RPM packaging process.
We Fedora Ruby packaging team is using below way to filter out the file.

Here is the case for execjs gem package's RPM config file to filter out files from the gemspec file.
We are using the sed command.

https://src.fedoraproject.org/rpms/rubygem-execjs/blob/master/f/rubygem-execjs.spec#_44

sed -i -e '/files/ s\|"lib/execjs/support/jscript_runner.js".freeze, \|\|' \
  | -e '/files/ s\|"lib/execjs/support/json2.js".freeze, \|\|' execjs.gemspec

We are also providing a common function (RPM macro %gemspec_remove_file) to do this operation in the packaging process.
Here is the case of the cucumber.
https://src.fedoraproject.org/rpms/rubygem-cucumber/blob/master/f/rubygem-cucumber.spec#_62

@flavorjones
Copy link
Member

flavorjones commented Jan 9, 2019

Hi all, I really appreciate the conversation here. I'd like to maybe state my opinion on a few of the options brought up:

  • rake gem:spec creates a test gemspec with a version like 1.10.1.20190101120103. This file will not be suitable for your purposes, I don't think. Hoe does not make it easy to have an external gemspec, but hoe-gemspec could be modified to do so if you'd like to send a PR there I'd be happy to accept it (I also maintain that). I won't be adding a real gemspec to the repository for reasons outlined in the repo file Y_U_NO_GEMSPEC.md.
  • Additionally that test gemspec also won't contain the test files, and so won't solve this problem by itself.
  • I prefer not to add tests back into the primary platform gems. Although I was initially hesitant about it, I've heard from enough people online and in RL that I now think it's a good and defensible decision, and one with real precedent if you look at Rails which is one of the flagship gems in the ecosystem.

I spent some time this morning looking at the gem2deb toolchain, and I understand now why it's not trivial to copy in the test directory; primarily this is because (again) the gemspec does not contain those test files.

As I see it, there are three options:

1. bring in the tests into the gem2deb toolchain process

I've written this script that almost does what you need, except for reopening the gem spec to add the test files (which should only take 10 more minutes or so to write that code):

#! /usr/bin/env bash

VERSION="1.9.1"
PATH_TO_SOURCE="${HOME}/code/oss/nokogiri"

if [[ ! -e nokogiri-${VERSION}.gem ]] ; then
  gem fetch nokogiri -v ${VERSION}
fi

if [[ ! -e nokogiri-${VERSION}.tar.gz ]] ; then
  gem2tgz nokogiri-${VERSION}.gem
fi

if [[ ! -d ruby-nokogiri-${VERSION} ]] ; then
  dh-make-ruby nokogiri-${VERSION}.tar.gz
fi

exit 0

pushd ruby-nokogiri-${VERSION}

  if [[ ! -d test ]] ; then
    cp -var ${PATH_TO_SOURCE}/test ruby-nokogiri-${VERSION}
    # TODO munge the gemspec here
    dpkg-source --commit
  fi

  dpkg-buildpackage -d -us -uc

popd

2. build from the source tree, but copy in the gemspec from the published gem

This still doesn't address the problem where that gemspec doesn't contain the test files, so litterally the exact same amount of work would need to be done here as in (1).

3. we could publish a variation on the Nokogiri gem that includes tests, for use by people who want to be able to run the tests on an installed gem or .deb package.

This might be the easiest thing to do. I'm investigating now.

@flavorjones
Copy link
Member

The idea behind 3 above is that you'd run `gem fetch nokogiri --platform=with-tests and get a gem file containing the tests.

I built a local gem with test files in it, and ran gem2deb on it and it seemed to work (though I don't understand how to run the tests, and can't seem to resolve ruby-mini-portile2).

@flavorjones
Copy link
Member

@boutil I'll note that it appears as though ruby-nokogiri is being built not against the debian libxml2/libxslt packages, but by compiling the patched vendored versions of those libraries. Is this what's intended? Why wouldn't you be building against system libraries? If you're not sure how to do that, let me know and I can help.

@flavorjones
Copy link
Member

Posting a test gem here. (It's a tarball because github won't allow me to post a .gem file.)

nokogiri-1.10.0-with_tests.gem.tar.gz

@boutil or @pravi can you let me know if it's satisfactory for your needs? If that works then I will try to publish something similar to rubygems.org and we can test it end-to-end.

@flavorjones
Copy link
Member

I've put the code that generate this gemfile into PR #1858 for visibility and review.

@boutil
Copy link
Contributor Author

boutil commented Jan 9, 2019

@flavorjones @junaruga thanks for your messages.

@junaruga I could generate the gemspec using your recipe and I think it would be sufficient for our need

gem2deb uses the gemspec files to ensure integration between rubygems and the package system: when installing gems, some dependencies can be satisfied using Ruby .deb packages installed on the system. Therefore it uses only the declaration of dependencies between gems, and not the list of files provided (anyway the path where files are installed on the system do not agree in general with the way they are set in the gem). So it is not a big issue if the list of files declared in the gemspec does not contain the test files.

So either solution 2 or 3 proposed by @flavorjones are equally good. I have a prototype using solution 2 for 1.10.0 (rebuilding the reverse dependencies), but I am willing to try the gem with test, if it does not cost too much work to you to generate. I'll report on this approach in a later message.

As a side note, we have some patches in Debian applied on top of the source code to remove usage ofr mini_portile2 and ensure that the code is built against system libraries, like this:
https://salsa.debian.org/ruby-team/ruby-nokogiri/blob/master/debian/patches/0003-Always-use-system-libraries.patch

@flavorjones
Copy link
Member

OK - @boutil I'll close this in a few days if I haven't heard back, since it sounds like you may be able to make this work without me shipping a special gem? For what it's worth, you can also generate the .gem with test files with this command, using the branch from #1858:

BUILD_GEM_WITH_TESTS=t bundle exec rake gem

@boutil
Copy link
Contributor Author

boutil commented Jan 16, 2019

@flavorjones I think you can indeed close. The approach proposed by @junaruga seems to fit our needs for the time being, and unfortunately, I won't have time in the coming days to play with your proposition, but I keep it bookmarked and certainly try it later. Thanks again for your help on this matter.

@junaruga
Copy link
Contributor

@boutil alright, good news :)

@flavorjones
Copy link
Member

OK. Thanks all. I'll close this issue, but leave the branch up for a while. Before deleting the branch I'll comment here to give people time to object.

@flavorjones
Copy link
Member

Noting, as I promised I would above, that I'm intending to delete the branch created for #1858 in a week or so, unless someone has objections.

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

No branches or pull requests

4 participants