Bug 1409355

Summary: Review Request: rubygem-tk - Tk interface module using tcltklib
Product: [Fedora] Fedora Reporter: Mamoru TASAKA <mtasaka>
Component: Package ReviewAssignee: František Dvořák <valtri>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: package-review, valtri, vondruch
Target Milestone: ---Flags: valtri: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2017-03-28 05:27:26 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Mamoru TASAKA 2017-01-01 08:46:02 UTC
Spec URL: https://mtasaka.fedorapeople.org/Review_request/gem-related/rubygem-tk.spec
SRPM URL: https://mtasaka.fedorapeople.org/Review_request/gem-related/rubygem-tk-0.1.1-1.fc.src.rpm
Description: 
Tk interface module using tcltklib.
Fedora Account System Username: mtasaka

Koji scratch build for F-26:
https://koji.fedoraproject.org/koji/taskinfo?taskID=17130847

Comment 1 František Dvořák 2017-01-24 13:41:26 UTC
It looks like, there is a problem with stability. I've tested:

/usr/share/gems/gems/tk-0.1.1/sample/demos-en/widget dialog2

And there are two problems:

1) Occasionally there is a cryptic error message:

  invalid command name ".w00010.msg"

This message occurs also on F25 (with both: ruby-tcltk package, or recompiled rubygem-tk), so this is already existing problem (not a show-stopper for upgrade from ruby-tcltk).

2) When closing the demo window, ruby crashes.

Ruby won't crash in F25, so this is a new problem with F26.

I'm not sure, what to do with the second problem. Continue with packaging and report upstream? It is possible it is only bug in the demo code? (there are used threads, ...).


Now the packaging itself, the easier part: :-)

3) the modification of gem.build_complete

It is interesting way, how to solve the rpmlint error. But the real fix probably belongs to the rpmlint. Maybe it could be better to know about it in the bodhi tests (and keep the error)?

Another way (without the real fix though) could be to use ".rpmlint" config file in the fedora git package sources.

4) gcc dependency is not needed

5) it could be safer to replace '/usr/local' and '/usr/bin/env ruby' only in 'sample' subdirectory (just a hint to consider, checking everything is probably fine and desired?)

6) missing the "Provides: ruby-tcltk"

There is comment about it, so it is probably OK a there is reason for it (not full functionality provided by 'tk' gem?). I've noticed there is still a TCL part available - at least the variable Tk::TCL_VERSION.

Comment 2 Mamoru TASAKA 2017-01-30 04:46:16 UTC
Thank you for comments! I will lool into this later, sorry for delay.

Comment 3 Mamoru TASAKA 2017-02-14 07:06:51 UTC
Hello, again sorry for long delay...

(In reply to František Dvořák from comment #1)
> It looks like, there is a problem with stability. I've tested:
> /usr/share/gems/gems/tk-0.1.1/sample/demos-en/widget dialog2
> And there are two problems:
> 
> 1) Occasionally there is a cryptic error message:
> 
>   invalid command name ".w00010.msg"
> 
> This message occurs also on F25 (with both: ruby-tcltk package, or
> recompiled rubygem-tk), so this is already existing problem (not a
> show-stopper for upgrade from ruby-tcltk).

Currently I don't see this issue (yet?)


> 2) When closing the demo window, ruby crashes.
> 
> Ruby won't crash in F25, so this is a new problem with F26.
> 
> I'm not sure, what to do with the second problem. Continue with packaging
> and report upstream? It is possible it is only bug in the demo code? (there
> are used threads, ...).

Will try on F-26, however currently I think this is ruby issue (perhaps), may be bug 1417590 : GC issue with gcc7, hopefully fixed in ruby-2.4.0-76.fc26


> Now the packaging itself, the easier part: :-)
> 
> 3) the modification of gem.build_complete
> 
> It is interesting way, how to solve the rpmlint error. But the real fix
> probably belongs to the rpmlint. Maybe it could be better to know about it
> in the bodhi tests (and keep the error)?
> 
> Another way (without the real fix though) could be to use ".rpmlint" config
> file in the fedora git package sources.

