Bug 195363

Summary: Review Request: esc
Product: [Fedora] Fedora Reporter: Jack Magne <jmagne>
Component: Package ReviewAssignee: David Cantrell <dcantrell>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: caillon, fedora-package-review, jrb, rrelyea, rstrode, timp
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2006-07-28 00:48:44 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:
Bug Depends On:    
Bug Blocks: 188268    
Attachments:
Description Flags
build log of failure none

Comment 1 Bill Nottingham 2006-06-14 21:42:52 UTC
We have xulrunner?


Comment 2 Bob Relyea 2006-06-14 23:58:01 UTC
ESC is a xulrunner application.
If we have a xulrunner an official xulrunner rpm for Fedora that would be
preferable to esc-xulrunner-devel.

Bill do you know if there's a schedule for xulrunner?

Thanks,

bob

Comment 3 Bill Nottingham 2006-06-15 00:49:53 UTC
Sometime between now and the final release. :)

Cc'ing caillon, jrb.

Comment 5 Jesse Keating 2006-06-30 19:15:12 UTC
I'd like to see this use the in distro xulrunner, still trying to get a timeline
on when that will be included in Core.  First glance at the specs look good
though.  Setting to needinfo in xulrunner.

Comment 6 Ray Strode [halfline] 2006-07-05 16:04:10 UTC
Last word was there won't be a system xulrunner for fc6/rhel5

Comment 7 Ray Strode [halfline] 2006-07-10 18:09:36 UTC
A couple of comments.

We upstream ESC, yes?  Can we make the tarball .tar.bz2 or .tar.gz instead of
.tgz ?  It's more consistent that way.  Also, why don't we ship the shell script
and desktop file in the upstream tarball?

I've noticed in the %install section you are manually copying over files.  Does
the makefile have a working install target?  The usual procedure is

make install DESTDIR=$RPM_BUILD_ROOT

Comment 8 Christopher Aillon 2006-07-11 18:26:05 UTC
I need to schedule a meeting with some people to figure out what the update
strategy for gecko is going to be moving on into the future before I can
accurately comment as to what I think we should do here.

But now with the latest revisions of the schedule, xulrunner 1.0 is slated to
not be officially released until after FC6 is out, which will cause problems one
way or another...

Comment 9 Jack Magne 2006-07-12 01:42:59 UTC
Thanks for the comments on the spec file.
I have updated the esc source RPM with suggested changes.

Comment 10 Ray Strode [halfline] 2006-07-14 14:23:07 UTC
So the esc-xulrunner-devel package is sort of weird.

First, we don't normally put -devel in the name of srpms.  -devel is usually
reserved for subpackages.

Also, you install all header files, images, config files, stylesheets, binaries,
etc into %{_libdir}.  Normally we put header files in %{_includedir}, images in
%{_datadir}, binaries in %{_bindir}, etc.

On the other hand, esc-xulrunner-devel is only needed for building esc, yea? not
for running esc?  Why are we installing it as a separate package at all then?
Can we just put the xulrunner tarball as an extra Source: line in the spec file
and get rid of esc-xulrunner-devel entirely?

Also, I tried to build the two packages and ran into problems.  esc looks for
something called nsinstall in wrong place. I had to create a symlink for the
build to finish.

After I got it built, it didn't work with our cert server.  It gave me an error
code 44 or something.  This actually brings up another point.

This tool only works with a closed source certificate server that most in the
fedora community don't have access to.  Maybe it would be better if we put this
in extras instead of core?  What do you think?

Comment 11 Jack Magne 2006-07-14 17:12:31 UTC
Thanks for the comments:

Yes, esc-xulrunner-devel is only needed for esc to build against. Also
ESC privately deploys the xulrunner directory which is output by the xulrunner
build. 

It would be no big deal to simply build xulrunner as part of the ESC build.

As for the nsinstall problem I have not seen that here. Have you any log
snippets of that build failure?





Comment 12 Ray Strode [halfline] 2006-07-14 17:26:22 UTC
Created attachment 132450 [details]
build log of failure

It looks like it's assuming the esc-xulrunner-devel build dir is still around.

Comment 13 Jack Magne 2006-07-21 01:27:41 UTC
Thanks to the suggestions I have updated "esc.spec" and "esc-1.0.0-1.src.rpm.

Now xulrunner gets built as part of the ess build process and the build glitches
reported should be taken care of.

Comment 14 Jesse Keating 2006-07-24 23:09:42 UTC
Ray, ping, can you continue this review?

