Created attachment 133548 [details]
log of rebuilding in mock on i386

Log of rebuilding src.rpm in mock on i386.

Result of rpmlint:
jfbterm-0.4.7-0.15.fc6.i386.rpm:

jfbterm-0.4.7-0.15.fc6.src.rpm:
W: jfbterm strange-permission jfbterm-0.4.7-configure-header.patch 0600
W: jfbterm strange-permission jfbterm-0.4.7-hang-onexit.patch 0600
W: jfbterm strange-permission jfbterm-0.4.7-mmap-newkernel.patch 0600
W: jfbterm strange-permission jfbterm-0.4.7-remove-warning.patch 0600
W: jfbterm strange-permission jfbterm-0.4.7-userspace.patch 0600
W: jfbterm strange-permission jfbterm-0.4.7.tar.gz 0600
W: jfbterm strange-permission efont-unicode-bdf-0.4.2.tar.bz2 0600
W: jfbterm mixed-use-of-spaces-and-tabs

jfbterm-debuginfo-0.4.7-0.15.fc6.i386.rpm:

Note:

I will bump the release number when this src.rpm is approved.

You will need to fix the permissions and mixed use of spaces and tabs

Not sure about all the stuff at the top for determining the distro used - there
seems to be alot in there for pre FC4. As FC4 is now officially in legacy, are
you sure you need to go back that far?

This is release 1, not 0

efont is another package and should be submitted as such. You then use
R:<package_name> (or BR if you need to use it to build).

[ %{buildroot} != '/' ] || exit 1 isn't required

You will also need BR : iconv

What you need to do before a full review occurs is this

1. Decide how far back you want this package to go. If you really want it to go
back prior to FC4, I would recommend either seeking advice on the Fedora Extras
list or seeing what problems you will encounter as the distros get older and
updates don't happen.

2. If you only support FC5 and FC6, you'll need to ditch a lot of the spec file

3. Separate out the fonts and source package and make this package drag in the
fonts. Remember, other packages can make use of the fonts. You will need to
submit that as another package for review and put a blocker on this package as
it relies on the fonts package.

4. Fix the permissions - they won't be let through unless they're correctly set

5. Add BR iconv

6. Add BR whatever provides tic

Paul, thank you for (pre-?) reviewing.

I will try to fix as requested by you quickly as I want to have this
package released soon.

However:
for 4: Of what files do you refer the permissions are incorrect?
for 5: iconv is in glibc-common so BR for iconv is not needed, I think
for 6: BR tor tic (ncurses) is already.
NOTE: this package is built cleanly in mock. 

For 4 : The patches - rpmlint showed them. They need to be 0644

5 and 6 - not a problem. For some reason, iconv always stuck in my head as being
a package by itself!

Okay, now I am trying separate fonts from jfbterm quickly.
When it is done, I will report it and block this bug.

Oh!!
Now font package is split:
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/efont-unicode-bdf.spec
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/efont-unicode-bdf-0.4.2-1.src.rpm

http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/jfbterm.spec
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/jfbterm-0.4.7-1.src.rpm

Currently, I cannot submit new review request: I encounter:
Software error:

