Spec URL: https://ribeiro.fedorapeople.org/rubygem-chake.spec SRPM URL: https://ribeiro.fedorapeople.org/rubygem-chake-0.7-1.fc22.src.rpm Description: chake allows one to manage a number of hosts via SSH by combining chef (solo) and rake. It doesn't require a chef server; all you need is a workstation from where you can SSH into all your hosts. chake automates copying the configuration management repository to the target host (including managing encrypted files), running chef on them, and arbitraty commands on the hosts. Fedora Account System Username: Ribeiro
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.