Bug 215256 - (firefox-32) Review Request: firefox-32 - Alternate Launcher for 32bit Firefox
Review Request: firefox-32 - Alternate Launcher for 32bit Firefox
Status: CLOSED WONTFIX
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Christopher Stone
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-11-12 18:38 EST by Warren Togami
Modified: 2008-08-11 20:19 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-11-26 19:37:12 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Warren Togami 2006-11-12 18:38:03 EST
http://togami.com/~warren/fedora/firefox-32-0.0.1-1.src.rpm
http://togami.com/~warren/fedora/firefox-32.spec

Description: 
Alternate Launcher for 32bit Firefox on Multilib Systems
If you have both 32bit /usr/lib and 64bit /usr/lib64 Firefox installed, the standard /usr/bin/firefox launcher will run only the 64bit version.  This launcher allows you to choose to run the 32bit browser by running /usr/bin/firefox-32.  Please be sure that all Firefox instances are closed before running this launcher.

Warren Togami thinks the necessity of this package is sad, given that a tiny change to the standard /usr/bin/firefox script could obviate the need for this package to exist.

POSSIBLE IMPROVEMENTS:
1) It could be made prettier with a custom icon.
2) Better name?  I dunno.
Comment 1 Peter Gordon 2006-11-12 19:05:07 EST
(In reply to comment #0)
> Warren Togami thinks the necessity of this package is sad, given that a tiny
change to the standard /usr/bin/firefox script could obviate the need for this
package to exist.

Perhaps I'm misunderstanding the situtation; but if that's the case, why not
file a bug against Firefox so that this is added?
Comment 2 Warren Togami 2006-11-13 00:35:21 EST
Bug #214100
Comment 3 Matthew Miller 2006-11-13 05:21:33 EST
For what ut's worth, we've chosen to locally hack the launch script to prefer
the i386 version if available. Despite the resolution of bug #214100, that seems
like the logical way for that script to work anyway. But this seems like it may
be a better approach.
Comment 4 Matthias Saou 2006-11-13 08:38:55 EST
Why include Sources as Patches?
If there is no way to require the 32bit package from this "wrapper", then the
package seems pretty broken, no? Would "Requires: /usr/lib/mozilla" work?

Oh, and Matthias Saou thinks it's pretty weird to address one's self using the
3rd person :-)
Comment 5 Warren Togami 2006-11-13 08:58:33 EST
> Why include Sources as Patches?

So it can be tracked in VCS instead of the binary cache.

> If there is no way to require the 32bit package from this "wrapper", then the
> package seems pretty broken, no? Would "Requires: /usr/lib/mozilla" work?

You might be correct, however Bug #214100 is the real reasonable thing to do. 
Given that has been rejected this package is our only solution until
nspluginwrapper is made perfect.

> Oh, and Matthias Saou thinks it's pretty weird to address one's self using the
> 3rd person :-)

That part is actually in the package %description.
Comment 6 Matthias Saou 2006-11-13 09:05:04 EST
(In reply to comment #5)
> > Why include Sources as Patches?
> 
> So it can be tracked in VCS instead of the binary cache.

I'm not aware of any restriction which would disallow commiting "SourceX:" files
into CVS. I actually do this a lot with scripts, desktop entries and such. But
putting source files as "PatchX:" seems wrong. Those aren't patches.

> > If there is no way to require the 32bit package from this "wrapper", then the
> > package seems pretty broken, no? Would "Requires: /usr/lib/mozilla" work?
> 
> You might be correct, however Bug #214100 is the real reasonable thing to do. 
> Given that has been rejected this package is our only solution until
> nspluginwrapper is made perfect.

But without a proper requirement to make sure the 32bit version of firefox will
be available, this package will be broken.

> > Oh, and Matthias Saou thinks it's pretty weird to address one's self using the
> > 3rd person :-)
> 
> That part is actually in the package %description.