Well, as you know this is just to suppress rpmlint error (if reviewer does not like rpmlint error). If you are okay, I can just make gem.build.complete unmodified (i.e. just empty).


> 4) gcc dependency is not needed
Unfortunately now this is MUST :(
https://fedoraproject.org/wiki/Packaging:C_and_C%2B%2B#BuildRequires_and_Requires

> 5) it could be safer to replace '/usr/local' and '/usr/bin/env ruby' only in
> 'sample' subdirectory (just a hint to consider, checking everything is
> probably fine and desired?)

Because rpmlint warns these :( (Well, anyway we don't support /usr/local so I think this is okay)

> 6) missing the "Provides: ruby-tcltk"
> 
> There is comment about it, so it is probably OK a there is reason for it
> (not full functionality provided by 'tk' gem?). I've noticed there is still
> a TCL part available - at least the variable Tk::TCL_VERSION.

The reason for this is that while I think (believe) ruby-tcltk functionality is fully provided by rubygem-tk, I am not sure at a moment.

Let me know it if you think it is better to modify. Again sorry for long delay...

Comment 4 František Dvořák 2017-02-14 14:09:45 UTC
(In reply to Mamoru TASAKA from comment #3)
> > 
> > 1) Occasionally there is a cryptic error message:
> > 
> >   invalid command name ".w00010.msg"
> > 
> > This message occurs also on F25 (with both: ruby-tcltk package, or
> > recompiled rubygem-tk), so this is already existing problem (not a
> > show-stopper for upgrade from ruby-tcltk).
> 
> Currently I don't see this issue (yet?)
> 

I have Fedora 25, and it happens only occasionally, with dialog1.rb and dialog2.rb. (tested both demo-en and demo-jp, with or without locales)

> 
> > 2) When closing the demo window, ruby crashes.
> > 
> > Ruby won't crash in F25, so this is a new problem with F26.
> > 
> > I'm not sure, what to do with the second problem. Continue with packaging
> > and report upstream? It is possible it is only bug in the demo code? (there
> > are used threads, ...).
> 
> Will try on F-26, however currently I think this is ruby issue (perhaps),
> may be bug 1417590 : GC issue with gcc7, hopefully fixed in
> ruby-2.4.0-76.fc26
> 

It's still happening with ruby-2.4.0-76.fc26. :-( It looks like some infinite loop after raise... Produced backtrace:

http://scientific.zcu.cz/fedora/REVIEWS/rubygem-tk/backtrace.txt

You can look at it as ruby specialist. I'm OK with this, if you say so (considering it as known issue, ...). I can also test other ruby/rubygem-tk packages in rawhide.

> 
> > Now the packaging itself, the easier part: :-)
> > 
> > 3) the modification of gem.build_complete
> > 

> 
> Well, as you know this is just to suppress rpmlint error (if reviewer does
> not like rpmlint error). If you are okay, I can just make gem.build.complete
> unmodified (i.e. just empty).
> 

Yes, that's OK.

> 
> > 4) gcc dependency is not needed
> Unfortunately now this is MUST :(
> https://fedoraproject.org/wiki/Packaging:
> C_and_C%2B%2B#BuildRequires_and_Requires
> 

Oh, I see. OK. (and I should check my packages :-))

> > 5) it could be safer to replace '/usr/local' and '/usr/bin/env ruby' only in
> > 'sample' subdirectory (just a hint to consider, checking everything is
> > probably fine and desired?)
> 
> Because rpmlint warns these :( (Well, anyway we don't support /usr/local so
> I think this is okay)
> 

OK.

> > 6) missing the "Provides: ruby-tcltk"
> > 
> > There is comment about it, so it is probably OK a there is reason for it
> > (not full functionality provided by 'tk' gem?). I've noticed there is still
> > a TCL part available - at least the variable Tk::TCL_VERSION.
> 
> The reason for this is that while I think (believe) ruby-tcltk functionality
> is fully provided by rubygem-tk, I am not sure at a moment.
> 
> Let me know it if you think it is better to modify. Again sorry for long
> delay...

