Bug 195363
Summary: | Review Request: esc | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Jack Magne <jmagne> | ||||
Component: | Package Review | Assignee: | David Cantrell <dcantrell> | ||||
Status: | CLOSED RAWHIDE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | medium | ||||||
Version: | rawhide | CC: | 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
Jack Magne
2006-06-14 21:40:47 UTC
We have xulrunner? 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 Sometime between now and the final release. :) Cc'ing caillon, jrb. Updated the following: Location of the packages Spec URL: http://directory.fedora.redhat.com/built/rpm_review/jmagne/esc.spec http://directory.fedora.redhat.com/built/rpm_review/jmagne/esc-xulrunner-devel.spec SRPM URL: http://directory.fedora.redhat.com/built/rpm_review/jmagne/esc-1.0.0-1.src.rpm http://directory.fedora.redhat.com/built/rpm_review/jmagne/esc-xulrunner-devel-1.8.0.1-1.src.rpm Updated: esc.spec esc-1.0.0-1.src.rpm 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. Last word was there won't be a system xulrunner for fc6/rhel5 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 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... Thanks for the comments on the spec file. I have updated the esc source RPM with suggested changes. 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? 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? Created attachment 132450 [details]
build log of failure
It looks like it's assuming the esc-xulrunner-devel build dir is still around.
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. Ray, ping, can you continue this review? 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. 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. 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. 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. 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. %{?dist} in Core packages now? I had removed them from those I moved from Extra's as per the package guidelines for CORE bob Core can use %{?dist}. I updated the guidelines the other day. 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/ looks good to me! 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. 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. 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. comps should be group 'gnome-desktop' default. bob what kind of files? you can't just remove directories on users systems (what if they put something in it?). Excellent point. I will remove that entry. (In reply to comment #27) > comps should be group 'gnome-desktop' default. > > bob Added to comps. Please close when package is built for rawhide. Successfully built on relevant platforms except ppc64. |