Bug 912960

http://mtasaka.fedorapeople.org/Review_request/ruby-gnome2-suite/rubygem-gdk3.spec
http://mtasaka.fedorapeople.org/Review_request/ruby-gnome2-suite/rubygem-gdk3-1.2.3-1.fc.src.rpm

* Fri Mar 22 2013 Mamoru TASAKA <mtasaka> - 1.2.3-1
- 1.2.3

koji scratch build
F-20: http://koji.fedoraproject.org/koji/taskinfo?taskID=5155569
F-19: http://koji.fedoraproject.org/koji/taskinfo?taskID=5155570
F-18: http://koji.fedoraproject.org/koji/taskinfo?taskID=5155572

http://mtasaka.fedorapeople.org/Review_request/ruby-gnome2-suite/rubygem-gdk3-1.2.6-1.fc.src.rpm
http://mtasaka.fedorapeople.org/Review_request/ruby-gnome2-suite/rubygem-gdk3.spec

* Mon Apr 29 2013 Mamoru TASAKA <mtasaka> - 1.2.6-1
- 1.2.6

koji scratch build:
F-20: http://koji.fedoraproject.org/koji/taskinfo?taskID=5311540
F-19: http://koji.fedoraproject.org/koji/taskinfo?taskID=5311543
F-18: http://koji.fedoraproject.org/koji/taskinfo?taskID=5311528

http://mtasaka.fedorapeople.org/Review_request/ruby-gnome2-suite/rubygem-gdk3.spec
mtasaka.fedorapeople.org/Review_request/ruby-gnome2-suite/rubygem-gdk3-2.0.2-1.fc.src.rpm

* Sun Aug 25 2013 Mamoru TASAKA <mtasaka> - 2.0.2-1
- 2.0.2

Taking.

rpmlint:
rubygem-gdk3.x86_64: E: incorrect-fsf-address /usr/share/gems/gems/gdk3-2.0.2/COPYING.LIB
rubygem-gdk3.src: W: strange-permission gdk3-2.0.2.gem 0600L


> ruby -Ilib:test ./test/run-test.rb || echo "Please investigate this"
Why that ||?
If the test fail, it will not take down whole build. Which is what we generally want to, isn't it?


I'm really not sure about:
  %global header_dir  %{ruby_vendorarchdir}
this is not in guidelines, so it is just IMHO, but... I think (after consultation with vondrouch), that header files should go to:
["vendorarchhdrdir", "/usr/include/vendor_ruby/x86_64-linux"]
------------^^^ notice this
or 
$  pkg-config --variable=vendorhdrdir ruby
/usr/include/vendor_ruby
which is unfortunately not provided by any macro (as rubygem-foo-devel is not usual).
Can you consider moving header files there?

And if you provide -devel package, should you provide even so-name?

Thank you for initial comments