Then I'd suggest you remove it. Personal opinions should be expressed on lists,
in bugzilla entries, in CVS commit messages... but not in "end user readable
areas" like descriptions, included READMEs and such. Just my personal advice,
though.
Comment 7 Matthew Miller 2006-11-13 09:12:04 EST
Rather than requiring the 32-bit version, can it be made to detect if the 32-bit
version is there and "hide" otherwise?
Comment 8 Matthias Saou 2006-11-13 09:18:02 EST
(In reply to comment #7)
> Rather than requiring the 32-bit version, can it be made to detect if the 32-bit
> version is there and "hide" otherwise?

Interesting idea, but I think it'll just confuse users. "Hey, I installed it but
I can't find it in the menus!?"
Comment 9 Warren Togami 2006-11-13 11:23:54 EST
> I'm not aware of any restriction which would disallow commiting "SourceX:"
> files into CVS. I actually do this a lot with scripts, desktop entries and 
> such. But putting source files as "PatchX:" seems wrong. Those aren't patches

Hmm, you might be right.  cvs-import.sh is what put SOURCE files into the binary
holding place.  If I import it manually it should be fine.  OK, I'll change it
to SOURCE.

> But without a proper requirement to make sure the 32bit version of firefox
> will be available, this package will be broken.

Not necessarily.  x86_64 default install pulls in both firefox.i386 and
firefox.x86_64.  This package is installable only on x86_64.

Sure, it is not perfect, but do we have a better option?

> Rather than requiring the 32-bit version, can it be made to detect if the 
> 32-bit version is there and "hide" otherwise?

Sounds good in theory, but there is actually NO WAY to do so.  Same problem that
there is no way of requiring firefox.i386 specifically because of the versioned
directories that change arbitrarily.
Comment 10 Warren Togami 2006-11-22 13:40:31 EST
http://togami.com/~warren/fedora/firefox-32-0.0.1-2.src.rpm
http://togami.com/~warren/fedora/firefox-32.spec

Changed the patch files into source files.  I will just be careful when doing
the import to check the source files in instead of putting them into the binary
cache.

I believe that was the only thing possible to fix in this package.  So please
either suggest further fixes or approve.  Thanks.
Comment 11 Christopher Stone 2006-11-23 13:50:46 EST
==== REVIEW CHECKLIST ====
X rpmlint output:
E: firefox-32 description-line-too-long If you have both 32bit /usr/lib and
64bit /usr/lib64 Firefox installed, the standard
E: firefox-32 description-line-too-long /usr/bin/firefox launcher will run only
the 64bit version.  This launcher allows you
E: firefox-32 description-line-too-long to choose to run the 32bit browser by
running /usr/bin/firefox-32.  Please be sure
W: firefox-32 strange-permission setup-firefox-32.sh 0755
E: firefox-32 hardcoded-library-path in /usr/lib
E: firefox-32 description-line-too-long If you have both 32bit /usr/lib and
64bit /usr/lib64 Firefox installed, the standard
E: firefox-32 description-line-too-long /usr/bin/firefox launcher will run only
the 64bit version.  This launcher allows you
E: firefox-32 description-line-too-long to choose to run the 32bit browser by
running /usr/bin/firefox-32.  Please be sure
E: firefox-32 only-non-binary-in-usr-lib
W: firefox-32 no-documentation
W: firefox-32 one-line-command-in-%trigger /usr/lib64/firefox-32/setup-firefox-32.sh
/tmp/firefox-32-0.0.1-2.fc6.x86_64.rpm.32469/usr/share/applications/firefox-32.desktop:
warning: boolean key "Terminal" has value "0", boolean values should be "false"
or "true", although "0" and "1" are allowed in this field for backwards
compatibility
/tmp/firefox-32-0.0.1-2.fc6.x86_64.rpm.32469/usr/share/applications/firefox-32.desktop:
error: invalid characters in value of key "StartupNotify", boolean values must
be "false" or "true" (found "True")
E: firefox-32 invalid-desktopfile
/tmp/firefox-32-0.0.1-2.fc6.x86_64.rpm.32469/usr/share/applications/firefox-32.desktop

I suggest you remove /usr/lib paths from the description and make sure the lines
are < 80 chars.

Definately remove the warren togami rant in the description, it does not belong
there, let's keep this professional.

rpmlint is saying setup-firefox-32.sh should be in /usr/share not /usr/lib64
move this to /usr/share or else add a comment in spec file indicating why it
should be in /usr/lib64

Fix desktop files so rpmlint likes them

Single line trigger files seem okay to me, not sure why rpmlint warns about them

- package named according to package naming guidelines
- spec filename matches %{name}
- package meets packaging guidelines
- package licensed with open source compatible license
O spec file matches actual license.  I'm assuming since you are both the
upstream author and packager this is the case.
- license not packaged with source or included in %doc
- written in American english
- spec file legible
O There is no upstream so I cannot verify source match, but since packager *is*
upstream this is okay
- package successfully compiles and builds on FC6 x86_64
X all build dependencies listed in BR (missing desktop-file-utils for Requires)
- no locales
- no shared libraries
- package is not relocatable
X package does not own all directories it creates
- no duplicates in %files
- file permissions set properly
- package contains proper %clean section
- macro usage consistent
- contains code
- no large documentation
- no header files or static libraries
- no pkgconfig files
- package does not require a devel subpackage
- does not contain .la files
X .desktop file is not installed using desktop-file-install
- package does not own files or directories owned by other packages

==== MUST ====
- shorten description to 80 chars in length
- remove 2nd paragraph in description, instead place a comment in the spec file
pointing to bug #214100
- investigate rpmlint strange permissions warning, consider using 775 instead of
rpmlint likes that better
- move shell script to /usr/share as rpmlint suggests, or if it must be in
/usr/lib64 then add a comment in spec file indicating why
- make rpmlint happy with .desktop file
- install desktop files with desktop-file-install in %install
- packages with .desktop files should Requires: desktop-file-utils
- package must own the /usr/lib64/firefox-32/ directory if this is where the .sh
files ultimately goes (see rpmlint warning indicating this file should go in
/usr/share)

==== SHOULD ====
- remove paths /usr/lib etc. from description, it confuses rpmlint and they are
not needed for the description
- place comment above Source0 URL indicating that this is a shell script written
by packager and there is no web location to find the script
- Include copy of GPL license in %doc
Comment 12 Christopher Stone 2006-11-23 14:56:48 EST
- Why not use the firefox icon if firefox is a requirement?  I think firefox.png
would be better than redhat-web-browser.png
- Why not use mozilla-firefox-32.desktop as the filename to be consistent with
the firefox package?
- Consider also using the mozilla-firefox.desktop file's Name, Generic Name and
Comment fields to use as a template for the firefox32 .desktop file.  I think
the 32 bit version should have the same icon/description as the 64 bit version
in order to make it consistent and easier to find.
Comment 13 Warren Togami 2006-11-26 17:18:23 EST
> - install desktop files with desktop-file-install in %install

Why is this necessary?

> - Why not use the firefox icon if firefox is a requirement?  
> I think firefox.png would be better than redhat-web-browser.png
> - Why not use mozilla-firefox-32.desktop as the filename to be 
> consistent with the firefox package?

I am hesitant to do this because it is politically sensitive that this package
exists at all and I don't want to bother arguing with the Mozilla Corporation
about using their icon here.  I would be happy to use another icon if someone
provides one that looks attractive.

> - Consider also using the mozilla-firefox.desktop file's Name, 
> Generic Name and Comment fields to use as a template for the 
> firefox32 .desktop file.  I think the 32 bit version should have
> the same icon/description as the 64 bit version in order to make
> it consistent and easier to find.

Aside from the reason above, doing this would not work fully as expected due to
the many translations.

In my opinion it is totally not worth the effort to do any of this .desktop
branding improvements.  However, I will accept contributions to improve this if
someone goes through the effort to do it the right way.

rpmlint on SRPM:
> W: firefox-32 strange-permission setup-firefox-32.sh 0755
rpmlint on RPM:
> E: firefox-32 only-non-binary-in-usr-lib
> W: firefox-32 no-documentation
> W: firefox-32 one-line-command-in-%trigger /usr/lib/firefox-32/setup-firefox-32.sh
/tmp/firefox-32-0.0.1-3.i386.rpm.3552/usr/share/applications/firefox-32.desktop:
warning: boolean key "Terminal" has value "0", boolean values should be "false"
or "true", although "0" and "1" are allowed in this field for backwards
compatibility

I am going to ignore these as they are not a problem.

http://togami.com/~warren/fedora/firefox-32-0.0.1-2.src.rpm
http://togami.com/~warren/fedora/firefox-32.spec
* Sun Nov 26 2006 Warren Togami <wtogami@redhat.com> - 0.0.1-3
- change license to Public Domain
- own firefox-32 directory
- fix .desktop file s/True/true/
Comment 14 Christopher Stone 2006-11-26 17:53:59 EST
Okay, everything looks good except for the .desktop file which according to the
Packaging Guidelines needs to be installed with desktop-file-utils.

Since you are limited on time, I have gone ahead and fixed this for you:

SPEC: http://tkmame.retrogames.com/fedora-extras/firefox-32.spec
SRPM: http://tkmame.retrogames.com/fedora-extras/firefox-32-0.0.1-4.src.rpm

%changelog
* Sun Nov 26 2006 Christopher Stone <chris.stone@gmail.com> - 0.0.1-4
- Add desktop-file-install command to install .desktop entry
- Modify .desktop entry to more closely match firefox
- Add BuildArch: noarch

If you are okay with my version and agree to use it I will go ahead and
FE-ACCEPT this.
Comment 15 Warren Togami 2006-11-26 19:37:12 EST
thanks, done
Comment 16 Kevin Fenzi 2006-12-05 22:12:35 EST
It seems this package was added incorrectly to owners.list... 

Fedora Extras|firefox|firefox
browser|wtogami@redhat.com|extras-qa@fedoraproject.org|

Should be 'firefox-32' ? Although there is a (very old) firefox module available
for some reason. 
Comment 17 Warren Togami 2006-12-05 22:51:05 EST
Oops, good catch.  I forgot to add firefox-32 to owners.list.

The firefox there is the *OLD* firefox that was in fedora.us, before firefox
became part of Fedora Core.
Comment 18 Rahul Sundaram 2008-08-11 20:19:20 EDT
Make this not show up on reports

Note You need to log in before you can comment on or make changes to this bug.