Bug 1728

I did a quick rpmbuild check and I had to add "libXaw-devel" as a BR for it to build.

Tried again under mock. Needs "BR: groff" as well.

A couple of questions:
1. I assume the number of patches is due to the game no longer being maintained? I noticed there's been no activity on the sf page since 2009.

2. The patch file names do not have %{name} on the front. Although not a requirement I have several source rpms installed and that could make it difficult to know which patches are for what program.

(In reply to comment #2)
> Tried again under mock. Needs "BR: groff" as well.

Thanks, I must admit I did not give it a spin in mock, I'll do a new version with libXaw-devel and groff added to the BR-s. But before I do that I was wondering if you had any other comments?

> A couple of questions:
> 1. I assume the number of patches is due to the game no longer being
> maintained? I noticed there's been no activity on the sf page since 2009.
> 

Yes I plan to file a project take over procedure with sourceforge and do a
new upstream release incorporating them all.

> 2. The patch file names do not have %{name} on the front. Although not a
> requirement I have several source rpms installed and that could make it
> difficult to know which patches are for what program.

Given the amount of patches I'm using a git tree to maintain them, this is how git-format-patch names them :)  Once a package is imported into CVS this becomes a non issue since all the files then live in their own dir.

(In reply to comment #3)
> (In reply to comment #2)
> > Tried again under mock. Needs "BR: groff" as well.
> 
> Thanks, I must admit I did not give it a spin in mock, I'll do a new version
> with libXaw-devel and groff added to the BR-s. But before I do that I was
> wondering if you had any other comments?

I didn't get a chance to actually run it last night after work. I'll try tonight.
I'll go through the review checklist too.

 
> > 2. The patch file names do not have %{name} on the front. Although not a
> > requirement I have several source rpms installed and that could make it
> > difficult to know which patches are for what program.
> 
> Given the amount of patches I'm using a git tree to maintain them, this is how
> git-format-patch names them :)  Once a package is imported into CVS this
> becomes a non issue since all the files then live in their own dir.

Yeah, not a big deal unless you manage multiple git packages and built with rpmbuild...  

Had a second at work, I reviewed as much as I could the Review guidelines.

I think the License in the spec file should be updated:
License: GPLv2 
Found in ./COPYING-2.0

rm $RPM_BUILD_ROOT/usr/lib/X11/app-defaults
If the files that go in this folder are not needed (they are in the resulting package) I think this needs -rf since it puts files in the directory and rm will not remove a directory by default.

No %{buildroot} or %clean. So I assume this package is intended for F13+ (no EPEL)

I know common practice for subpackages is to require the main package 
i.e.: Requires: %{name} = %{version}-%{release}
But the packaging guidelines recommend arch specific as well:
i.e.: Requires: %{name}%{?_isa} = %{version}-%{release}
In this case since it's just an editor I would expect it to work fine but is it still more approprite to include the arch?

Verified it builds in mock for both x86_64 and i686.

Ok, I'm getting the following error trying to run:

$ xmris
X Error of failed request:  BadName (named color or font does not exist)
  Major opcode of failed request:  45 (X_OpenFont)
  Serial number of failed request:  14
  Current serial number in output stream:  15

$ xmred 
X Error: BadName (named color or font does not exist)
  Major opcode: 45
  Minor opcode: 0
  Resource id: 0x4000001
  Serial number: 14
Aborted (core dumped)

This looks to be a font issue but starting the xfs server didn't help. I'm still trying to figure out what the problem is.

Also, shouldn't this be blocking bug #2?

(In reply to comment #5)
> Had a second at work, I reviewed as much as I could the Review guidelines.
> 
> I think the License in the spec file should be updated:
> License: GPLv2 
> Found in ./COPYING-2.0
> 

I can understand your reasoning, but this is not how the GPL version which actual applies gets determined. The rule is first source code headers, then readme, the version of the COPYING file is irrelevant from a legal pov.

Since the README only specifies GPL without mentioning a header any GPL version will do (the person distributing can choose which version he wants to distribute under), so the correct license tag is 

> rm $RPM_BUILD_ROOT/usr/lib/X11/app-defaults
> If the files that go in this folder are not needed (they are in the resulting
> package) I think this needs -rf since it puts files in the directory and rm
> will not remove a directory by default.

This rm command is removing a symlink for compatibility with older libX* versions which imake insists on installing.

> No %{buildroot} or %clean. So I assume this package is intended for F13+ (no
> EPEL)

Correct.

> I know common practice for subpackages is to require the main package 
> i.e.: Requires: %{name} = %{version}-%{release}
> But the packaging guidelines recommend arch specific as well:
> i.e.: Requires: %{name}%{?_isa} = %{version}-%{release}
> In this case since it's just an editor I would expect it to work fine but is it
> still more approprite to include the arch?

isa requires are only relevant for packages with are multilib, iow both the i386 and x86_64 versions of them get included into 64 bit composes. This only happens for libraries (well the diagnostic used by the compose tool is packages with a -devel subpackage, either wat this package won't get multilib-ed, so npo need for isa requires.


(In reply to comment #6)
> Ok, I'm getting the following error trying to run:
> 
> $ xmris
> X Error of failed request:  BadName (named color or font does not exist)
>   Major opcode of failed request:  45 (X_OpenFont)
>   Serial number of failed request:  14
>   Current serial number in output stream:  15
> 
> $ xmred 
> X Error: BadName (named color or font does not exist)
>   Major opcode: 45
>   Minor opcode: 0
>   Resource id: 0x4000001
>   Serial number: 14
> Aborted (core dumped)
> 
> This looks to be a font issue but starting the xfs server didn't help. I'm
> still trying to figure out what the problem is.
> 

Hmm, that should not happen :)  Can you attach your
/etc/X11/fontpath.d/xorg-x11-fonts-Type1/fonts.dir

File here please? As well as Xorg.0.log? 

> Also, shouldn't this be blocking bug #2?

Ah, right, well bug #3 now since review has begun more or less.

(In reply to comment #7)
> (In reply to comment #5)
> > Had a second at work, I reviewed as much as I could the Review guidelines.
> > 
> > I think the License in the spec file should be updated:
> > License: GPLv2 
> > Found in ./COPYING-2.0
> > 
> 
> I can understand your reasoning, but this is not how the GPL version which
> actual applies gets determined. The rule is first source code headers, then
> readme, the version of the COPYING file is irrelevant from a legal pov.
> 
> Since the README only specifies GPL without mentioning a header any GPL version
> will do (the person distributing can choose which version he wants to
> distribute under), so the correct license tag is 

Wow, welcome to the convoluted world of GPL... :)

 
> > rm $RPM_BUILD_ROOT/usr/lib/X11/app-defaults
> > If the files that go in this folder are not needed (they are in the resulting
> > package) I think this needs -rf since it puts files in the directory and rm
> > will not remove a directory by default.
> 
> This rm command is removing a symlink for compatibility with older libX*
> versions which imake insists on installing.

Since it's not obvious (atleast to me) what's going on there it may be helpful to add a comment to better meet the "spec files must be legible" requirement.


> > I know common practice for subpackages is to require the main package 
> > i.e.: Requires: %{name} = %{version}-%{release}
> > But the packaging guidelines recommend arch specific as well:
> > i.e.: Requires: %{name}%{?_isa} = %{version}-%{release}
> > In this case since it's just an editor I would expect it to work fine but is it
> > still more approprite to include the arch?
> 
> isa requires are only relevant for packages with are multilib, iow both the
> i386 and x86_64 versions of them get included into 64 bit composes. This only
> happens for libraries (well the diagnostic used by the compose tool is packages
> with a -devel subpackage, either wat this package won't get multilib-ed, so npo
> need for isa requires.

Hmm... maybe the wiki needs to be updated then to be more clear as it doesn't even mention the non-arch version.


> (In reply to comment #6)
> > Ok, I'm getting the following error trying to run:
> > 
> > $ xmris
> > X Error of failed request:  BadName (named color or font does not exist)
> >   Major opcode of failed request:  45 (X_OpenFont)
> >   Serial number of failed request:  14
> >   Current serial number in output stream:  15
> > 
> > $ xmred 
> > X Error: BadName (named color or font does not exist)
> >   Major opcode: 45
> >   Minor opcode: 0
> >   Resource id: 0x4000001
> >   Serial number: 14
> > Aborted (core dumped)
> > 
> > This looks to be a font issue but starting the xfs server didn't help. I'm
> > still trying to figure out what the problem is.
> > 
> 
> Hmm, that should not happen :)  Can you attach your
> /etc/X11/fontpath.d/xorg-x11-fonts-Type1/fonts.dir

Never mind, it was my fault. I had changed the before mentioned 'rm' to 'rm -rf' which was the culprit.

Richard

Ok, so summarizing, as needs works items I now have:
1) Add BR's for libXaw-devel and groff
2) add a comment documenting the removal of the app-defaults compat symlink

Anything else you would like to see me fix when I spin a new version?

(In reply to comment #9)
> Ok, so summarizing, as needs works items I now have:
> 1) Add BR's for libXaw-devel and groff
> 2) add a comment documenting the removal of the app-defaults compat symlink
> 
> Anything else you would like to see me fix when I spin a new version?

Nope! I think that's it. I went ahead and grep'ed through the mock build log for warning and errors on only found this:

Warning: Swap color 'COLOR_BLACK' has no pixels
Warning: Swap color 'COLOR_BLACK' has no pixels
Warning: Swap color 'COLOR_BLACK' has no pixels
Warning: Noswap color 'COLOR_BACKGROUND' has no pixels
Warning: Noswap color 'COLOR_BACKGROUND' has no pixels
Warning: Swap color 'COLOR_BACKGROUND' has no pixels
Warning: Swap color 'COLOR_BACKGROUND' has no pixels
Warning: Noswap color 'COLOR_BACKGROUND' has no pixels
Warning: Swap color 'COLOR_BACKGROUND' has no pixels
Warning: Noswap color 'COLOR_BACKGROUND' has no pixels
Warning: Noswap color 'COLOR_BACKGROUND' has no pixels
Warning: Noswap color 'COLOR_BACKGROUND' has no pixels
Warning: Noswap color 'COLOR_BACKGROUND' has no pixels
Warning: Noswap color 'COLOR_BACKGROUND' has no pixels
Warning: Noswap color 'COLOR_BACKGROUND' has no pixels
Warning: Noswap color 'COLOR_WHITE' has no pixels
Warning: Swap color 'COLOR_WHITE' has no pixels
Warning: Noswap color 'COLOR_FOREGROUND' has no pixels
Warning: Swap color 'COLOR_FOREGROUND' has no pixels

I'm assuming these can be ignored. 

Richard 

Hans,

I think there's still something wrong. I'm getting the X_OpenFont error again. I was testing it from work over X11 Forwarding to a Cygwin host when I got it to run properly. Now that I'm home I'm getting the error again.

Richard

(In reply to comment #10)
> (In reply to comment #9)

<snip>

> Warning: Noswap color 'COLOR_FOREGROUND' has no pixels
> Warning: Swap color 'COLOR_FOREGROUND' has no pixels
> 
> I'm assuming these can be ignored. 

Yes, that is just noise from the tool which parses the bitmaps into header files which get included into the binary.

(In reply to comment #11)
> I think there's still something wrong. I'm getting the X_OpenFont error again.
> I was testing it from work over X11 Forwarding to a Cygwin host when I got it
> to run properly. Now that I'm home I'm getting the error again.

So quoting myself from comment #7:
"Hmm, that should not happen :)  Can you attach your
/etc/X11/fontpath.d/xorg-x11-fonts-Type1/fonts.dir

File here please? As well as Xorg.0.log?"

And also can you do a "printenv LANG" and copy paste the output here?

Thanks,

Hans

# printenv LANG
en_US.UTF-8

Created attachment 625 [details]
/etc/X11/fontpath.d/xorg-x11-fonts-Type1/fonts.dir

Created attachment 626 [details]
Xorg log

Thanks,

I believe I know what I've done wrong now, I added a requires for
xorg-x11-fonts-Type1, but that provides "courier 10 pitch" rather then "courier", my bad.

Can you install xorg-x11-fonts-100dpi, and then try again, I think I need to change the requires to xorg-x11-fonts-100dpi.

Regards,

Hans

Almost! I had to install xorg-x11-fonts-75dpi before it would work.

(In reply to comment #17)
> Almost! I had to install xorg-x11-fonts-75dpi before it would work.
> 

Hmm, did you log out and back in again after installing the 100dpi fonts, and what about the 75 dpi fonts? It should work fine with the 100 dpi fonts. Can you try removing the 75 dpi fonts again and then after log out + back in give xmris another try?

(In reply to comment #18)
> (In reply to comment #17)
> > Almost! I had to install xorg-x11-fonts-75dpi before it would work.
> > 
> 
> Hmm, did you log out and back in again after installing the 100dpi fonts, and
> what about the 75 dpi fonts? It should work fine with the 100 dpi fonts. Can
> you try removing the 75 dpi fonts again and then after log out + back in give
> xmris another try?

No, but I didn't logout after installing the 75dpi fonts either when it worked.

I uninstalled the 75dpi fonts logged out and back in and it still failed with the same error. Installing the 75dpi fonts without logging out still allowed xmris to run.

Hi,

I'm back, sorry for the long silence (long story). I've looked at how other
packages handle the 75 versus 100dpi font thingie and they simply require both, so I've followed their example. Here is a new version which should fix all issues you've found:

Spec: http://jwrdegoede.danny.cz/xmris.spec
SRPM: http://jwrdegoede.danny.cz/xmris-4.0.5-2.fc14.src.rpm

Regards,

Hans

Ok, everything looks good, the only new rpmlint output is:

/home/build/rpmbuild/SPECS/xmris.spec:88: E: hardcoded-library-path in $RPM_BUILD_ROOT/usr/lib/X11/app-defaults

But it's the 'rm' line so not an issue.

Based on that you package is approved! (Feels weird saying that!)

Package CVS request
======================
Package Name: xmris
Short Description: Maze digging and cherry eating game
Owners: jwrdegoede
Branches: F-14 F-15
InitialCC:
----------------------
License tag: free

Thanks for the CVS admin work, and the pushing too :)

Imported, build and pushed to the repo, closing.