Spec URL: http://mtasaka.fedorapeople.org/Review_request/rubygem-mini_portile/rubygem-mini_portile.spec SRPM URL: http://mtasaka.fedorapeople.org/Review_request/rubygem-mini_portile/rubygem-mini_portile-0.5.1-1.fc.src.rpm Description: Simplistic port-like solution for developers. It provides a standard and simplified way to compile against dependency libraries without messing up your system. Fedora Account System Username: mtasaka Koji scratch build: F-20 http://koji.fedoraproject.org/koji/taskinfo?taskID=5670886 F-19 http://koji.fedoraproject.org/koji/taskinfo?taskID=5670888
Hi Mamoru, Thanks for submitting the package. Here's a couple questions. 1. In %prep, I haven't seen this "tmpunpackdir" convention in many other gems, and I don't see it referenced in the Ruby packaging guidelines. If this convention is necessary for mini_portile, could you please add a comment to the .spec file indicating why this is necessary? 2. There is a section in %prep where you run "chmod 0644" on all the .rb files. From what I can see by running "gem unpack" by hand on mini_portile-0.5.1.gem, the single .rb file is already 644. In other words, it looks to me as if this step is unnecessary. Can you remove it from your .spec file? 3. From what I read in the current Ruby packaging guidelines, "gem build" is supposed to happen in %build. In your package, it happens in %prep. If there is a reason for this deviation, can you please clarify it in a comment in the .spec? 4. I'm curious about this line: %doc %{gem_instdir}/[A-Z]* Why not just specify LICENSE.txt explicitly here? From my understanding of the Ruby packaging guidelines, the History.txt and README.rdoc files belong in the -doc subpackage. 5. Regarding these lines: # Currently no useful %exclude %{gem_instdir}/examples/ The comment should read "Currently not useful". Also, can you please expand the comment to explain why it would not be useful to ship the example Rakefile in that directory?
Hello, Thank you for initial comments! (In reply to Ken Dreyer from comment #1) > Hi Mamoru, > > Thanks for submitting the package. Here's a couple questions. > > 1. In %prep, I haven't seen this "tmpunpackdir" convention in many other > gems, and I don't see it referenced in the Ruby packaging guidelines. If > this convention is necessary for mini_portile, could you please add a > comment to the .spec file indicating why this is necessary? This is my convension, to explicitly clarify that unpacking gem file is done under the directory created by %setup -q -c -T (i.e. to make it sure that no files are left after %clean is done: using %setup after unpacking gem is confusing, and I remember that old rpm unpacked source on unexpected directory when %setup was not yet called) > 2. There is a section in %prep where you run "chmod 0644" on all the .rb > files. From what I can see by running "gem unpack" by hand on > mini_portile-0.5.1.gem, the single .rb file is already 644. In other words, > it looks to me as if this step is unnecessary. Can you remove it from your > .spec file? For safety. I have seen many times that .rb's have executable permission. > 3. From what I read in the current Ruby packaging guidelines, "gem build" is > supposed to happen in %build. In your package, it happens in %prep. If there > is a reason for this deviation, can you please clarify it in a comment in > the .spec? Because I clean up tmpunpack dir. Here I want to make it sure that no garbage files are left. (note that gem build is to repackage gem, and not building something actually) > 4. I'm curious about this line: > %doc %{gem_instdir}/[A-Z]* > > Why not just specify LICENSE.txt explicitly here? From my understanding of > the Ruby packaging guidelines, the History.txt and README.rdoc files belong > in the -doc subpackage. README.rdoc must be in main (this is why "README" exists). Also it is quite common that "History" file is in main. > 5. Regarding these lines: > > # Currently no useful > %exclude %{gem_instdir}/examples/ > > The comment should read "Currently not useful". Also, can you please expand > the comment to explain why it would not be useful to ship the example > Rakefile in that directory? Rakefile is something like "Makefile" on autotool-based tarballs, which we don't usually package into binary rpms (also calling "rake" is discouraged in current guideline)
(In reply to Mamoru TASAKA from comment #2) > Hello, Thank you for initial comments! > > (In reply to Ken Dreyer from comment #1) > > Hi Mamoru, > > > > Thanks for submitting the package. Here's a couple questions. > > > > 1. In %prep, I haven't seen this "tmpunpackdir" convention in many other > > gems, and I don't see it referenced in the Ruby packaging guidelines. If > > this convention is necessary for mini_portile, could you please add a > > comment to the .spec file indicating why this is necessary? > > This is my convension, to explicitly clarify that unpacking gem file is > done under the directory created by %setup -q -c -T > (i.e. to make it sure that no files are left after %clean is done: > using %setup after unpacking gem is confusing, and I remember that > old rpm unpacked source on unexpected directory when %setup was not > yet called) That is good convention. We should similarly update the packaging guidelines. Although it would be worth of some macros, since the things gets a bit ugly. > > 2. There is a section in %prep where you run "chmod 0644" on all the .rb > > files. From what I can see by running "gem unpack" by hand on > > mini_portile-0.5.1.gem, the single .rb file is already 644. In other words, > > it looks to me as if this step is unnecessary. Can you remove it from your > > .spec file? > > For safety. I have seen many times that .rb's have executable permission. If it was me, I would correct only stuff rpmlint reports. Doing some actions without real need is superfluous and does .spec files less readable. But this is not blocker. > > 3. From what I read in the current Ruby packaging guidelines, "gem build" is > > supposed to happen in %build. In your package, it happens in %prep. If there > > is a reason for this deviation, can you please clarify it in a comment in > > the .spec? > > Because I clean up tmpunpack dir. Here I want to make it sure that no > garbage files are left. (note that gem build is to repackage gem, and not > building something actually) This is related to first point and it does not matter in which section the "build" is done. > > 4. I'm curious about this line: > > %doc %{gem_instdir}/[A-Z]* > > > > Why not just specify LICENSE.txt explicitly here? From my understanding of > > the Ruby packaging guidelines, the History.txt and README.rdoc files belong > > in the -doc subpackage. > > README.rdoc must be in main (this is why "README" exists). Also it is quite > common that "History" file is in main. I agree with Ken at this point, but we might disagree. This is not a blocker. > > 5. Regarding these lines: > > > > # Currently no useful > > %exclude %{gem_instdir}/examples/ > > > > The comment should read "Currently not useful". Also, can you please expand > > the comment to explain why it would not be useful to ship the example > > Rakefile in that directory? > > Rakefile is something like "Makefile" on autotool-based tarballs, which we > don't usually package into binary rpms (also calling "rake" is discouraged > in current guideline) This is example in its real sense, i.e. it pretty much corresponds with appropriate section of README.rdoc [1] and it is out of question that it should stay in -doc subpackage. [1] https://github.com/luislavena/mini_portile#how-can-i-combine-this-with-my-compilation-task
Sorry for very late response. (In reply to Vít Ondruch from comment #3) > (In reply to Mamoru TASAKA from comment #2) > > Hello, Thank you for initial comments! > > > > (In reply to Ken Dreyer from comment #1) > > > Hi Mamoru, > > > > > > Thanks for submitting the package. Here's a couple questions. > > > > > > 1. In %prep, I haven't seen this "tmpunpackdir" convention in many other > > > gems, and I don't see it referenced in the Ruby packaging guidelines. If > > > this convention is necessary for mini_portile, could you please add a > > > comment to the .spec file indicating why this is necessary? > > > > This is my convension, to explicitly clarify that unpacking gem file is > > done under the directory created by %setup -q -c -T > > (i.e. to make it sure that no files are left after %clean is done: > > using %setup after unpacking gem is confusing, and I remember that > > old rpm unpacked source on unexpected directory when %setup was not > > yet called) > > That is good convention. We should similarly update the packaging > guidelines. Although it would be worth of some macros, since the things gets > a bit ugly. "That" is _my_ ? Anyway actually old rpm behavior _does_ cause issues when %setup is not done beforehand, now I really recalled this: http://www.redhat.com/archives/rhl-devel-list/2008-February/msg02054.html It is much preferable that * %setup is done first * all %prep action is guaranteed to be done under the directory that %prep is created And I don't think this is a blocker. > > > 3. From what I read in the current Ruby packaging guidelines, "gem build" is > > > supposed to happen in %build. In your package, it happens in %prep. If there > > > is a reason for this deviation, can you please clarify it in a comment in > > > the .spec? > > > > Because I clean up tmpunpack dir. Here I want to make it sure that no > > garbage files are left. (note that gem build is to repackage gem, and not > > building something actually) > > This is related to first point and it does not matter in which section the > "build" is done. See above. > > > 5. Regarding these lines: > > > > > > # Currently no useful > > > %exclude %{gem_instdir}/examples/ > > > > > > The comment should read "Currently not useful". Also, can you please expand > > > the comment to explain why it would not be useful to ship the example > > > Rakefile in that directory? > > > > Rakefile is something like "Makefile" on autotool-based tarballs, which we > > don't usually package into binary rpms (also calling "rake" is discouraged > > in current guideline) > > This is example in its real sense, i.e. it pretty much corresponds with > appropriate section of README.rdoc [1] and it is out of question that it > should stay in -doc subpackage. Yeah, this is really "example" Rakefile, good to be in -doc. I will fix this when I import this into Fedora git (unless there is anything to be fixed beforehand)
(In reply to Mamoru TASAKA from comment #4) > "That" is _my_ ? Anyway actually old rpm behavior _does_ cause issues when > %setup is not done beforehand, now I really recalled this: > > http://www.redhat.com/archives/rhl-devel-list/2008-February/msg02054.html > > It is much preferable that > * %setup is done first > * all %prep action is guaranteed to be done under the directory that %prep > is created > > And I don't think this is a blocker. Ah, up until now, I thought you are workarounding different problem. In that case, I must say I am not fond of this approach, since I very much prefer mock, which will never suffer this issue. But if you think it helps, it is not a blocker for me (I am not fan of this gem rebuild stuff anyway). Otherwise the package looks good => APPROVED. Please fix the Rakefile example prior importing.
Thank you! New Package SCM Request ======================= Package Name: rubygem-mini_portile Short Description: Simplistic port-like solution for developers Owners: mtasaka Branches: f18 f19 f20 InitialCC:
Git done (by process-git-requests).
Already imported, closing. Thank you for review.