Bug 676308 - Review Request: rubygem-net-scp - A pure Ruby implementation of the SCP client protocol
Summary: Review Request: rubygem-net-scp - A pure Ruby implementation of the SCP clien...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-02-09 13:26 UTC by Vít Ondruch
Modified: 2014-07-29 19:19 UTC (History)
5 users (show)

Fixed In Version: rubygem-net-scp-1.0.4-2.fc15
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-03-24 17:39:34 UTC
mtasaka: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Vít Ondruch 2011-02-09 13:26:17 UTC
Spec URL: http://people.redhat.com/vondruch/rubygem-net-scp.spec
SRPM URL: http://people.redhat.com/vondruch/rubygem-net-scp-1.0.4-1.fc14.src.rpm
Description: A pure Ruby implementation of the SCP client protocol

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

Comment 1 Mamoru TASAKA 2011-03-13 16:57:07 UTC
If you are still interested in importing this package into
Fedora, I will have a look at this one. Instead I will appreciate
it if you would review one of my review requests (e.g. bug 678678)

Some notes:

* Explicit version-specific Requires
  - Please consider if you really have to write explicit
    version-specific Requires like rubygems ">= 1.2".
    * Usually when packages shipped on currently supported
      Fedora branches all satisfy such version requirements,
      we don't add such explicit version Requires:
      https://fedoraproject.org/wiki/Packaging/Guidelines#Explicit_Requires

* Remove obsolete stuff
  - rm -rf %{buildroot} at the top of %install is no longer needed
    (if you intend to import this only on Fedora).

! Directly installing gem archive into %buildroot
  - I always recommend to install gem archive once under %_builddir
    to make it sure that no additional files are generated or files
    are not modified after installation is done.
    * For example, some testsuites generate additional executables
      or log files or modify files, which frequently confuses %files 
      entry or leads to rpmbuild failure.

    And because of the same reason, it is recommended that %buildroot
    is touched only in %install section (i.e. executing %check under
    %buildroot is discouraged).

Comment 2 Mamoru TASAKA 2011-03-13 18:01:01 UTC
* By the way, I also recommend to add "-V" option to "gem install"
  because it shows some debuging information.