DBD::Pg::st execute failed: ERROR:  column "everconfirmed" specified more than once
 [for Statement "INSERT INTO bugs
(version,rep_platform,bug_severity,priority,op_sys,assigned_to,bug_status,bug_file_loc,short_desc,target_milestone,resolution,qa_contact,everconfirmed,product_id,component_id,
reporter, creation_ts, everconfirmed, estimated_time, remaining_time)VALUES
('devel','All','normal','normal','Linux','193505','NEW','','Review Request:
efont-unicode-bdf: Unicode font by Electronic Font Open
Laboratory','---','','197703','1','59','16465',166855, NOW(), 1, 0, 0)"] at
Bugzilla/DB.pm line 71
	Bugzilla::DB::SendSQL('INSERT INTO bugs
(version,rep_platform,bug_severity,priority,...') called at Bugzilla/Bug.pm line
1154
	Bugzilla::Bug::Add('Bugzilla::Bug', 'HASH(0x1376c80)') called at
/var/www/html/bugzilla/post_bug.cgi line 109

For help, please send mail to the webmaster (bugzilla-owner), giving
this error message and the time and date of the error. 

Note:
I am continuing to submit new package review request (efont-unicode-bdf)

Both (efont-unicode-bdf and jfbterm) now bear no rpmlint complaint
(for both src.rpm and binary).

Remember to add a blocker on this bug with the bugzilla reference for the new
package (this one will rely on that one).

As soon as it's in, I'll add it to my review list.

Currently, I cannot submit new FE review request:
I mailed this problem to bugzilla-owner and
am waiting for reply.

Once I am able to sumbit new request, I will make the new
bug block this one. 

Okay. I suceeded in submitting new FE review request (efont-unicode-bdf)
Now, this bug is blocked by bug 202032.

Just chewing over the spec - I've not built it yet (my remote machine connection
is down)

The whole font section in %prep - is this actually required for building the
package?

The calls to aclocal et al. Normally, the configure system calls them (unless
the configure scripts have been messed with and need regenerating). The website
certainly says that it's just a matter of running configure/make/make install

On the make line, is there a reason for not using _smp_mflags? If there is (for
example, it won't build), could you please add a comment to that effect?

%{_mandir}/man*/*

No. If it needs man1/package-name, then call it that. There are not normally
that many man files to install and using /man*/* takes an incorrect ownership. A
quick way if there are many files are to glob them together viz

%{_mandir}/man1/foo*
%{_mandir}/man4/bar*

Can you please check them and if just upload a new spec file?

A. >The whole font section in %prep - is this actually required for building the
package?

not in build stage, however, make install requires these fonts.

B. aclocal and so on...
A patch is for Makefile.am. This is for removing sticky bit on /usr/bin/jfbterm
binary. So perhaps aclocal and so on is needed, I think.

C. man entry
Split. new spec file and srpm is now uploaded.
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/jfbterm.spec
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/jfbterm-0.4.7-2.src.rpm

Oh:
D. _smp_mflags

I didn't try it. Now I added this and succeeded in building on
2.6.17-1.2157_FC5smp (_smp_mflags is -j3).

This fix is included in 0.4.7-2.

> A patch is for Makefile.am. This is for removing sticky bit on /usr/bin/jfbterm
> binary. So perhaps aclocal and so on is needed, I think.

It would be better to patch Makefile.in instead if possible (I
haven't looked at that precise example, but from your explanation
it seems so).
And, if possible, you could report upstream that making the setuid
bit should be in a specific make target, or it should be checked
the the user running make is root, since otherwise it prevents
staged installs (like we do for rpms).

(In reply to comment #15)
> It would be better to patch Makefile.in instead if possible (I
> haven't looked at that precise example, but from your explanation
> it seems so).

Okay, another attempt to fix Makefile by patching against Makefile.in
is done on
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/jfbterm-0.4.7-3.src.rpm
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/jfbterm.spec

This removed the necessity of automake, aclocal.
Note: the previous spec file is preserved on
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/jfbterm-0.4.7-2.spec

(In reply to comment #12)

> The whole font section in %prep - is this actually required for building the
> package?
> 

In jfbterm-0.4.7-3, copying fonts section is moved after %{__make}.

Created attachment 133971 [details]
build log of jfbterm in mock 

Build log of jfbterm-0.4.7-3.fc5 in mock on fc5 system.
Note that efont-unicode-bdf is installed from LOCAL.

rpmlint shows no errors.
jfbterm-0.4.7-3.fc5.i386.rpm:

jfbterm-0.4.7-3.fc5.src.rpm:

jfbterm-debuginfo-0.4.7-3.fc5.i386.rpm:

My newest srpm is in comment #16.
Paul, would you check it?

bug 202032 (efont-unicode-bdf):

Change in efont-unicode-bdf (font path change) affects jfbterm packaging.
Please check bug 202032 before this.

#20 : 

This is probably why I'm seeing 

cp: cannot stat `/usr/share/fonts/japanese/misc/b16.pcf.gz': No such file or
directory

Can you fix the spec file and just upload that?

(In reply to comment #21)
> #20 : 
> cp: cannot stat `/usr/share/fonts/japanese/misc/b16.pcf.gz': No such file or
> directory

Yes, this is because I changed the directory in efont-unicode-bdf, is it
okay?
The spec of jfbterm with efont-unicode-bdf > 0.4.2-5 is:
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/jfbterm.spec
(0.4.7-4)
This should fix the error on the comment #20.

Builds fine normally and rpmlint is clean. I am a tad concerned with this
snippet though and have asked for advice on it. There may be both a security and
SELinux implication with it.

8-->
%{__cat} > 60-jfbterm.perms <<EOF
# permission definitions
<console> 0660 /dev/tty0    0660 root
<console> 0600 /dev/console 0600 root
EOF

%{__mkdir_p} -m 755 %{buildroot}%{_sysconfdir}/security/console.perms.d
%{__install} -m 644 60-jfbterm.perms \
   %{buildroot}%{_sysconfdir}/security/console.perms.d/
<--8

As for the spec file...

Good
Follows FE dist tags
No duplicates in the BRs
In american english and with correct permissions
No ownership conflicts
Includes docs
Set out logically
It looks good.

Unsure
Are you installing fonts to %{_datadir}/fonts/%{name}? if you are, you'll need
to do what you did with fonts package this relies on with mkfontdir

Very unsure
Security implications (detailed above)

Builds cleanly in mock

Before fixing spec file:

(In reply to comment #23)
> Builds fine normally and rpmlint is clean. I am a tad concerned with this
> snippet though and have asked for advice on it. There may be both a security and
> SELinux implication with it.
> 
> 8-->
> %{__cat} > 60-jfbterm.perms <<EOF
> # permission definitions
> <console> 0660 /dev/tty0    0660 root
> <console> 0600 /dev/console 0600 root
> EOF
> 
> %{__mkdir_p} -m 755 %{buildroot}%{_sysconfdir}/security/console.perms.d
> %{__install} -m 644 60-jfbterm.perms \
>    %{buildroot}%{_sysconfdir}/security/console.perms.d/
> <--8
> 
> 
> Very unsure
> Security implications (detailed above)

This application (/usr/bin/jfbterm) needs device access right for
/dev/console and /dev/tty0. So usual compilation of jfbterm
sets sticky bit on /usr/bin/jfbterm, with the permission 4755 like
/usr/bin/kon (in kon2-0.3.9b-26.2.1 rpm) With stilly bit, installing
60-jfbterm.perms is not necessary.
Note: kon cannot deal with frame buffer.
Note: pam has /etc/security/console.perms.d/50-default.perms

Original packager (Hideki Machida) and me concluded that it may be
better that we use console.perms method than use sticky bit.
What do you think of this? Umm. I don't know well about SELINUX....

> Are you installing fonts to %{_datadir}/fonts/%{name}? 
jfbterm requires some fonts (in install stage and on the "real use"), 
however, these fonts are actually the copies of fonts in other
packages (in fonts-japanese, xorg-x11-fonts-XXXXX, and fonts-japanese)

Would it be better that I use only the symlink against that fonts?
Doing so requires a bit of trick on install stage.

Okay, I've had the advice back and it seems like there is nothing untoward
security wise.

If the package is making a copy of fonts, I'd prefer a symlink rather than
bloating the system up with needless copies. A more favourable solution though
would be to have the application just use the fonts already there.

(In reply to comment #25)
> Okay, I've had the advice back and it seems like there is nothing untoward
> security wise.
> 

Okay, then I use console.perms method.

> If the package is making a copy of fonts, I'd prefer a symlink rather than
> bloating the system up with needless copies. A more favourable solution though
> would be to have the application just use the fonts already there.
Umm... It seems that I have to install fonts required to /usr/share/fonts/jfbterm .
So, I changed so that requires fonts are installed as symlinks to
that directory.

The fixed spec and srpm are:
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/jfbterm.spec
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/jfbterm-0.4.7-6.src.rpm

rpmlint says than symlink should be relative (not absolute), whic made
spec file a bit complex. This spec file (0.4.7-6) leaves rpmlint messages:

W: jfbterm dangling-relative-symlink /usr/share/fonts/jfbterm/shnmk16.pcf.gz
../japanese/misc/shnmk16.pcf.gz
W: jfbterm dangling-relative-symlink /usr/share/fonts/jfbterm/shnm8x16r.pcf.gz
../japanese/misc/shnm8x16r.pcf.gz
W: jfbterm dangling-relative-symlink
/usr/share/fonts/jfbterm/8x13-ISO8859-14.pcf.gz
../../X11/fonts/misc/8x13-ISO8859-14.pcf.gz
W: jfbterm dangling-relative-symlink /usr/share/fonts/jfbterm/b16.pcf.gz
../japanese/efont-unicode-bdf/b16.pcf.gz
W: jfbterm dangling-relative-symlink
/usr/share/fonts/jfbterm/8x13-ISO8859-9.pcf.gz
../../X11/fonts/misc/8x13-ISO8859-9.pcf.gz
W: jfbterm dangling-relative-symlink
/usr/share/fonts/jfbterm/8x13-ISO8859-7.pcf.gz
../../X11/fonts/misc/8x13-ISO8859-7.pcf.gz
W: jfbterm dangling-relative-symlink /usr/share/fonts/jfbterm/8x16.pcf.gz
../../X11/fonts/misc/8x16.pcf.gz
W: jfbterm dangling-relative-symlink
/usr/share/fonts/jfbterm/8x13-ISO8859-5.pcf.gz
../../X11/fonts/misc/8x13-ISO8859-5.pcf.gz
W: jfbterm dangling-relative-symlink /usr/share/fonts/jfbterm/hanglg16.pcf.gz
../../X11/fonts/misc/hanglg16.pcf.gz
W: jfbterm dangling-relative-symlink /usr/share/fonts/jfbterm/gb16fs.pcf.gz
../../X11/fonts/misc/gb16fs.pcf.gz
W: jfbterm dangling-relative-symlink
/usr/share/fonts/jfbterm/jisksp16-1990.pcf.gz ../japanese/misc/jisksp16-1990.pcf.gz
W: jfbterm dangling-relative-symlink
/usr/share/fonts/jfbterm/8x13-ISO8859-8.pcf.gz
../../X11/fonts/misc/8x13-ISO8859-8.pcf.gz
W: jfbterm dangling-relative-symlink
/usr/share/fonts/jfbterm/8x13-ISO8859-1.pcf.gz
../../X11/fonts/misc/8x13-ISO8859-1.pcf.gz
W: jfbterm dangling-relative-symlink
/usr/share/fonts/jfbterm/8x13-ISO8859-2.pcf.gz
../../X11/fonts/misc/8x13-ISO8859-2.pcf.gz
W: jfbterm dangling-relative-symlink
/usr/share/fonts/jfbterm/8x13-ISO8859-15.pcf.gz
../../X11/fonts/misc/8x13-ISO8859-15.pcf.gz

.... I cannot help but leave this because the "real" fonts are
in other packages. I added some font packages to Require.

Can you not do
        
        ln -s %{buildroot}%{_datadir}/fonts/jfbterm/shnmk16.pcf.gz
        %{_datadir}/fonts/japanese/misc/shnmk16.pcf.gz
        
?
        
That should avoid the dangling problem (that said, the dangling isn't
that big a worry from what I can see)

ln <source> <dest>

You mean 
ln -s %{_datadir}/fonts/japanese/misc/shnmk16.pcf.gz \
   %{buildroot}%{_datadir}/fonts/jfbterm/shnmk16.pcf.gz ?

Anyway, %{_datadir}/fonts/japanese/misc/shnmk16.pcf.gz is not in
jfbterm but in fonts-japanese and this cause
dangling-symlink (not dangling-"relative"-symlink).
Also, rpmlint warn about "symlink-should-be-absolute"
(although I think that absolute symlink can be allowed......)

It's built cleanly in mock and rpmlint said nothing when I ran the rpms past it,
so that's find.

Review time

consistent use of macros
no ownership problems
documentation included
no dupes in the installed rpms
builds cleanly in mock
no devel, so no problems with pkgconfig, .so/la etc
software works when run
no security problems (according to advice)
installed rpm has no missing requires

I'm happy for this to go

APPROVED

(In reply to comment #29)
> I'm happy for this to go
> 
> APPROVED

Thank you!!

* Finally released as 0.4.7-7 to fix the mixed use of %{build_root}
  and $RPM_BUILD_ROOT and the compilation problem on ppc
  (now 0.4.7-7 can be built on i386, x86_64, ppc).
http://buildsys.fedoraproject.org/build-status/job.psp?uid=14387
* SyncNeeded is requiested for FE-5.

Now I close this as CLOSED NEXTRELEASE. Thank you for reviewing
this package.