Comment 15 Ray Strode [halfline] 2006-07-25 05:34:35 UTC
So I built this today. I've noticed a few things.

Some packaging issues:

- in general, packages almost always follow %{macro_name} format instead of
%macro_name
- you need to call gtk-update-icon-cache in %post for your icon to be visible in
the menus, etc.
- the vendor thing is a bit odd.  Why do you rename the icon to have the vendor
prefix?  I think I would probably just drop the vendor stuff altogether and pass
--vendor esc to desktop-file-install.
- why do you install the LICENSE into libdir?
- in general, if we put things in the menu in the default install we give them a
generic name (e.g, "Text Editor" instead of "gEdit").  Maybe "Smart Card Manager" ?
- you've got it in the wrong spot I think.  It probably makes more sense to be
in "Administration".
- It needs a root password, yes?  So you'll need to use consolehelper.  install
esc in /usr/sbin, create a symlink from /usr/bin/consolehelper to /usr/bin/esc
and install a file called esc to /etc/pam.d with this in it:

#%PAM-1.0
auth            include         config-util
account         include         config-util
session         include         config-util

- If you want to start the monitoring bits at login, you'll need to install a
desktop file (like the one you put in /usr/share/applications) into
/etc/xdg/autostart .  Note, the program will be run as a normal user, not as
root, so you'll need to separate the management bits from the monitoring bits
for it to work.

- If you do start it at login, make sure you hide the icon until someone inserts
a security token.

cosmetic issues:
1) the icon in the notification area is different than the icon in the app.
2) the gradient is a bit ugly
3) the spacing in the side frame is weird
4) some of the text is wonky, could probably use some proof reading

Some other things:

- When it starts up it asks for a config uri.  I gave the cseng one and it
didn't work.  It just gave me error code 28.  At some point I switched to
connecting to the mountain view vpn (from the westford one) and then it started
working.  I don't know if changing vpn's is what fixed it or if it only works
sometimes.

- If I click the test button on the config dialog it gives me another dialog
telling me it's about to do the test I just asked for.  that dialog isn't really
a good idea.

- It probably would be a good idea to disallow the token that was used for
logging in from being able to be formatted. 

- why is the log file for esc in libdir?

Note, I never actually got enrollment to work.  It formatted my token fine, but
after a few blinks during the enrolment step the token led just turns off and
the client sits with two spinning throbbers indefinitely.  It's just sitting in
poll waiting for events I think.

Comment 16 Ray Strode [halfline] 2006-07-25 05:43:57 UTC
So I closed esc in the middle of the operation and it looks like it succeeded
fine.  Maybe the dialog just needs to change state when it's done.

The plugged-in keys list doesn't know my name or the issuer, but i'm guessing
that's a cert server configuration issue.

Comment 17 Jack Magne 2006-07-25 22:40:21 UTC
Thanks again for the great suggestions. I have completed as many as I could in
the short time.

My comments mixed with yours.

Some packaging issues:
- in general, packages almost always follow %{macro_name} format instead of
%macro_name
- you need to call gtk-update-icon-cache in %post for your icon to be visible in
the menus, etc.
- the vendor thing is a bit odd.  Why do you rename the icon to have the vendor
prefix?  I think I would probably just drop the vendor stuff altogether and pass
--vendor esc to desktop-file-install.

All addressed.


- why do you install the LICENSE into libdir?
Not changed.


- in general, if we put things in the menu in the default install we give them a
generic name (e.g, "Text Editor" instead of "gEdit").  Maybe "Smart Card Manager" ?

Done.


- you've got it in the wrong spot I think.  It probably makes more sense to be
in "Administration".

Done.

- It needs a root password, yes?  So you'll need to use consolehelper.  install
esc in /usr/sbin, create a symlink from /usr/bin/consolehelper to /usr/bin/esc
and install a file called esc to /etc/pam.d with this in it:

The app runs just fine as a regular user.


- If you want to start the monitoring bits at login, you'll need to install a
desktop file (like the one you put in /usr/share/applications) into
/etc/xdg/autostart .  Note, the program will be run as a normal user, not as
root, so you'll need to separate the management bits from the monitoring bits
for it to work.

Done.


- If you do start it at login, make sure you hide the icon until someone inserts
a security token.

This one I will have to figure out.


cosmetic issues:
1) the icon in the notification area is different than the icon in the app.
2) the gradient is a bit ugly
3) the spacing in the side frame is weird
4) some of the text is wonky, could probably use some proof reading

Work on the UI in general is ongoing including the above.


Some other things:

