[Bug 1244764] Review Request: rubygem-diffy - Convenient diffing in ruby
https://bugzilla.redhat.com/show_bug.cgi?id=1244764 Vít Ondruch changed: What|Removed |Added Status|ASSIGNED|CLOSED Fixed In Version||rubygem-diffy-3.0.7-4.fc24 Resolution|--- |RAWHIDE Last Closed||2015-09-29 01:43:15 --- Comment #13 from Vít Ondruch --- This is already in Rawhide => closing. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1244764] Review Request: rubygem-diffy - Convenient diffing in ruby
https://bugzilla.redhat.com/show_bug.cgi?id=1244764 --- Comment #12 from Ilya Gradina --- Hi Vit, Thank you!) -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1244764] Review Request: rubygem-diffy - Convenient diffing in ruby
https://bugzilla.redhat.com/show_bug.cgi?id=1244764 --- Comment #11 from Vít Ondruch --- Please note that you can request new package directly in PkgDb [1] now, although it is undocumented :) [1] https://admin.fedoraproject.org/pkgdb/ -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1244764] Review Request: rubygem-diffy - Convenient diffing in ruby
https://bugzilla.redhat.com/show_bug.cgi?id=1244764 Vít Ondruch changed: What|Removed |Added Flags|fedora-review? |fedora-review+ --- Comment #10 from Vít Ondruch --- 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 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1244764] Review Request: rubygem-diffy - Convenient diffing in ruby
https://bugzilla.redhat.com/show_bug.cgi?id=1244764 Vít Ondruch changed: What|Removed |Added Blocks|177841 (FE-NEEDSPONSOR) | Referenced Bugs: https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1244764] Review Request: rubygem-diffy - Convenient diffing in ruby
https://bugzilla.redhat.com/show_bug.cgi?id=1244764 --- Comment #9 from Ilya Gradina --- unofficial review: https://bugzilla.redhat.com/show_bug.cgi?id=1262644#c1 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1244764] Review Request: rubygem-diffy - Convenient diffing in ruby
https://bugzilla.redhat.com/show_bug.cgi?id=1244764 --- Comment #8 from Ilya Gradina --- unofficial review: https://bugzilla.redhat.com/show_bug.cgi?id=1259532#c1 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1244764] Review Request: rubygem-diffy - Convenient diffing in ruby
https://bugzilla.redhat.com/show_bug.cgi?id=1244764 Vít Ondruch changed: What|Removed |Added Status|NEW |ASSIGNED Assignee|nob...@fedoraproject.org|vondr...@redhat.com Flags||fedora-review? -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1244764] Review Request: rubygem-diffy - Convenient diffing in ruby
https://bugzilla.redhat.com/show_bug.cgi?id=1244764 --- Comment #7 from Vít Ondruch --- 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? -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1244764] Review Request: rubygem-diffy - Convenient diffing in ruby
https://bugzilla.redhat.com/show_bug.cgi?id=1244764
--- Comment #6 from Ilya Gradina ---
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
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1244764] Review Request: rubygem-diffy - Convenient diffing in ruby
https://bugzilla.redhat.com/show_bug.cgi?id=1244764
--- Comment #5 from Vít Ondruch ---
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.
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1244764] Review Request: rubygem-diffy - Convenient diffing in ruby
https://bugzilla.redhat.com/show_bug.cgi?id=1244764 --- Comment #4 from Ilya Gradina --- 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 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1244764] Review Request: rubygem-diffy - Convenient diffing in ruby
https://bugzilla.redhat.com/show_bug.cgi?id=1244764
--- Comment #3 from Vít Ondruch ---
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.
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1244764] Review Request: rubygem-diffy - Convenient diffing in ruby
https://bugzilla.redhat.com/show_bug.cgi?id=1244764 --- Comment #2 from Ilya Gradina --- 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1244764] Review Request: rubygem-diffy - Convenient diffing in ruby
https://bugzilla.redhat.com/show_bug.cgi?id=1244764
Vít Ondruch changed:
What|Removed |Added
CC||vondr...@redhat.com
--- Comment #1 from Vít Ondruch ---
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
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1244764] Review Request: rubygem-diffy - Convenient diffing in ruby
https://bugzilla.redhat.com/show_bug.cgi?id=1244764 Ilya Gradina changed: What|Removed |Added Blocks||177841 (FE-NEEDSPONSOR) Referenced Bugs: https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