Both packages looks identical (although I only compared list of ruby files and symbols in native libraries and tested some demos).

  Frantisek

Comment 5 Mamoru TASAKA 2017-02-22 09:43:39 UTC
Again sorry for delay... I will update this (perhaps tomorrow)

Comment 6 Vít Ondruch 2017-02-23 08:57:20 UTC
(In reply to František Dvořák from comment #4)
> > > 6) missing the "Provides: ruby-tcltk"
> > > 
> > > There is comment about it, so it is probably OK a there is reason for it
> > > (not full functionality provided by 'tk' gem?). I've noticed there is still
> > > a TCL part available - at least the variable Tk::TCL_VERSION.
> > 
> > The reason for this is that while I think (believe) ruby-tcltk functionality
> > is fully provided by rubygem-tk, I am not sure at a moment.
> > 
> > Let me know it if you think it is better to modify. Again sorry for long
> > delay...
> 
> Both packages looks identical (although I only compared list of ruby files
> and symbols in native libraries and tested some demos).


I don't think there should be "Provides: ruby-tcltk", since the library is on different place now (unless there is ruby- subpackage, as we used to provide, but I'm neither in favor of this idea).

Comment 7 František Dvořák 2017-03-06 15:09:03 UTC
So everything is covered now, only remains the stability issues?

It looks like the crash occurs also with this command:

  ruby -rtk -e 'TkRoot.new; Tk.mainloop'

Crashes after closing window on F26, works OK on F25...

Comment 8 Mamoru TASAKA 2017-03-17 06:30:44 UTC
Sorry for loooong delay... But during this delay, a happy news!

(In reply to František Dvořák from comment #7)
> So everything is covered now, only remains the stability issues?
> 
> It looks like the crash occurs also with this command:
> 
>   ruby -rtk -e 'TkRoot.new; Tk.mainloop'
> 
> Crashes after closing window on F26, works OK on F25...

Now I could reproducible this issue, and this was fixed in 0.1.2. 
https://bugs.ruby-lang.org/issues/13297
https://github.com/ruby/tk/pull/7

https://mtasaka.fedorapeople.org/Review_request/gem-related/rubygem-tk.spec
https://mtasaka.fedorapeople.org/Review_request/gem-related/rubygem-tk-0.1.2-1.fc27.src.rpm

* Wed Mar 15 2017 Mamoru TASAKA <mtasaka.ne.jp> - 0.1.2-1
- 0.1.2

F-27:
https://koji.fedoraproject.org/koji/taskinfo?taskID=18391965
F-26:
https://koji.fedoraproject.org/koji/taskinfo?taskID=18391600
F-25:
https://koji.fedoraproject.org/koji/taskinfo?taskID=18391611

Again sorry for long delay.

Comment 9 František Dvořák 2017-03-20 15:36:14 UTC
Great news about the crashing problem!

Some hints about the latest spec:

1) gem.build_complete - workaround in all ruby arch packages doesn't seem right (proper place to fix would be rpmlint), but it's OK, if you really want to suppress the error :-)

2) '/usr/bin' in the sed could be replaced for %{_bindir} (just a hint when looking though the fedora-review list)

3) functionality: the occasional 'invalid command name ".w00010.msg"' message from dialog1 and dialog2 is still there; that's probably not serious problem, and it was there also in F25

They're just details to consider. Package looks good, APPROVED!

Comment 10 Mamoru TASAKA 2017-03-22 09:42:26 UTC
Okay, thank you for approving!
For ".w00010.msg" message, I will report upstream later.

Comment 11 Gwyn Ciesla 2017-03-22 12:57:40 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/rubygem-tk

Comment 12 Mamoru TASAKA 2017-03-28 05:27:26 UTC
Imported. Thank you for review!