Bug 1244764

Hi Ilya,

Looking on the .spec file, I see a few small issues:

* Test suite is not executed.
  - It is good habit to execute the test suite, since it is the only way how to
    be sure that the code actually works. There is no compiler or anything else
    which would watch your back. You can refer to Ruby packaging guidelines [1]
    how to do this.

* You should not ship hidden files in you package.
  - There are excluded several dot files, but you missed the
    "%{gem_instdir}/.rspec"
  - In my packages, I usually remove all of them by simple:
    "%{gem_instdir}/.*"
  - This issue is reported by rpmlint, so I guess you did not run it. This is
    suggested by packaging guidelines [2] as well as review guidelines [3].

I can sponsor you, but I would appreciate, if you can take a look on some other packages and submit informal reviews to take more familiar with Fedora packaging process and guidelines. I am pretty sure you can find some other GitLab related packages already submitted for a review in Bugzilla.

BTW you lists two FAS nicks and the changelog states that the .spec file was prepared by Igor Gnatenko, so could you please clarify who is actually author and submitter etc. Just to be clear. Thank you.


[1] https://fedoraproject.org/wiki/Packaging:Ruby#Testing_frameworks_usage
[2] https://fedoraproject.org/wiki/Packaging:Guidelines#Use_rpmlint
[3] https://fedoraproject.org/wiki/Packaging:ReviewGuidelines
[4] https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Get_Sponsored

Hi Vit,

excuse, I am the author,I worked at the friend's computer.

I corrected issues (add execution test suite and exclude hidden files):
Spec URL: http://repo.clanwars.org/gitlab/rubygem-diffy.spec
SRPM URL: http://repo.clanwars.org/gitlab/rubygem-diffy-3.0.7-2.fc24.src.rpm
Description: Convenient diffing in ruby
Fedora Account System Username: ilgrad

Thank you.

Ok, thx for update. I have a few nitpicks:

* Execute the test suite in .%{gem_instdir}
  - Let me explain. Although with current guidelines and for this particular
    gem the test suite execution works as you did, it is generally better to
    execute the test suite in .%{gem_instdir}, e.g. the %check section should
    looks like:

      pushd .%{gem_instdir}
      rspec -Ilib spec
      popd

    The reason for this is that the current directory contains just the content
    of "gem unpack" command executed in %prep section, while the .%{gem_instdir}
    contains the content after installation. Although it is usually the same for
    noarch gems, for gems with binary extensions, the .%{gem_instdir} contains
    also the build extension, etc. It is also backward compatible practice from
    days, we have not used "gem unpack" command.

* Exclude %{gem_instdir}/diffy.gemspec
  - I would suggest to exclude the diffy.gemspec from the resulting package,
    since:

    1) It has no meaning.
    2) What is worser, it is not the original copy of the file originally
       shipped with the package, but this copy was created by the "gem spec"
       command.

BTW it is good habit to include Koji scratch build results ("$ fedpkg --dist f24 scratch-build --srpm"), to show that the package successfully builds in Fedora, e.g:

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

This is first good sign for package reviewer :)

Otherwise, the package looks good.

Prior I sponsor you, could you please fix appropriately our other packages you submitted for a review and also link some informal review of some other package you did?

Thank you.

Spec URL: http://repo.clanwars.org/gitlab/rubygem-diffy.spec
SRPM URL: http://repo.clanwars.org/gitlab/rubygem-diffy-3.0.7-3.fc24.src.rpm
Koji:     http://koji.fedoraproject.org/koji/taskinfo?taskID=10428185

Thanks for the update.

* Better description
  - I forgot to mention that the description is not really descriptive :/ Could
    you please update the description? Probably the first paragraph from
    README.md will do the job.

* Dependency on diff
  - Since I read the description now ( ;) ), I noticed that that the package
    depends on external diff. It might be good idea to include
    "Recommends: %{_bindir}/diff". On the other hand, I am not 100% sure if that
    is really needed because:

    1) diff is very likely available even in some minimal installation
    2) The case when diff is not available looks to be handled gracefully
       (although the message is probably not very clear).
    3) There could be used also rubygem-diff-lcs alternatively (but this is not
       very likely due to (1))

    So I leave it up to you, I just wanted to mention it here.

Spec URL: http://repo.clanwars.org/gitlab/rubygem-diffy.spec
SRPM URL: http://repo.clanwars.org/gitlab/rubygem-diffy-3.0.7-4.fc24.src.rpm

rubygem-diffy uses system's "diff" or "ldiff" (from %{_bindir}, tbh). "diff" is coming from package called "diffutils" and it present in minimal system (because if I'm trying to remove diffutils I'm getting errors about removing systemd and dnf). This means that Requires: /usr/bin/diff is not needed. About "ldiff".. It comes from "rubygem-diff-lcs". According to sources[0], it tries to find "diff" first and if it's not found - tries to use "ldiff", so if we will add Requires: /usr/bin/ldiff it will be not used at all, so it's unnecessary.


[0]https://github.com/samg/diffy/blob/master/lib/diffy/diff.rb#L151-L153

Ok, thx. The package looks good and is ready for approval from my POV. Could you please apply your newly gained experience to your other package as well?

unofficial review:
https://bugzilla.redhat.com/show_bug.cgi?id=1259532#c1

unofficial review:
https://bugzilla.redhat.com/show_bug.cgi?id=1262644#c1

Hi Ilya,

I see you posted several informal reviews as well as other packages, so to keep you busy, I sponsored you into packagers group.

And since this package looks good, I also APPROVE this package. Feel free to continue with SCM request [1] and let me know should you have any issues importing your first package.


[1] https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner

Please note that you can request new package directly in PkgDb [1] now, although it is undocumented :)


[1] https://admin.fedoraproject.org/pkgdb/

Hi Vit,

Thank you!)

This is already in Rawhide => closing.