Bug 989400

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.