(In reply to Miroslav Suchý from comment #5)
> rpmlint:
> rubygem-gdk3.x86_64: E: incorrect-fsf-address
> /usr/share/gems/gems/gdk3-2.0.2/COPYING.LIB

- Will ping upstream

> rubygem-gdk3.src: W: strange-permission gdk3-2.0.2.gem 0600L

- Will fix on next push (if there is other things to fix on review

> > ruby -Ilib:test ./test/run-test.rb || echo "Please investigate this"
> Why that ||?
> If the test fail, it will not take down whole build. Which is what we
> generally want to, isn't it?

- Well, this is wrapper for gdk, and actually (if test fails)
  debugging it sometimes takes really much times. Sometimes
  even upstream can't fix this in a short time. So unless there
  is apparently critical issue to fix, I want to just notice upstream
  about test failure (if any).

  And for gtk, debugging is much harder...

> I'm really not sure about:
>   %global header_dir  %{ruby_vendorarchdir}
> this is not in guidelines, so it is just IMHO, but... I think (after
> consultation with vondrouch), that header files should go to:
> ["vendorarchhdrdir", "/usr/include/vendor_ruby/x86_64-linux"]
> ------------^^^ notice this
> or 
> $  pkg-config --variable=vendorhdrdir ruby
> /usr/include/vendor_ruby
> which is unfortunately not provided by any macro (as rubygem-foo-devel is
> not usual).
> Can you consider moving header files there?

- Well, 
  * (while currently I am using F-19) actually /usr/include/vendor_ruby
    is not owned by at least ruby-devel and others ... while
    /usr/lib/ruby/vendor_ruby is owned by ruby-libs.

  * And neither of vendorarchhdrdir or /usr/include/vendor_ruby actually
    does not work (in the meaning that they are not included in include
    path) (see
    http://koji.fedoraproject.org/koji/taskinfo?taskID=5850464
    )
  (Well, 

> And if you provide -devel package, should you provide even so-name?
- This is not needed. As in other packages, dlopen'ed modules (i.e.
  not used to be linked against) need not have sonames (and they are
  under subdirectories of /usr/lib, so they don't appear in
  ld library path)

>- Well, this is wrapper for gdk, and actually (if test fails)
>  debugging it sometimes takes really much times. Sometimes
>  even upstream can't fix this in a short time. So unless there
>  is apparently critical issue to fix, I want to just notice upstream
>  about test failure (if any).

Sounds reasonable. But only if the test is currently failing. I would like to the there:
  ruby -Ilib:test ./test/run-test.rb
and if test will start fail, you can then *temporary* add 
 ... || echo "Know problem BZ XXX"
This way users (e.g provepackagers) will know why the failing test is waived and can checkout the progress. You may want to add some comment for provenpackagers, that if test is failing, you can temporary disable it this way and report the problem at XXX.
Do you agree?


> * And neither of vendorarchhdrdir or /usr/include/vendor_ruby actually
>    does not work (in the meaning that they are not included in include
>    path) (see

Which is IMO bug in ruby. Filed bug 1001528.

And from different POV: why you are copying it to ruby_vendorarchdir? Why it could not stay in %{gem_instdir}/lib/ ? You will probably then have to manualy specify include in rubygem-gtk, but thats IMO not problem and big deal. Opinions?

(In reply to Miroslav Suchý from comment #7)

> > * And neither of vendorarchhdrdir or /usr/include/vendor_ruby actually
> >    does not work (in the meaning that they are not included in include
> >    path) (see
> 
> Which is IMO bug in ruby. Filed bug 1001528.

- Note that arch-dependent headers must not be in /usr/include, in
  that case /usr/lib/foo is also needed.

> And from different POV: why you are copying it to ruby_vendorarchdir? Why it
> could not stay in %{gem_instdir}/lib/ ? You will probably then have to
> manualy specify include in rubygem-gtk, but thats IMO not problem and big
> deal. Opinions?

Well,
- If (while currently not in this case) when header files become
  arch-dependent, they cannot be in %{gem_instdir}/lib/ anyway.

- And ruby-gnome2 related header files come not only from
  rubygem-gdk3 (you can see that rubygem-gdk3 already contains
  not a few BuildRequires about rubygem-foo-devel)

  (note that other ruby-gnome2 related packages also needs other
   ruby-gnome2 header files, e.g. rubygem-gdk_pixbuf needs 
   rubygem-glib-devel, currently there are 13 packages related to
   ruby-gnome2 )

  If we stick to use %gem_instdir, correspoing -Ifoo has to be
  added each ruby-gnome2 related gems, which is annoying.

Current header files are not arch-dependent. So lets stick to it. We (or you) will address it when (and if) they become arch dependent. Which is BTW discouraged.

(In reply to Miroslav Suchý from comment #7)
> >- Well, this is wrapper for gdk, and actually (if test fails)
> >  debugging it sometimes takes really much times. Sometimes
> >  even upstream can't fix this in a short time. So unless there
> >  is apparently critical issue to fix, I want to just notice upstream
> >  about test failure (if any).
> 
> Sounds reasonable. But only if the test is currently failing. I would like
> to the there:
>   ruby -Ilib:test ./test/run-test.rb
> and if test will start fail, you can then *temporary* add 
>  ... || echo "Know problem BZ XXX"
> This way users (e.g provepackagers) will know why the failing test is waived
> and can checkout the progress. You may want to add some comment for
> provenpackagers, that if test is failing, you can temporary disable it this
> way and report the problem at XXX.
> Do you agree?

I prefer to grep for specific results, e.g. | grep "216 tests, 325 assertions, 0 failures, 16 errors, 0 skips". That ensures, that the test results are constant and we know about changes. The || echo or || : is too vague for my taste.

Not mentioning that in this specific case, there is not executed even single test, since it blows apart right from the start :/

(In reply to Miroslav Suchý from comment #9)
> Current header files are not arch-dependent. So lets stick to it. We (or
> you) will address it when (and if) they become arch dependent. Which is BTW
> discouraged.

Then leave this as it is. Touching every 13 (and more) packages (again
not only for rubygem-gdk3) to add -I/usr/share/.... is very annoying.

Also, putting header file under "/usr/share" is _much_ discouraged than
to put it inder /usr/lib (while putting header file under /usr/lib
can be seen by other packages, putting header file under /usr/share
is - as far as I know - no package puts them.

(In reply to Vít Ondruch from comment #10)
> (In reply to Miroslav Suchý from comment #7)
> > >- Well, this is wrapper for gdk, and actually (if test fails)
> > >  debugging it sometimes takes really much times. Sometimes
> > >  even upstream can't fix this in a short time. So unless there
> > >  is apparently critical issue to fix, I want to just notice upstream
> > >  about test failure (if any).
> > 
> > Sounds reasonable. But only if the test is currently failing. I would like
> > to the there:
> >   ruby -Ilib:test ./test/run-test.rb
> > and if test will start fail, you can then *temporary* add 
> >  ... || echo "Know problem BZ XXX"
> > This way users (e.g provepackagers) will know why the failing test is waived
> > and can checkout the progress. You may want to add some comment for
> > provenpackagers, that if test is failing, you can temporary disable it this
> > way and report the problem at XXX.
> > Do you agree?
> 
> I prefer to grep for specific results, e.g. | grep "216 tests, 325
> assertions, 0 failures, 16 errors, 0 skips". That ensures, that the test
> results are constant and we know about changes. The || echo or || : is too
> vague for my taste.

Well, actually I was going to propose to _not_ using grep.
Using grep, actually what tests failed cannot be seen on build.log
(only the result of grep appears), which makes build.log less useful
then other people tries to look at it.

(BTW, it is preferable to check build.log even if build is successful)

> 
> Not mentioning that in this specific case, there is not executed even single
> test, since it blows apart right from the start :/

(In reply to Mamoru TASAKA from comment #8)
> Well,
> - If (while currently not in this case) when header files become
>   arch-dependent, they cannot be in %{gem_instdir}/lib/ anyway.

To repeat, at least header file must be under /usr/include, not
/usr/share anyway.

BTW this looks to be older version of gdk3. It does not appear that gdk3 2.x has any headers in %{gem_libdir}. It keeps the headers in %{gem_instdir}/ext directories.

(In reply to Mamoru TASAKA from comment #11)
> (In reply to Miroslav Suchý from comment #9)
> > Current header files are not arch-dependent. So lets stick to it. We (or
> > you) will address it when (and if) they become arch dependent. Which is BTW
> > discouraged.
> 
> Then leave this as it is. Touching every 13 (and more) packages (again
> not only for rubygem-gdk3) to add -I/usr/share/.... is very annoying.
> 
> Also, putting header file under "/usr/share" is _much_ discouraged than
> to put it inder /usr/lib (while putting header file under /usr/lib
> can be seen by other packages, putting header file under /usr/share
> is - as far as I know - no package puts them.

Unless we create something like /usr/include/gems/headers/gdk3-1.2.6 I don't see any better option :/

> Well, actually I was going to propose to _not_ using grep.
> Using grep, actually what tests failed cannot be seen on build.log
> (only the result of grep appears), which makes build.log less useful
> then other people tries to look at it.

You are right, that is PITA. We should use it in combination with 'tee' or something ...

So let's keep header files under %{ruby_vendorarchdir}, which is actually working.

(In reply to Vít Ondruch from comment #13)
> BTW this looks to be older version of gdk3. It does not appear that gdk3 2.x
> has any headers in %{gem_libdir}. It keeps the headers in %{gem_instdir}/ext
> directories.

gdk3 2.0.2 actually installs header files under ./%{gem_libdir}
(see build log:
http://koji.fedoraproject.org/koji/taskinfo?taskID=5850464
)

Like other packages, only header files needed to be installed
system-wide are to be copied into ./%{gem_libdir}
(but again, unless adding many -Ifoo -Ibar, we have to copy them
 to %{ruby_vendorarchdir}
)

(In reply to Mamoru TASAKA from comment #15)
Ah, that is actually a good catch! I was installing the gem as a root:

# gem install gdk3

and guess what, the headers are installed into:

$ ls -la /usr/local/lib/gems/ruby/gdk3-2.0.2/lib/
total 708
drwxr-xr-x. 1 root root     68 Aug 27 13:17 .
drwxr-xr-x. 1 root root      6 Aug 27 13:16 ..
-rwxr-xr-x. 1 root root 710669 Aug 27 13:17 gdk3.so
-rw-r--r--. 1 root root   2347 Aug 27 13:17 rbgdk3.h
-rw-r--r--. 1 root root   8160 Aug 27 13:17 rbgdk3conversions.h

That is why I missed them ;) So they actually should go into %{gem_extdir} side by side with the .so file. Would that be acceptable for you?

(In reply to Vít Ondruch from comment #16)
> 
> So they actually should go into %{gem_extdir}
> side by side with the .so file. Would that be acceptable for you?

The problem is, these are header files, not used by "gem" mechanism
but used by gcc or so (these header files are really needed for compilation).

So unless we have some automated mechanism to
  - resolve dependency for gems
  - add -Ifoo flags for each dependency gem with correct order
with_out_ using gem (and without using complicated patches or so)
keeping header files into %gem_extdir won't bear any benefit

(We already have received complaints about header path inclusion
 for ruby related packages, e.g. bug 975660 - first of all
 header files must be put into just _working_ directory)

(In reply to Mamoru TASAKA from comment #17)
> The problem is, these are header files, not used by "gem" mechanism
> but used by gcc or so (these header files are really needed for compilation).

Sorry, I can't understand the issue. I am able to install using regular "gem install" the gtk3 gem, which uses the headers in question. There is apparently some infrastructure in ruby-gnome2 project, which can find the headers on their original place (it would be %{gem_extdir}/lib} in our case) without specifying any additional -I. So I really don't know what other use case you have on your mind :/ Could you please enlighten me?

(In reply to Vít Ondruch from comment #18)
> (In reply to Mamoru TASAKA from comment #17)
> > The problem is, these are header files, not used by "gem" mechanism
> > but used by gcc or so (these header files are really needed for compilation).
> 
> Sorry, I can't understand the issue. I am able to install using regular "gem
> install" the gtk3 gem, which uses the headers in question.

That is because you installed gem out of Fedora's rpm path but 
into your local path.

ruby-gnome2 tries to find header files under
Gem::Specification.find_by_name(module).full_gem_path, which is
usually "/usr/share"/gems/gems/module-version, which does not work.
(Please don't ask me about when installing gem as root without
 using rpm, I don't want to try it)

(In reply to Mamoru TASAKA from comment #19)

> ruby-gnome2 tries to find header files under
> Gem::Specification.find_by_name(module).full_gem_path, which is
> usually "/usr/share"/gems/gems/module-version, which does not work.

When installing locally as non-root without using rpm, this would
be ~/.gem/..........

Would you point out really _blocking_ issue now?

First three items from #5. I.e. that rpmlint errors and run all test and do not waive them out for all cases. Waive them out only by some tee & grep.

Finding some nice solution for those header would be nice, but this not mandated by guidelines and we can continue on ruby-sig mailing list.

So I did some research:

* The ruby-gnome2 project expects headers in %{gem_instdir}/ext by default [1]. Headers installed into %{gem_libdir} seems to be useless.

* There is no way how to get full require_paths from RubyGems, so there is no reasonable way to teach the rubygem-glib2, how to find the headers. I proposed [2] to RubyGems upstream, but that is a long shot :/

* Although bug 975660 might look as a reasonable solution, gdk3 does not support it nor we don't have it implemented (and I don't think it is worth of implementation ATM)

So at the end, it seems I must agree with %{ruby_vendorarchdir}. Hopefully, there will be chance to reiterate this in the future.


[1] https://github.com/ruby-gnome2/ruby-gnome2/blob/master/glib2/lib/mkmf-gnome2.rb#L114
[2] https://github.com/rubygems/rubygems/pull/632

(In reply to Miroslav Suchý from comment #22)
> First three items from #5. I.e. that rpmlint errors and run all test and do
> not waive them out for all cases. Waive them out only by some tee & grep.

- License text *must not* be patched even if FSF address differs.
  I may ping upstream, however modifying license text must not be
  done downstream

- Please explain why || : is not sufficient for you. Anyway checking
  build.log is always preferable. Also || : is commonly used:
  e.g.
  http://pkgs.fedoraproject.org/cgit/gcc.git/tree/gcc.spec
  http://pkgs.fedoraproject.org/cgit/glibc.git/tree/glibc.spec

- tarball rpmlint issue can be fixed when importing to Fedora git.

- Also "grep" is anyway not a perfect solution. 
  "One error" may occur at the different than expected and manual
  investigation is anyway needed.

> License text *must not* be patched even if FSF address differs.

Agree. Just put here some link to mail archive or issue request of upstream, so I know that you notified upstream.

>- Please explain why || : is not sufficient for you. Anyway checking
>  build.log is always preferable. 

Checking build.log is preferable. But I always seen that build.log is checked only if build fail. If build succeed then it is usually "HURRAY, move on".
And sometimes is even manual check of build.log not possible - e.g. mass rebuilds.

So I really thing you should change it to:

FILE=$(mktemp)
ruby -Ilib:test ./test/run-test.rb | tee $FILE || :
grep "5 tests, 5 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications" $FILE

And in case some test file replace the last line with something like:

# test foo is failing, reported as http://foo/issue/1
cat $FILE | grep "4 tests, 4 assertions, 1 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications"

This way you waive out just that one failure. And if there will be second failure you will be immediately notified by failed build. And yes manual intervention is needed as you must report that failure to upstream (or fix it on your own) and add new comment about reported bug and change the grepped string.

For me this is blocker.
If there are examples of other packages which does that, then IMO they are bad examples and it should be fixed as well.

Again grep is not a good solution as I said in comment 25 . I don't think other packagers will agree with adding grep.

Please consider again if grep is really _must_ item and blocker. Again I don't agree and I don't think making this mandatory is technically correct.

I will bring this to packagers list.

Thread start:
https://lists.fedoraproject.org/pipermail/packaging/2013-September/009486.html

Email from me with patches that probably fix the code itself here:
https://lists.fedoraproject.org/pipermail/packaging/2013-September/009493.html

Let me know if you need a direct copy of the patches since the mailman web archives have the patches inline.

Toshio, thank you for trying for patch, however as I want to verify your patch (since you say you "probably" fix the code) and also anyway I want to ask upstream, for now I will leave your patch pending.

So using grep is not a good idea. Miroslav, would you check again? (Please remind that I approved your package even if you disabled any check in your package).

Just additional note:

I don't think "hiding" check errors by patches is not a good idea and rather I want to show check errors as it is and let upstrem know about it.

(Note that "hiding" here means to disable failing tests by patch - unless it is "apparent" that the test is completely broken, or the test cannot be executed due to missing additional sources or so)

Few comments:

About the definition of hiding:  The patch doesn't hide the test failure.  It attempts to fix the test failure.  Hiding it would be to comment out that test or disabling the test suite.

About verifying with upstream: The rationale for not applying the patches is that you're not able to evaluate whether the patches to the test case are correct and that's worrisome enough that you won't apply a proposed fix for it because it might break the package more.  However, that means that the package you are building could be severely broken.  Severely broken packages aren't supposed to pass review.  So enough review of the issue needs to take place so that we know whether the issue is one that compromises the operation of the package.  But once someone does that it becomes apparent whether the patch is okay to apply or not: if the issue is really serious, the patch to the test case is incorrect and the package should not be approved until the actual issue is fixed.  If the issue is minor, the proposed fix to the test case is something that can be applied and the problem and proposed solution sent upstream.  It can be changed if upstream's evaluation turns out to be different.

There's two places where our expectations might not match up here.  The first would be that we have different implicit rankings of solutions to test failure.  Here's mine from most desirable to least desirable:

1) Fix the bug the right way.
2) Potentially fix the bug the right way (unsure enough to need further evaluation)
3) Fix the bug in a way that you know is too hacky for upstream but fixes the problem on Fedora
4) Disable the specific case that's provoking the issue.
5) Disable the entire test suite.

The second place where our thoughts might mismatch is in the definition of "disable".  For me, a disabled test or test suite is one that does not fail the build if the test or test suite doesn't pass.  I think that your definition is slightly different; I'm guessing you'd say that not running the test at all is what constitutes disabling.  The reasons I'd give for my view are:

* On rawhide if the build succeeds the new build will replace the old package no matter what the test suite printed to the logs.
* When someone else rebuilds the package for some reason (releng rebuild, provenpackagers rebuilding for changes to guidelines Fedora Change Owner rebuilding dependent packages), they won't know that they have to look at the build log of this package to see the test suite results.  Even if they do, they won't know what the old baseline was, only what the current results are.
* If you should give up maintaining the package and someone else take over (maybe just to satisfy the deps in their own package) they won't know that a failing test suite will not fail the build.

And one note - all this is about *potential* problem in future. Because the tests are passing right now. So right now there is really no need to do "|| echo".

And one note - all this is about *potential* problem in future. Because the tests are passing right now. So right now there is really no need to do "|| echo".

Well, looking at the code gtk+-3.8.4, it seems to me that 0.3 should _exactly_ be turned into 77, not 76, as it seems all decimals used here can be exactly represented by double type, so still I will leave the patch posted pending.

(In reply to Toshio Ernie Kuratomi from comment #34)
> Few comments:
> 
> About the definition of hiding:  The patch doesn't hide the test failure. 
> It attempts to fix the test failure.  Hiding it would be to comment out that
> test or disabling the test suite.

The same as I explained by comment #33 - and this is to the comment you said in the packaging thread.

> About verifying with upstream: 
(long comments cut)

As currently I don't think your patch is correct as I said above, I will leave your patch pending, better to ask the upstream for correct solution.
(At least applying patches by myself needs some degree of _my_ confidence that the patch is correct).


> There's two places where our expectations might not match up here.  The
> first would be that we have different implicit rankings of solutions to test
> failure.  Here's mine from most desirable to least desirable:
> 
> 1) Fix the bug the right way.
> 2) Potentially fix the bug the right way (unsure enough to need further
> evaluation)
> 3) Fix the bug in a way that you know is too hacky for upstream but fixes
> the problem on Fedora
> 4) Disable the specific case that's provoking the issue.
> 5) Disable the entire test suite.
> 
> The second place where our thoughts might mismatch is in the definition of
> "disable".  For me, a disabled test or test suite is one that does not fail
> the build if the test or test suite doesn't pass.  I think that your
> definition is slightly different; I'm guessing you'd say that not running
> the test at all is what constitutes disabling.  The reasons I'd give for my
> view are:
> 

Okay, so your view and mine differ, and please comment if the different of this is really blocker or not.

(In reply to Miroslav Suchý from comment #35)
> And one note - all this is about *potential* problem in future. Because the
> tests are passing right now. So right now there is really no need to do "||
> echo".

It seems that on f20 i686 test fails (sometimes??)

I won't say that reviews must not say anything which is not written in review guidelines, however reviewers must be careful not to insist on making submitters apply reviewrs' policy or faith.

(Maybe some rounding effect is happening on ruby inside, however I am not sure)

Ah..... 0.3 is not a value that can be exactly represented, sorry..

(In reply to Mamoru TASAKA from comment #37)
> It seems that on f20 i686 test fails (sometimes??)

On f20 koji it failed with an error that I attribute to a change in API that this package depends on.  That's the first patch in my email.  After fixing that, I didn't retry the build multiple times to see if the .to_str() failure was only occurring sometimes but I can confirm that it happened on x86 f20 but passed on x86_64:

http://koji.fedoraproject.org/koji/taskinfo?taskID=5899933

(In reply to Mamoru TASAKA from comment #39)
> (Maybe some rounding effect is happening on ruby inside, however I am not
> sure)

<nod>  Isn't the rule of thumb: Never depend on C code to precisely represent any floating point numbers? ;-)

(In reply to Mamoru TASAKA from comment #38)
> I won't say that reviews must not say anything which is not written in review
> guidelines, however reviewers must be careful not to insist on making
> submitters apply reviewrs' policy or faith.

In so far as style goes I would agree with this.  (For instance, trailing periods in Summary, removing shebang lines in importable code modules that trigger rpmlint but don't affect the code...)  However, at some point undocumented points do make a technical difference.  The testsuite is one of those places.  If there's disagreement here, the reviewer could choose to try to make it a blocker (the most common route for this is to propose to the FPC that they add a rule covering it to the guidelines).  Or the reviewer could consider that they consider it enough of a problem that they won't approve the package themselves but they understand that there's a difference of opinion and will let someone else pick up the review and finish it if they choose.

I hope that explains that I don't consider that msuchy or anyone else has an obligation to approve the package without a commitment not to disable (sorry, my definition of disable) the testsuite.  But at the same time, it's not currently something that all reviewers might consider a blocker.

From the Preamble of the Packaging Guidelines:

"It is the reviewer's responsibility to point out specific problems with a package and a packager's responsibility to deal with those issues. The reviewer and packager work together to determine the severity of the issues (whether they block a package or can be worked on after the package is in the repository.)"

The reviewer has pointed out a shortcoming of the package.  The two of you disagree on the severity.  One of you could take this to FPC to make a definitive ruling or the two of you could decide to let another reviewer decide if they would like to approve this despite the problem.

My update:
I would accept those headers on that place, since this is probably first package where this happen. But please consider bringing up this discussion to ruby-sig and find some final and better solution.

I would accept that waive of these, but only if:
* you put there in comment why it is currently waived out (does not work on F21 because of...)
* if you put there in comment link to upstream report of that issue.
And once those tests are functional again in future, remove that "|| echo".

http://mtasaka.fedorapeople.org/Review_request/ruby-gnome2-suite/rubygem-gdk3.spec
http://mtasaka.fedorapeople.org/Review_request/ruby-gnome2-suite/rubygem-gdk3-2.0.2-2.fc.src.rpm

* Thu Sep 19 2013 Mamoru TASAKA <mtasaka> - 2.0.2-2
- Patch from upstream to fix TestGdkRGBA

Koji scratch build:
F-21: http://koji.fedoraproject.org/koji/taskinfo?taskID=5954074
F-20: http://koji.fedoraproject.org/koji/taskinfo?taskID=5954078

The upstream patch is safe because 255 * 0.2 + 0.5 = 51.5, for example.

Forbidden

You don't have permission to access /Review_request/ruby-gnome2-suite/rubygem-gdk3-2.0.2-2.fc.src.rpm on this server.

Can you please fix it?

Permission fixed.

(By the way, you can also download it from
http://koji.fedoraproject.org/scratch/mtasaka/task_5954074/
)

One last thing - you are missing:
Requires: ruby(release)

(In reply to Miroslav Suchý from comment #46)
> One last thing - you are missing:
> Requires: ruby(release)

See this line:
# MRI only
Requires:	ruby

(Basically for any gem packages containing C extension modules,
 "(Build)Requires: ruby(release)" is nonsense, because they work
  only with CRuby = MRI.
  Current ruby guideline says that any rubygem-based rpm must indicate
  that some ruby interpreter is needed, and if any ruby interpreter can
  satisfy the dependency, ruby(release) should be used. Otherwise ruby
  guideline says that the package _should_ (not _must_) require the
  specific ruby interpreter.
)

APPROVED

But I strongly suggest you to continue discussing the problematic parts (like the headers files) on ruby-sig mailing list.

Thank you.

New Package SCM Request
=======================
Package Name: rubygem-gdk3
Short Description: Ruby binding of GDK-3.x
Owners: mtasaka
Branches: f20
InitialCC:

Git done (by process-git-requests).

Rebuilt for all branches, push requested for F-20, closing.

Thank you for reviewing and git procedure.