Bug 1242056

Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=10332041

Self introduction email: https://lists.fedoraproject.org/pipermail/devel/2015-July/212228.html

Hi Athos, as promised I will review your package.
But before sponsoring you, I would like to see at
least one informal package review from you.

Just take any package you feel comfortably with, from
http://fedoraproject.org/PackageReviewStatus/NEW.html
then, do not become owner, just run fedora-review
on it, and add your comments to the bug report.

Please let me know when you do so :)

Some extra comments. I see you were following documentation :)
Since I am actually not much experienced with ruby, I was
checking all build stages, and noticed that %gem_install
receives a -d argument, that would avoid installing under
%_builddir and then copying to %buildroot, but I just found,
again, the problems described at
http://fedoraproject.org/wiki/Archive:PackagingDrafts/RubyGem_with_C_code
e.g.
Binary file /home/pcpa/rpmbuild/BUILDROOT/rubygem-chake-0.7-1.fc23.x86_64/usr/share/gems/doc/chake-0.7/rdoc/js/searcher.js.gz matches

Properly fixing it, e.g. by somehow using the --build-root
option would require changing/patching gem itself. So, it is
not an issue for the review.

I noticed another issue, this time in fedora-review ruby plugin,
that has a regex for '^.*gem\s+install', that matches the
"gem install" text inside a comment, so, this, along with comments
about behaviour of %gem_install could be a long time TODO, as you
become more involved with Fedora ruby packages :)


Summarizing.

Please fix:

[!]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf %{buildroot} present but not required

    The line

    find %{buildroot}%{gem_instdir}/bin -type f | xargs chmod a+x

    does not appear to be required as well, for this specific package.


Please comment:

[?]: Development files must be in a -devel package

   Can you guarantee all files installed are required for runtime? Or
some are only need for development?


Note, when submitting an update, please bump the Release and add the
changes to the %changelog.

Hi Paulo, Thank you for your Review!

> But before sponsoring you, I would like to see at
> least one informal package review from you.

Here is my first review attempt:
https://bugzilla.redhat.com/show_bug.cgi?id=1243507

I also replied in this request where sources were not available, I was not sure what to do with the ticket status though:
https://bugzilla.redhat.com/show_bug.cgi?id=1229913

> Please fix:

> [!]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
>      beginning of %install.
>      Note: rm -rf %{buildroot} present but not required

>     The line

>     find %{buildroot}%{gem_instdir}/bin -type f | xargs chmod a+x

>     does not appear to be required as well, for this specific package.

Done and bumped the release. (shoud I bump it even if it wasn't uploaded to fedora cgit? i.e. during the review process?)

New files are at:
Spec URL: https://ribeiro.fedorapeople.org/rubygem-chake.spec
SRPM URL: https://ribeiro.fedorapeople.org/rubygem-chake-0.7-2.fc22.src.rpm

> Please comment:

> [?]: Development files must be in a -devel package

>    Can you guarantee all files installed are required for runtime? Or
> some are only need for development?

Yes. We could question the chake.spec.erb file (which is a template to generate a spec file) presence in there, but it is called by the Rakefile which is installed by the -doc package, which means that if we move that to a devel package the -doc package would require the -devel one. It is in the gem in order to generate rpm packages from it. For the next version I will submit a patch to change this rpm generation (I am already having this conversation with upstream).

> I noticed another issue, this time in fedora-review ruby plugin,
> that has a regex for '^.*gem\s+install', that matches the
> "gem install" text inside a comment, so, this, along with comments
> about behaviour of %gem_install could be a long time TODO, as you
> become more involved with Fedora ruby packages :)

I will take a look into this one, Thank you!

Just attaching a link for the new koji build, sorry for all the spamming.
http://koji.fedoraproject.org/koji/taskinfo?taskID=10373324

Hi Athos,

Sorry for the delay replying. I was very busy this week :)

A release bump is required even if not yet making an
official build. It is required, otherwise, it is very
to get confused. I personally asked this before, because
writing -0.1, -0.2 ... -0.3, and the -1 in final release
works, but a real bump was considered preferred.