Comment 3 Vít Ondruch 2011-03-17 10:53:45 UTC
(In reply to comment #1)
> If you are still interested in importing this package into
> Fedora, I will have a look at this one. Instead I will appreciate
> it if you would review one of my review requests (e.g. bug 678678)

Yes, I am still interested in import. Thank you for your comments. Here is updated version:

Spec URL: http://people.redhat.com/vondruch/rubygem-net-scp.spec
SRPM URL:
http://people.redhat.com/vondruch/rubygem-net-scp-1.0.4-2.fc15.src.rpm

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

> Some notes:
> 
> * Explicit version-specific Requires
>   - Please consider if you really have to write explicit
>     version-specific Requires like rubygems ">= 1.2".
>     * Usually when packages shipped on currently supported
>       Fedora branches all satisfy such version requirements,
>       we don't add such explicit version Requires:
>       https://fedoraproject.org/wiki/Packaging/Guidelines#Explicit_Requires

I removed the explicit versions. You are right.

> * Remove obsolete stuff
>   - rm -rf %{buildroot} at the top of %install is no longer needed
>     (if you intend to import this only on Fedora).

Done

> ! Directly installing gem archive into %buildroot
>   - I always recommend to install gem archive once under %_builddir
>     to make it sure that no additional files are generated or files
>     are not modified after installation is done.
>     * For example, some testsuites generate additional executables
>       or log files or modify files, which frequently confuses %files 
>       entry or leads to rpmbuild failure.
> 
>     And because of the same reason, it is recommended that %buildroot
>     is touched only in %install section (i.e. executing %check under
>     %buildroot is discouraged).

I agree with you if the gem has binary extension, but this is not the case.

Unfortunately your statement is double-edged sword. You want to ensure that the test suite you are executing is testing the gem, you are going to install to our users. However, during install phase, there are typically done some changes in folder structure, some adjustments in path etc. If this is true, then executing of test suite in build folders is worthless, because it does not ensure anything. There might be bugs introduces by the install step.

On the other hand, you can done the restructuring also during build phase, but then, what is the purpose of the install section? Just copy the already prepared folder structure? There is no point.

Comment 4 Mamoru TASAKA 2011-03-17 11:41:02 UTC
For just for the last comment (will check the whole thing later,
please wait for a moment...)

(In reply to comment #3)
> (In reply to comment #1)
> > ! Directly installing gem archive into %buildroot
> >   - I always recommend to install gem archive once under %_builddir
> >     to make it sure that no additional files are generated or files
> >     are not modified after installation is done.
> >     * For example, some testsuites generate additional executables
> >       or log files or modify files, which frequently confuses %files 
> >       entry or leads to rpmbuild failure.
> > 
> >     And because of the same reason, it is recommended that %buildroot
> >     is touched only in %install section (i.e. executing %check under
> >     %buildroot is discouraged).
> 
> I agree with you if the gem has binary extension, but this is not the case.

First of all, I have already seen some cases in which even 
noarch gem based srpm
- produced binary rpms containing unneeded files (such as test.log or some
  additional temp files generated during %check section)
  * This usually happens when %files entry just contain %geminstdir/
- or case to fail to build due to such addtional unneeded files
  (if you write %files entry rather verbosely as we recommend, this
   can happen)
- or even generates arch-dependent files during %check (although the package
  itself can be noarch)

test.log or so may be generated even with arch-independent gem-based rpms
(and you can find that this is not specific to rubygem based pkgs).
We have to make sure that %buildroot is not polluted during %check section.
So
- preparing everything before %install
- and generally %install is actually only for %install
- %check section uses %_builddir, not %buildroot
is the easiest way for ensuring this. 

> Unfortunately your statement is double-edged sword. You want to ensure that the
> test suite you are executing is testing the gem, you are going to install to
> our users. However, during install phase, there are typically done some changes
> in folder structure, some adjustments in path etc.
- So if some "large" structure change or so is needed on %install,
  it must be moved to %build, actually.

> If this is true, then
> executing of test suite in build folders is worthless, because it does not
> ensure anything. There might be bugs introduces by the install step.
- So see above. If such "large" change is needed on %install, it
  must be done in %build. Basically %install is for install (as the string
  says).


> On the other hand, you can done the restructuring also during build phase, but
> then, what is the purpose of the install section? Just copy the already
> prepared folder structure? There is no point.
- But this is not specific to rubygem based pkgs and we usually do so
  (i.e. %install is only for installation, i.e. install or cp commands). 
  Generally what is needed before installation should be done before
  %install. For autotools based pkgs,
  we
  - basically just unpack tarball on %prep (tar xf foo.tar.bz2)
  - do everything needed before installation at %build (configure -> make
    -> some other stuffs)
  - then in %install, basically just do "make install".
  - in %check, we usually actually do check under %_builddir

  So actually we want to follow this structure also when using rubygem, like
  - in %prep, just unpack gem
  - in %build, modify or create everything needed
  - then in %install, just install the needed files

  The problem is that we have no idea how to do this (gem install does
  what we usually do on %prep and %build). However anyway not installing
  files under %buildroot directly is preferred because of the reason I said
  above.

* Note that as you know, arch-dependent gem cannot be installed under
  %buildroot directly due to generating debuginfo rpm issue.

Comment 5 Mamoru TASAKA 2011-03-17 12:01:20 UTC
Note that I am not going to make this issue (i.e. my previous comment)
as a blocker.

Comment 6 Vít Ondruch 2011-03-17 12:20:48 UTC
(In reply to comment #4)
> For just for the last comment (will check the whole thing later,
> please wait for a moment...)
> 
> (In reply to comment #3)
> > (In reply to comment #1)
> > > ! Directly installing gem archive into %buildroot
> > >   - I always recommend to install gem archive once under %_builddir
> > >     to make it sure that no additional files are generated or files
> > >     are not modified after installation is done.
> > >     * For example, some testsuites generate additional executables
> > >       or log files or modify files, which frequently confuses %files 
> > >       entry or leads to rpmbuild failure.
> > > 
> > >     And because of the same reason, it is recommended that %buildroot
> > >     is touched only in %install section (i.e. executing %check under
> > >     %buildroot is discouraged).
> > 
> > I agree with you if the gem has binary extension, but this is not the case.
> 
> First of all, I have already seen some cases in which even 
> noarch gem based srpm
> - produced binary rpms containing unneeded files (such as test.log or some
>   additional temp files generated during %check section)
>   * This usually happens when %files entry just contain %geminstdir/
> - or case to fail to build due to such addtional unneeded files
>   (if you write %files entry rather verbosely as we recommend, this
>    can happen)
> - or even generates arch-dependent files during %check (although the package
>   itself can be noarch)
> 
> test.log or so may be generated even with arch-independent gem-based rpms
> (and you can find that this is not specific to rubygem based pkgs).
> We have to make sure that %buildroot is not polluted during %check section.
> So
> - preparing everything before %install
> - and generally %install is actually only for %install
> - %check section uses %_builddir, not %buildroot
> is the easiest way for ensuring this. 
> 

I agree that we have to avoid pollution. But at the end, if the test pollutes the %buildroot of %_builddir, you still have to either do cleanup or exclude these files from installing/copying. So building into %buildroot cannot save you and it is invalid argument in this discussion.

> > Unfortunately your statement is double-edged sword. You want to ensure that the
> > test suite you are executing is testing the gem, you are going to install to
> > our users. However, during install phase, there are typically done some changes
> > in folder structure, some adjustments in path etc.
> - So if some "large" structure change or so is needed on %install,
>   it must be moved to %build, actually.
> 
> > If this is true, then
> > executing of test suite in build folders is worthless, because it does not
> > ensure anything. There might be bugs introduces by the install step.
> - So see above. If such "large" change is needed on %install, it
>   must be done in %build. Basically %install is for install (as the string
>   says).
> 
> 
> > On the other hand, you can done the restructuring also during build phase, but
> > then, what is the purpose of the install section? Just copy the already
> > prepared folder structure? There is no point.
> - But this is not specific to rubygem based pkgs and we usually do so
>   (i.e. %install is only for installation, i.e. install or cp commands). 
>   Generally what is needed before installation should be done before
>   %install. For autotools based pkgs,
>   we
>   - basically just unpack tarball on %prep (tar xf foo.tar.bz2)
>   - do everything needed before installation at %build (configure -> make
>     -> some other stuffs)
>   - then in %install, basically just do "make install".
>   - in %check, we usually actually do check under %_builddir
> 
>   So actually we want to follow this structure also when using rubygem, like
>   - in %prep, just unpack gem
>   - in %build, modify or create everything needed
>   - then in %install, just install the needed files
> 
>   The problem is that we have no idea how to do this (gem install does
>   what we usually do on %prep and %build). However anyway not installing
>   files under %buildroot directly is preferred because of the reason I said
>   above.
> 
> * Note that as you know, arch-dependent gem cannot be installed under
>   %buildroot directly due to generating debuginfo rpm issue.

It is always good to follow parallels. So I agree that we should probably follow the %prep, %build and %install as you described above, although in this particular case it will be more work. But we should be strict about it, therefore I would like to see this in Ruby packaging guidelines, instead of this paragraph: "The %prep and %build sections of the specfile should be empty."

But I can't agree that the test suite should be always executed in %_builddir, because as I already pointed above, it might happen that you will not catch all possible bugs and you have to care about pollution anyway.

Comment 7 Mamoru TASAKA 2011-03-17 20:03:48 UTC
(In reply to comment #6)
> (In reply to comment #4)
>
> I agree that we have to avoid pollution. But at the end, if the test pollutes
> the %buildroot of %_builddir, you still have to either do cleanup or exclude
> these files from installing/copying. So building into %buildroot cannot save
> you and it is invalid argument in this discussion.

- Well, perhaps you are just confused about %_builddir vs %buildroot
  (I don't know what you mean here by "the %buildroot of %_builddir")
  Usually 
  * %_builddir is ~/rpmbuild/BUILD
    With normal %setup -q,
    - rpmbuild firstly moves to ~/rpmbuild/BUILD (%_builddir)
    - unpack tarball
    - then move to  ~/rpmbuild/BUILD/%name-%version
    - And then, when %build, %install, %check begins, rpmbuild
      will always move to ~/rpmbuild/BUILD/%name-%version first
  * On the other hand, %buildroot is usually ~/
    rpmbuild/BUILDROOT/%{name}-%{version}-%{release}-%{_arch}.
    On install, files must be installed under %buildroot.

  So what I am saying is that usually 
  - %install section should generally just do copy files under 
    ~/rpmbuild/BUILD into ~/rpmbuild/BUILDROOT
  - and do not touch files under ~/rpmbuild/BUILDROOT during %check
    and touch only files under ~/rpmbuild/BUILD
  And rpmbuild always do the scripts in the order of
  %prep -> %build -> %install -> %check (not %prep -> %build -> %check
  -> %install).

  So even if testsuite may pollute files under %_builddir on %check,
  files under %buildroot won't be polluted (again %install is done
  before %check).

  Also please check this:

  https://fedoraproject.org/wiki/Packaging/Guidelines#Scriplets_are_only_allowed_to_write_in_certain_directories

  (%_builddir and %buildroot are clearly distinguished and it is written
   that %check should not write anything under %buildroot).

> It is always good to follow parallels. So I agree that we should probably
> follow the %prep, %build and %install as you described above, although in this
> particular case it will be more work. But we should be strict about it,
> therefore I would like to see this in Ruby packaging guidelines, instead of
> this paragraph: "The %prep and %build sections of the specfile should be
> empty."

- Note that when containing C extension modules, the paragraph is already
  invalid (as written in ruby packaging guidelines). 
  Well, we may really have to revise current ruby packaging guideline
  (even for arch-independent gem based srpm).

> But I can't agree that the test suite should be always executed in %_builddir,
> because as I already pointed above, it might happen that you will not catch all
> possible bugs and you have to care about pollution anyway.

- Well, I don't think we can catch all possible bugs anyway...
  And for %buildroot pollution issue, please check my comments above.

Comment 8 Vít Ondruch 2011-03-18 10:14:16 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #4)
> >
> > I agree that we have to avoid pollution. But at the end, if the test pollutes
> > the %buildroot of %_builddir, you still have to either do cleanup or exclude
> > these files from installing/copying. So building into %buildroot cannot save
> > you and it is invalid argument in this discussion.
> 
> - Well, perhaps you are just confused about %_builddir vs %buildroot
>   (I don't know what you mean here by "the %buildroot of %_builddir")

This is clear to me.

>   Usually 
>   * %_builddir is ~/rpmbuild/BUILD
>     With normal %setup -q,
>     - rpmbuild firstly moves to ~/rpmbuild/BUILD (%_builddir)
>     - unpack tarball
>     - then move to  ~/rpmbuild/BUILD/%name-%version
>     - And then, when %build, %install, %check begins, rpmbuild
>       will always move to ~/rpmbuild/BUILD/%name-%version first
>   * On the other hand, %buildroot is usually ~/
>     rpmbuild/BUILDROOT/%{name}-%{version}-%{release}-%{_arch}.
>     On install, files must be installed under %buildroot.
> 
>   So what I am saying is that usually 
>   - %install section should generally just do copy files under 
>     ~/rpmbuild/BUILD into ~/rpmbuild/BUILDROOT
>   - and do not touch files under ~/rpmbuild/BUILDROOT during %check
>     and touch only files under ~/rpmbuild/BUILD
>   And rpmbuild always do the scripts in the order of
>   %prep -> %build -> %install -> %check (not %prep -> %build -> %check
>   -> %install).

I did not realized the order, you are right.

> 
>   So even if testsuite may pollute files under %_builddir on %check,
>   files under %buildroot won't be polluted (again %install is done
>   before %check).
> 
>   Also please check this:
> 
>  
> https://fedoraproject.org/wiki/Packaging/Guidelines#Scriplets_are_only_allowed_to_write_in_certain_directories
> 
>   (%_builddir and %buildroot are clearly distinguished and it is written
>    that %check should not write anything under %buildroot).

While I agree that pollution is wrong and the guideline is reasonable, I have the feeling that the guideline comes from C/C++ world. For such applications, you are typically executing test suite using "make test" and it is hard to adjust the test suite to run from different place, so nobody bothers. In this case the test suite ensures that the build was correct, but does not ensure that much that the installation itself will be working.

But we can do this with Ruby. We can try to ensure that the installation structure, i.e. the %buildroot, is correct, although it is not free of penalties. Even better, we should foster upstream in preventing pollution of the %buildroot, i.e. they should place temporary files on suitable places, such as /tmp folder.

> 
> > It is always good to follow parallels. So I agree that we should probably
> > follow the %prep, %build and %install as you described above, although in this
> > particular case it will be more work. But we should be strict about it,
> > therefore I would like to see this in Ruby packaging guidelines, instead of
> > this paragraph: "The %prep and %build sections of the specfile should be
> > empty."
> 
> - Note that when containing C extension modules, the paragraph is already
>   invalid (as written in ruby packaging guidelines). 
>   Well, we may really have to revise current ruby packaging guideline
>   (even for arch-independent gem based srpm).

+1

> 
> > But I can't agree that the test suite should be always executed in %_builddir,
> > because as I already pointed above, it might happen that you will not catch all
> > possible bugs and you have to care about pollution anyway.
> 
> - Well, I don't think we can catch all possible bugs anyway...

Everybody knows that, but we can't reconcile with it.

>   And for %buildroot pollution issue, please check my comments above.

Comment 9 Mamoru TASAKA 2011-03-18 11:28:16 UTC
(Anyway for now this [i.e. installing gem under %_builddir first] is for
 now just my recommendation and again I won't make this as a blocker).

Comment 10 Mamoru TASAKA 2011-03-19 19:11:09 UTC
Alright.

-------------------------------------------------------
    This package (rubygem-net-scp) is APPROVED by
    mtasaka
-------------------------------------------------------

Comment 11 Vít Ondruch 2011-03-21 12:51:07 UTC
Thank you for your review!

New Package SCM Request
=======================
Package Name: rubygem-net-scp
Short Description: A pure Ruby implementation of the SCP client protocol
Owners: vondruch
Branches: f15

Comment 12 Jason Tibbitts 2011-03-21 13:44:53 UTC
Git done (by process-git-requests).

Comment 13 Fedora Update System 2011-03-22 12:16:39 UTC
rubygem-net-scp-1.0.4-2.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/rubygem-net-scp-1.0.4-2.fc15

Comment 14 Fedora Update System 2011-03-23 04:44:33 UTC
rubygem-net-scp-1.0.4-2.fc15 has been pushed to the Fedora 15 testing repository.

Comment 15 Fedora Update System 2011-03-29 03:56:13 UTC
rubygem-net-scp-1.0.4-2.fc15 has been pushed to the Fedora 15 stable repository.

Comment 16 Troy Dawson 2014-07-29 18:58:16 UTC
Package Change Request
======================
Package Name: rubygem-net-scp
New Branches: epel7
Owners: tdawson

Comment 17 Gwyn Ciesla 2014-07-29 19:19:16 UTC
Git done (by process-git-requests).


Note You need to log in before you can comment on or make changes to this bug.