- When it starts up it asks for a config uri.  I gave the cseng one and it
didn't work.  It just gave me error code 28.  At some point I switched to
connecting to the mountain view vpn (from the westford one) and then it started
working.  I don't know if changing vpn's is what fixed it or if it only works
sometimes.

The current latest esc app is designed to be able to call back to the server to
get many quantities such as the TPS URL. Your server does not have this
functionality as of yet.
As a backup it still supports the "esc.tps.url" pref value in
/usr/lib/esc-1.0.0/defaults/preferences/esc-prefs.js. This can be set manually.



- If I click the test button on the config dialog it gives me another dialog
telling me it's about to do the test I just asked for.  that dialog isn't really
a good idea.

Done.


- It probably would be a good idea to disallow the token that was used for
logging in from being able to be formatted. 

Good idea.


- why is the log file for esc in libdir?

Now the file goes under the user's profile. Which on Linux is under:

~/.redhat/esc


Note, I never actually got enrollment to work.  It formatted my token fine, but
after a few blinks during the enrolment step the token led just turns off and
the client sits with two spinning throbbers indefinitely.  It's just sitting in
poll waiting for events I think.

This was a simple Javascript glitch which has been addressed.


Comment 18 Ray Strode [halfline] 2006-07-26 02:51:16 UTC
It looks like the spec and package moved to 

http://directory.fedora.redhat.com/built/rpm_review/jmagne/

yea? Looking good.  A few things:

- The gtk-update-icon-cache stuff still isn't right.  Have a look at
http://fedoraproject.org/wiki/ScriptletSnippets for the right idiom.

- you don't have to manually run tar for xulrunner, you can specify another
%setup line, although I don't remember the right arguments to give it off hand.

- Also, I don't think you should need %{_buildir} anywhere. rpm normally puts
you in the right directories i think.

- I think if you just put %doc esc/LICENSE in the filelist then it should get
moved to the right place (/usr/share/doc/esc-1.0.0/LICENSE) automatically, but I
could be wrong.

Comment 19 Ray Strode [halfline] 2006-07-26 04:54:54 UTC
oh also, I guess we're supposed to use the %{?dist} macro in packages now.

just put that after the release number (with no space).  It gets expanded to
.fc6 on fedora and .el5 on rhel.

Comment 20 Bob Relyea 2006-07-26 16:48:22 UTC
%{?dist} in Core packages now? I had removed them from those I moved from
Extra's as per the package guidelines for CORE

bob

Comment 21 Jesse Keating 2006-07-26 20:17:00 UTC
Core can use %{?dist}.  I updated the guidelines the other day.

Comment 22 Jack Magne 2006-07-27 03:14:46 UTC
Thanks for further suggestions.

Updated version available of esc.spec and esc-1.0.0-1.src.rpm.

http://directory.fedora.redhat.com/built/rpm_review/jmagne/


Comment 23 Ray Strode [halfline] 2006-07-27 03:25:59 UTC
looks good to me!

Comment 24 Ray Strode [halfline] 2006-07-27 03:31:44 UTC
well it's a little odd, that call you pwd in %install 
and rm -rf %{_libdir}/esc-1.0.0 in %postun

Fix those and we should get it built quick.

Comment 25 Jesse Keating 2006-07-27 03:41:35 UTC
I've added esc to dist-fc6.

Where might it go in comps?

If you build it (into dist-fc6-HEAD) soon enough, I'll move it over to get it
into the Test2 / Beta1 releases.

Comment 26 Jack Magne 2006-07-27 17:16:33 UTC
Thanks!

The "pwd" is a mistake.

As for removing the directory,
Xulrunner leaves some stuff behind in the directory that does
not get removed when the RPM is un-installed.

I figured that we would not want to have the directory lying around.

Comment 27 Bob Relyea 2006-07-27 18:43:40 UTC
comps should be group 'gnome-desktop' default.

bob

Comment 28 Ray Strode [halfline] 2006-07-27 19:03:55 UTC
what kind of files?  you can't just remove directories on users systems (what if
they put something in it?).

Comment 29 Jack Magne 2006-07-27 21:23:55 UTC
Excellent point.

I will remove that entry.

Comment 30 Jesse Keating 2006-07-27 21:44:59 UTC
(In reply to comment #27)
> comps should be group 'gnome-desktop' default.
> 
> bob

Added to comps.

Please close when package is built for rawhide.

Comment 31 Jack Magne 2006-07-28 00:48:44 UTC
Successfully built on relevant platforms except ppc64.