You are doing very fine. But the 2 reviews were a bit too
weak. I suggest doing a review of some ruby related
package, so that you would better show your knowledge.

I believe it is almost done about review, but I will ask
you, to send an email to fedora-devel, or any ruby SIG,
for an explanation of the files
/usr/share/gems/doc/chake-0.7/rdoc/fonts/*.ttf
these files apparently are from
$ rpm -ql rubygem-rdoc|grep ttf
/usr/share/gems/gems/rdoc-4.2.0/lib/rdoc/generator/template/darkfish/fonts/Lato-Light.ttf
/usr/share/gems/gems/rdoc-4.2.0/lib/rdoc/generator/template/darkfish/fonts/Lato-LightItalic.ttf
/usr/share/gems/gems/rdoc-4.2.0/lib/rdoc/generator/template/darkfish/fonts/Lato-Regular.ttf
/usr/share/gems/gems/rdoc-4.2.0/lib/rdoc/generator/template/darkfish/fonts/Lato-RegularItalic.ttf
/usr/share/gems/gems/rdoc-4.2.0/lib/rdoc/generator/template/darkfish/fonts/SourceCodePro-Bold.ttf
/usr/share/gems/gems/rdoc-4.2.0/lib/rdoc/generator/template/darkfish/fonts/SourceCodePro-Regular.ttf

and there are several policies about fonts in fedora,
and these being duplicates is not good. See

https://fedoraproject.org/wiki/Packaging:Guidelines#Avoid_bundling_of_fonts_in_other_packages
https://fedoraproject.org/wiki/Packaging:FontsPolicy#Package_layout_for_fonts

Hello, sorry for leaving this open for so much time, I have been really busy with my masters degree. I am able to be back contributing and am working on other packages as well.

> You are doing very fine. But the 2 reviews were a bit too
> weak. I suggest doing a review of some ruby related
> package, so that you would better show your knowledge.

New reviews on the way.

> 
> I believe it is almost done about review, but I will ask
> you, to send an email to fedora-devel, or any ruby SIG,
> for an explanation of the files
> /usr/share/gems/doc/chake-0.7/rdoc/fonts/*.ttf
> these files apparently are from
> $ rpm -ql rubygem-rdoc|grep ttf
> /usr/share/gems/gems/rdoc-4.2.0/lib/rdoc/generator/template/darkfish/fonts/
> Lato-Light.ttf
> /usr/share/gems/gems/rdoc-4.2.0/lib/rdoc/generator/template/darkfish/fonts/
> Lato-LightItalic.ttf
> /usr/share/gems/gems/rdoc-4.2.0/lib/rdoc/generator/template/darkfish/fonts/
> Lato-Regular.ttf
> /usr/share/gems/gems/rdoc-4.2.0/lib/rdoc/generator/template/darkfish/fonts/
> Lato-RegularItalic.ttf
> /usr/share/gems/gems/rdoc-4.2.0/lib/rdoc/generator/template/darkfish/fonts/
> SourceCodePro-Bold.ttf
> /usr/share/gems/gems/rdoc-4.2.0/lib/rdoc/generator/template/darkfish/fonts/
> SourceCodePro-Regular.ttf
> 
> and there are several policies about fonts in fedora,
> and these being duplicates is not good. See
> 
> https://fedoraproject.org/wiki/Packaging:
> Guidelines#Avoid_bundling_of_fonts_in_other_packages
> https://fedoraproject.org/wiki/Packaging:FontsPolicy#Package_layout_for_fonts

As you can see here http://paste.fedoraproject.org/369093/46379839/ and here http://paste.fedoraproject.org/369092/63798193/raw/, this generated font files are replicated in 526 other rubygem packages. I did ask in #fedora-ruby about it and they told me there's a ticket open about this issue, and this may not be the best thing to do, but since these are generated files, it's ok.

Here are new files with upstream latest version:
Spec URL: https://ribeiro.fedorapeople.org/rubygem-chake.spec
SRPM URL: https://ribeiro.fedorapeople.org/rubygem-chake-0.13-4.fc25.src.rpm
COPR build: https://copr.fedorainfracloud.org/coprs/ribeiro/tests/build/316493/

Here is a more complete informal review https://bugzilla.redhat.com/show_bug.cgi?id=967338

Also, while searching for ruby packages to review, I realized that some old tickets do not have spec and srpm files available anymore, as in https://bugzilla.redhat.com/show_bug.cgi?id=1052524 and https://bugzilla.redhat.com/show_bug.cgi?id=1220103 and https://bugzilla.redhat.com/show_bug.cgi?id=997190.

What should be done whenever this happens? I mean, should these tickets remain open?

Also, here is the latest Koji build http://koji.fedoraproject.org/koji/taskinfo?taskID=14237125

  Hi Athos,

  Please comment on this:
W: simplecov not installed, we won't have a coverage report
maybe it needs this build requires?
rubygem-simplecov.noarch : Code coverage analysis tool for Ruby 1.9

  There is also this issue:
https://rubygems.org/gems/chake-0.13.gem :
  CHECKSUM(SHA256) this package     : 6a3ae97b0efbc40eed8de527c5345ecfea2786c8ef327a46cd5f8bbe9102897e
  CHECKSUM(SHA256) upstream package : d3726ddb2293edc6ad056272cb9ef2f159fdc6a3fd48f17a5bed49d708fbfd4f
Apparently the file was uploaded again, with same name, but
different contents, and version in srpm does not match download
link.

  I see the -doc package is installing files under:
/usr/share/gems/gems/chake-0.13/
I believe this is incorrect. Are you sure the main package runs
without the files installed there?

  Please also comment on the directory:
/usr/share/gems/doc/chake-0.13/ri
is it really required by the -doc package? Either way, what is
installing in /usr/share/gems/doc/chake-0.13/ should be installed
in /usr/share/doc/chake

  About the fonts, I believe the bug report is
https://bugzilla.redhat.com/show_bug.cgi?id=1224715 and it will
not get any attention, as the linked, related upstream report
is closed https://github.com/rdoc/rdoc/issues/186 as they
apparently had a different idea about it.
Please check that just adding a 'Requires: lato-fonts' is not
enough to display the documentation, and if not enough, please
check what kind of patch could be done, apparently only in the
*.css files.
I understand it is replicated in more than 500 packages, but
that is not correct :(

  Please check https://fedoraproject.org/wiki/Packaging:Ruby
the template there installs documentation under:
$ rpm -E %_defaultdocdir
/usr/share/doc
what should be done by the sample command in the sample spec:
rdoc --op %{buildroot}%{_defaultdocdir}/%{name}

  You should likely also join the ruby SIG, and check the tools
there, as well as other documentation:
https://fedoraproject.org/wiki/Ruby_SIG

  I believe you are doing good, but I will prefer to have you
knowing well about all packaging details before approving the
package.

  About the reviews with no longer srpm or spec, what you did
is fine, just comment about it in the bug report :)

  Your informal review of a sample rubygem package is also good.

(In reply to Paulo Andrade from comment #12)
>   Hi Athos,

Hi Paulo, thank you for the feedback.

> 
>   Please comment on this:
> W: simplecov not installed, we won't have a coverage report
> maybe it needs this build requires?
> rubygem-simplecov.noarch : Code coverage analysis tool for Ruby 1.9

simplecov is a gem to measure test coverage. I don't see any advantage on measuring test coverage during the build step since the output would be ignored. It would also generate a "coverege" directory which would have to be removed. see https://github.com/colszowka/simplecov for reference.

> 
>   There is also this issue:
> https://rubygems.org/gems/chake-0.13.gem :
>   CHECKSUM(SHA256) this package     :
> 6a3ae97b0efbc40eed8de527c5345ecfea2786c8ef327a46cd5f8bbe9102897e
>   CHECKSUM(SHA256) upstream package :
> d3726ddb2293edc6ad056272cb9ef2f159fdc6a3fd48f17a5bed49d708fbfd4f
> Apparently the file was uploaded again, with same name, but
> different contents, and version in srpm does not match download
> link.

Sorry for that, the new build (link below) has the right source and the checksums match

> 
>   I see the -doc package is installing files under:
> /usr/share/gems/gems/chake-0.13/
> I believe this is incorrect. Are you sure the main package runs
> without the files installed there?

Yes,

the -doc files under /usr/share/gems/gems/chake-0.13/ are

/usr/share/gems/gems/chake-0.13/examples
	which is a directory with examples on how to use chake

/usr/share/gems/gems/chake-0.13/spec
	which is a directory with integration tests

/usr/share/gems/gems/chake-0.13/ChangeLog.md
/usr/share/gems/gems/chake-0.13/README.md
	upstream changelog and readme files

/usr/share/gems/gems/chake-0.13/Rakefile
	Which is a file for rake (make for ruby, so this would be something like a Makefile)
/usr/share/gems/gems/chake-0.13/Gemfile
	File defining all dependencies of the project
/usr/share/gems/gems/chake-0.13/chake.gemspec
	File that defines the gem

These files usually go in the -doc in other packaged gems.

> 
>   Please also comment on the directory:
> /usr/share/gems/doc/chake-0.13/ri
> is it really required by the -doc package? Either way, what is
> installing in /usr/share/gems/doc/chake-0.13/ should be installed
> in /usr/share/doc/chake

The ri directory contains the ri documentation for the gem. ri documentation can be browsed through ri calls on the command line.

About being installed in /usr/share/doc/chake:

As you can see here (f23):
$ dnf repoquery --repoid=fedora -l rubygem-*-doc | grep -i "^/usr/share/gems/doc/" | cut -d/ -f6 | uniq | wc -l
531
$ dnf repoquery --repoid=fedora rubygem-*-doc | grep rubygem | wc -l
533

only 2 out of 533 gems do not install documentation in /usr/share/gems/doc, shouldn't I also follow this pattern?

note that %{gem_docdir} expands to /usr/share/gems/gems/NAME-VERSION, as defined in https://fedoraproject.org/wiki/Packaging:Ruby#Macros.

> 
>   About the fonts, I believe the bug report is
> https://bugzilla.redhat.com/show_bug.cgi?id=1224715 and it will
> not get any attention, as the linked, related upstream report
> is closed https://github.com/rdoc/rdoc/issues/186 as they
> apparently had a different idea about it.

I see...

> Please check that just adding a 'Requires: lato-fonts' is not
> enough to display the documentation, and if not enough, please
> check what kind of patch could be done, apparently only in the
> *.css files.
> I understand it is replicated in more than 500 packages, but
> that is not correct :(

I removed the generated fonts, required the two packages wich contain them and added links to these files, how about that? I am also checking with Ruby SIG if that approach would be feasible for all the other 500+ -doc packages.

> 
>   Please check https://fedoraproject.org/wiki/Packaging:Ruby
> the template there installs documentation under:
> $ rpm -E %_defaultdocdir
> /usr/share/doc
> what should be done by the sample command in the sample spec:
> rdoc --op %{buildroot}%{_defaultdocdir}/%{name}

The example shown is for ruby applications, there are other instructions for packaging gems there. As mentioned above, there is a macro defined with the path for gems documentations in https://fedoraproject.org/wiki/Packaging:Ruby#Macros

> 
>   You should likely also join the ruby SIG, and check the tools
> there, as well as other documentation:
> https://fedoraproject.org/wiki/Ruby_SIG

Done!

> 
>   I believe you are doing good, but I will prefer to have you
> knowing well about all packaging details before approving the
> package.
> 
>   About the reviews with no longer srpm or spec, what you did
> is fine, just comment about it in the bug report :)

I was thinking about parsing all the open review requests tickets and upload a page with information on those, but I believe this should be discussed in a new ticket, right?

> 
>   Your informal review of a sample rubygem package is also good.

New sources:

Spec URL: https://ribeiro.fedorapeople.org/rubygem-chake.spec
SRPM URL: https://ribeiro.fedorapeople.org/rubygem-chake-0.13-6.fc25.src.rpm
Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=14336411

(In reply to Paulo Andrade from comment #4)

> I noticed another issue, this time in fedora-review ruby plugin,
> that has a regex for '^.*gem\s+install', that matches the
> "gem install" text inside a comment, so, this, along with comments
> about behaviour of %gem_install could be a long time TODO, as you
> become more involved with Fedora ruby packages :)

I just sent a patch for that.

https://github.com/timlau/FedoraReview/pull/24

Still not the best fix for that, but we should stop getting those annoying warnings on the %{gem_install} macro.

(In reply to Athos Ribeiro from comment #13)
> (In reply to Paulo Andrade from comment #12)
> >   Hi Athos,
> 
> Hi Paulo, thank you for the feedback.
> 
> > 
> >   Please comment on this:
> > W: simplecov not installed, we won't have a coverage report
> > maybe it needs this build requires?
> > rubygem-simplecov.noarch : Code coverage analysis tool for Ruby 1.9
> 
> simplecov is a gem to measure test coverage. I don't see any advantage on
> measuring test coverage during the build step since the output would be
> ignored. It would also generate a "coverege" directory which would have to
> be removed. see https://github.com/colszowka/simplecov for reference.

That is right. We typically omit code coverage runs from the test suite. This is upstream stuff, we don't generally care about it.

Less dependencies for package build is always better.

> > 
> >   I see the -doc package is installing files under:
> > /usr/share/gems/gems/chake-0.13/
> > I believe this is incorrect. Are you sure the main package runs
> > without the files installed there?
> 
> Yes,
> 
> the -doc files under /usr/share/gems/gems/chake-0.13/ are
> 
> /usr/share/gems/gems/chake-0.13/examples
> 	which is a directory with examples on how to use chake
> 
> /usr/share/gems/gems/chake-0.13/spec
> 	which is a directory with integration tests
> 
> /usr/share/gems/gems/chake-0.13/ChangeLog.md
> /usr/share/gems/gems/chake-0.13/README.md
> 	upstream changelog and readme files
> 
> /usr/share/gems/gems/chake-0.13/Rakefile
> 	Which is a file for rake (make for ruby, so this would be something like a
> Makefile)
> /usr/share/gems/gems/chake-0.13/Gemfile
> 	File defining all dependencies of the project
> /usr/share/gems/gems/chake-0.13/chake.gemspec
> 	File that defines the gem
> 
> These files usually go in the -doc in other packaged gems.


This list and the split between main package and -doc subpackage looks reasonable to me.

> > 
> >   Please also comment on the directory:
> > /usr/share/gems/doc/chake-0.13/ri
> > is it really required by the -doc package? Either way, what is
> > installing in /usr/share/gems/doc/chake-0.13/ should be installed
> > in /usr/share/doc/chake
> 
> The ri directory contains the ri documentation for the gem. ri documentation
> can be browsed through ri calls on the command line.
> 
> About being installed in /usr/share/doc/chake:
> 
> As you can see here (f23):
> $ dnf repoquery --repoid=fedora -l rubygem-*-doc | grep -i
> "^/usr/share/gems/doc/" | cut -d/ -f6 | uniq | wc -l
> 531
> $ dnf repoquery --repoid=fedora rubygem-*-doc | grep rubygem | wc -l
> 533
> 
> only 2 out of 533 gems do not install documentation in /usr/share/gems/doc,
> shouldn't I also follow this pattern?


The generated documentation must be available at these locations, since the rubygems tooling expects it to be there. For example "gem server" can server the documentation from there. Similarly, ther "ri" command knows these location to server the documentation properly.

> > 
> >   About the fonts, I believe the bug report is
> > https://bugzilla.redhat.com/show_bug.cgi?id=1224715 and it will
> > not get any attention, as the linked, related upstream report
> > is closed https://github.com/rdoc/rdoc/issues/186 as they
> > apparently had a different idea about it.
> 
> I see...
> 
> > Please check that just adding a 'Requires: lato-fonts' is not
> > enough to display the documentation, and if not enough, please
> > check what kind of patch could be done, apparently only in the
> > *.css files.
> > I understand it is replicated in more than 500 packages, but
> > that is not correct :(
> 
> I removed the generated fonts, required the two packages wich contain them
> and added links to these files, how about that? I am also checking with Ruby
> SIG if that approach would be feasible for all the other 500+ -doc packages.

While I agree this should be fixed, I don't think it is good idea to fix only single gem. I would prefer all or nothing.


> > 
> >   Please check https://fedoraproject.org/wiki/Packaging:Ruby
> > the template there installs documentation under:
> > $ rpm -E %_defaultdocdir
> > /usr/share/doc
> > what should be done by the sample command in the sample spec:
> > rdoc --op %{buildroot}%{_defaultdocdir}/%{name}
> 
> The example shown is for ruby applications, there are other instructions for
> packaging gems there. As mentioned above, there is a macro defined with the
> path for gems documentations in
> https://fedoraproject.org/wiki/Packaging:Ruby#Macros

Agree, this is correct explanation.

> >   About the reviews with no longer srpm or spec, what you did
> > is fine, just comment about it in the bug report :)

Please note that there is process for stalled reviews:

https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews

Thank you for the comments, Vít.

Should I go ahead and revert the latest modifications which remove and link the fonts? I will wait to see if Paulo has any comments on that.

Maybe removing and linking fonts could be done in the end of %{gem_install} with some checks to find out if the files are present, how about that?

(In reply to Athos Ribeiro from comment #16)
> Should I go ahead and revert the latest modifications which remove and link
> the fonts? I will wait to see if Paulo has any comments on that.

Yes, that is what I prefer, but lets wait for Paulo.

> Maybe removing and linking fonts could be done in the end of %{gem_install}
> with some checks to find out if the files are present, how about that?

I don't think that %gem_install is the right place, since the files installed by %gem_install are later moved into the final position, which could break some links etc. Also, there would need to be additional dependencies on the font packages specified for every package.

  Athos, sorry for the long delay.

  I will trust Vít observations. I was requiring a lot of details
fixed mostly because once a package is reviewed, packagers tend
to not fix the problems later. But I understand the problem is
not specific to your package, but with the way rubygem packages
are handled.

  I did a new full review, and not blocking the review, I would
only want to ask you to make sure the upstream file is no longer
replaced without a version change, and cosmetic changes would be
to correct mixed lines starting with spaces and tabs.

  There is a warning about dangling symlinks for the fonts, but
I believe it is ok. The way to silence rpmlint would be to
manually create and remove then in %post and %postun, but that
is usually error prone, and having the links in the package is
safer and easier to manage.

  The package is approved!

  I will sponsor you!

  I am currently in vacation, so this helped me to miss your
email, but shortly I will be available all the time in freenode
as pcpa, but also feel free to send me an email if you need
any help.

  I will do my duties as sponsor, and one of the is to watch your
bugzilla interaction :)

  Welcome as Fedora packager!

  Athos,

  Please follow now the instructions starting at
https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner

Thank you Paulo, I really appreciate all the time you dedicated helping me here.

I reverted the changes related to the fonts linking as observed by Vít.

Spec URL: https://ribeiro.fedorapeople.org/rubygem-chake.spec
SRPM URL: https://ribeiro.fedorapeople.org/rubygem-chake-0.13-7.fc25.src.rpm
Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=14736148
COPR build: https://copr.fedorainfracloud.org/coprs/ribeiro/fedora/build/364902/

I also requested the new package as you pointed out.

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/rubygem-chake

rubygem-chake-0.13-7.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-7c1fb1c7c1

rubygem-chake-0.13-7.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-7c1fb1c7c1

rubygem-chake-0.13-7.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.