Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 179802
Summary: | Review Request: seamonkey | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Kai Engert (:kaie) (inactive account) <kengert> |
Component: | Package Review | Assignee: | Christopher Aillon <caillon> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | caillon, dmitry, gwync, jwboyer, stransky, zing |
Target Milestone: | --- | Flags: | gwync:
fedora-cvs+
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-04-11 16:29:12 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: | 163779 |
Description
Kai Engert (:kaie) (inactive account)
2006-02-03 01:03:03 UTC
I've been asked to use SeaMonkey (capitalized M) in descriptions, will change that in next version of spec file. Review comments: - Is this really distributed under NPL/MPL? I think it should be MPL/GPL/LGPL - Lose the Prefix: tag - Don't BuildRequire autoconf213; if you make changes to configure, include that part in the patch - Remove the ExclusiveArch. You are including all of our current platforms, and it probably builds on others that we don't. - You probably ought to have the FindExternalProvides stuff that the Firefox and Thunderbird package does. Since the libraries provided aren't versioned, this can cause problems when two packages provide the same libraries (mozilla also provides these for now, and when xulrunner eventually takes over, it will do so). - I think you can safely remove the conditional for desktop_file, unless you really want to push this to really old releases (I think FC1 needed it, newer don't). - Without a GRE, the -devel package should arguably not be built since that is a key part of the -devel platform. - regxpcom is no longer required. This (and the entire block surrounding it) should go away. - Your comment about cp -L doesn't seem needed. - Since this is for Fedora Extras, you probably shouldn't name the default pref/bookmarks files with redhat :-) - seamonkey-rebuild-databases should not be needed - I don't think selinux/chcon stuff should be in this specfile. Is there a bug you are trying to work around? - The following are installed with +x and shouldn't be. Using a %defattr in %files with the appropriate modes will fix this. ++ seamonkey ++ /usr/lib/seamonkey-1.0/components/nsXmlRpcClient.js /usr/lib/seamonkey-1.0/components/nsComposerCmdLineHandler.js /usr/lib/seamonkey-1.0/components/nsSidebar.js /usr/lib/seamonkey-1.0/components/nsProgressDialog.js /usr/lib/seamonkey-1.0/components/nsCloseAllWindows.js /usr/lib/seamonkey-1.0/components/nsHelperAppDlg.js /usr/lib/seamonkey-1.0/components/nsFilePicker.js /usr/lib/seamonkey-1.0/components/nsDictionary.js /usr/lib/seamonkey-1.0/components/nsUpdateNotifier.js /usr/lib/seamonkey-1.0/components/nsDownloadProgressListener.js /usr/lib/seamonkey-1.0/components/jsconsole-clhandler.js /usr/lib/seamonkey-1.0/components/nsProxyAutoConfig.js /usr/lib/seamonkey-1.0/components/nsResetPref.js /usr/lib/seamonkey-1.0/components/nsInterfaceInfoToIDL.js ++ seamonkey-chat ++ /usr/lib/seamonkey-1.0/components/chatzilla-service.js ++ seamonkey-dom-inspector ++ /usr/lib/seamonkey-1.0/components/inspector-cmdline.js ++ seamonkey-js-debugger ++ /usr/lib/seamonkey-1.0/components/venkman-service.js ++ seamonkey-mail ++ /usr/lib/seamonkey-1.0/components/offlineStartup.js /usr/lib/seamonkey-1.0/components/nsLDAPPrefsService.js /usr/lib/seamonkey-1.0/components/nsAbLDAPAttributeMap.js /usr/lib/seamonkey-1.0/components/smime-service.js /usr/lib/seamonkey-1.0/components/mdn-service.js ++++ Optional: - Use a .mozconfig file (see what I do in the firefox package). This will make it easier to do development with the same flags with a different tree (just copy the mozconfig over) Fixed up the tracker bugs > - Is this really distributed under NPL/MPL? Yes it is, I explicitly asked on irc.mozilla.org #seamonkey. In addition, SeaMonkey's about page says "Mozilla Public License and Netscape Public License" > - Lose the Prefix: tag removed Not sure which of all the changes were the culprit, but I got build failures, I had to replace some %{prefix} statements to make it work again. > - Don't BuildRequire autoconf213; if you make changes to configure, include that > part in the patch I think that request makes package maintenance a bit inconvenient, but you probably have a good reason to suggest that. removed > - Remove the ExclusiveArch. You are including all of our current platforms, and > it probably builds on others that we don't. removed > - You probably ought to have the FindExternalProvides stuff that the Firefox and > Thunderbird package does. Since the libraries provided aren't versioned, this > can cause problems when two packages provide the same libraries (mozilla also > provides these for now, and when xulrunner eventually takes over, it will do so). I'm confused, because you mention FindExternal and Provides in one word, but I can't find such a thing in the TB and FF spec files. Are you suggesting to add the following 3 lines? AutoProv: 0 %define _use_internal_dependency_generator 0 %define __find_requires %{SOURCE100} That's what I did. But I ended up with a depency problem, although package "seamonkey" contained libxpcom_core.so, package "seamonkey-mail" complained that lib can't be found. Therefore I set AutoProv to on, that made it work, but we again have lots of provides. Is that ok, or do you suggest a different way to fix it? > - I think you can safely remove the conditional for desktop_file, unless you > really want to push this to really old releases (I think FC1 needed it, newer > don't). removed %define desktop_file 1 and all conditionals and all "else" portions. > - Without a GRE, the -devel package should arguably not be built since that is a > key part of the -devel platform. Ok, removed for now, as the primary intention is indeed to provide the application. We can re-add it in a future package version, if required. > - regxpcom is no longer required. This (and the entire block surrounding it) > should go away. > - seamonkey-rebuild-databases should not be needed removed, also removed ldconfig commands > - Your comment about cp -L doesn't seem needed. It wasn't my comment, it's a leftover from mozilla.spec. removed > - Since this is for Fedora Extras, you probably shouldn't name the default > pref/bookmarks files with redhat :-) Renamed the files from "mozilla-redhat-" to "seamonkey-fedora-". Also changed the bookmarks file to match your firefox bookmarks file. > - I don't think selinux/chcon stuff should be in this specfile. Is there a bug > you are trying to work around? My rawhide system runs with selinux enabled. It failed during build, and using chcon was the only fix I found. But now that regxpcom is no longer necessary, chcon isn't either. I have now removed both regxpcom and chcon, and have been able to build on the rawhide system. > - Use a .mozconfig file (see what I do in the firefox package). This will make > it easier to do development with the same flags with a different tree (just copy > the mozconfig over) Ok, done. Motivated by your proposal, I tried to use the same build code as used in the firefox spec file. But that failed, I got errors in install stage, the pathes were incorrect. So I decided to continue to use the current build commands, and only moved the compilation configure options to mozconfig. I guess that's sufficient to satisfy your request. > - The following are installed with +x and shouldn't be. Using a %defattr in > %files with the appropriate modes will fix this. The mozilla 1.7.x package does the same thing! The statement currently in use is %defattr(-,root,root). I think we must not chance the modes for most of the files, so your request means, we'd have to filter the *.js files from the "list of files" and use explicit %defattr(644) statements for them. Do you want me to hack "sed/grep" code to change the file lists? Or should we use some "find" statements to change the modes before they are copied? Or do you have a better idea? Other changes: - capitalize M in SeaMonkey - use official 1.0 release source tarball New SPEC file: http://kuix.de/mozilla/seamonkey/1.0-4/src/seamonkey.spec New SRPM file: http://kuix.de/mozilla/seamonkey/1.0-4/src/seamonkey-1.0-4.src.rpm Note, if you want to download at the SOURCES without downloading the .src.rpm, look here: http://kuix.de/mozilla/seamonkey/1.0-4/src/ (In reply to comment #4) > > - Don't BuildRequire autoconf213; if you make changes to configure, include that > > part in the patch > > I think that request makes package maintenance a bit inconvenient, > but you probably have a good reason to suggest that. > removed Mozilla's build inadequacies is their problem. The only reason we have this package at all is because mozilla still needs it to generate configure. There's no need to force this requirement on people who simply want to build an SRPM. > > - You probably ought to have the FindExternalProvides stuff that the Firefox and > > Thunderbird package does. Since the libraries provided aren't versioned, this > > can cause problems when two packages provide the same libraries (mozilla also > > provides these for now, and when xulrunner eventually takes over, it will do so). > > I'm confused, because you mention FindExternal and Provides in one word, > but I can't find such a thing in the TB and FF spec files. > Are you suggesting to add the following 3 lines? > AutoProv: 0 > %define _use_internal_dependency_generator 0 > %define __find_requires %{SOURCE100} > That's what I did. > > But I ended up with a depency problem, although package "seamonkey" contained > libxpcom_core.so, package "seamonkey-mail" complained that lib can't be found. > Therefore I set AutoProv to on, that made it work, but we again have > lots of provides. Is that ok, or do you suggest a different way to fix it? Does it work if you Require: seamonkey (it should be Require anyway, and please one Require per line.) > > - Use a .mozconfig file (see what I do in the firefox package). This will make > > it easier to do development with the same flags with a different tree (just copy > > the mozconfig over) > > Ok, done. Motivated by your proposal, I tried to > use the same build code as used in the firefox spec file. > But that failed, I got errors in install stage, What specific errors? > The mozilla 1.7.x package does the same thing! Yeah, its a bug that we need to fix there, too. Don't copy over bugs :-) > The statement currently in use is %defattr(-,root,root). > I think we must not chance the modes for most of the files, > so your request means, we'd have to filter the *.js files > from the "list of files" and use explicit %defattr(644) statements for them. > Do you want me to hack "sed/grep" code to change the file lists? > Or should we use some "find" statements to change the modes before they > are copied? Or do you have a better idea? I think either solution is probably sufficient. Will look at the rest over the weekend. > > Ok, done. Motivated by your proposal, I tried to > > use the same build code as used in the firefox spec file. > > But that failed, I got errors in install stage, > > What specific errors? During install, the RPM build system tries to access a path that does not match what is produced in /var/tmp As it requires so much time to build, I was not motivated to make further attempts, but decided to go with the code that works. > Does it work if you Require: seamonkey (it should be Require anyway, and > please one Require per line.) You mean RequireS: instead of Prereq: ? Trying. > > Or should we use some "find" statements to change the modes before they > > are copied? Or do you have a better idea? > > I think either solution is probably sufficient. I found the existing spec has # fix permissions of the chrome directories /usr/bin/find . -type d -perm 0700 -exec chmod 755 {} \; || : I will add # We don't want JS files to be executable /usr/bin/find . -type f -name \*.js -exec chmod 644 {} \; || : and see if that works > > Does it work if you Require: seamonkey (it should be Require anyway, and > > please one Require per line.) > You mean RequireS: instead of Prereq: ? > Trying. No, it still does not work. [root@kaiefast x86_64]# rpm -ivh seamonkey-1.0-5.x86_64.rpm seamonkey-mail-1.0-5.x86_64.rpm error: Failed dependencies: libxpcom_core.so()(64bit) is needed by seamonkey-mail-1.0-5.x86_64 > > - The following are installed with +x and shouldn't be. > > I will add > # We don't want JS files to be executable > /usr/bin/find . -type f -name \*.js -exec chmod 644 {} \; || : > and see if that works Yes, that worked fine. Summary of the previous comments. Chris, from all your comments, only one issue is left: > > - You probably ought to have the FindExternalProvides stuff that the Firefox and > > Thunderbird package does. Since the libraries provided aren't versioned, this > > can cause problems when two packages provide the same libraries (mozilla also > > provides these for now, and when xulrunner eventually takes over, it will do so). > > Are you suggesting to add the following 3 lines? > AutoProv: 0 > %define _use_internal_dependency_generator 0 > %define __find_requires %{SOURCE100} As said before, that didn't work for me. You also proposed > Does it work if you Require: seamonkey (it should be Require anyway, and please > one Require per line.) But that didn't work either. What should we do? Is there an easy way to make the dependency/provides system use absolute pathes, so your predicted ambiguity won't occur, or could we skip this for now? I discussed with Chris. In order to get rid of the "Provides" problem, we agreed to simplify, no longer use separate packages for mail etc., but produce a single seamonkey package. I'll also remove the devel package. I uploaded a new spec file to http://kuix.de/mozilla/seamonkey/1.0-6/src/seamonkey.spec See also this diff: http://kuix.de/mozilla/seamonkey/1.0-6/src/spec.diff Chris, can you please continue the review? Chris asked me to run mock and rpmlint on the RPM. mock does not seem to report anything, it says: init clean prep setup build ending done rpmlint gives source: W: seamonkey strange-permission seamonkey-make-package.pl 0775 W: seamonkey strange-permission seamonkey.sh.in 0775 W: seamonkey strange-permission find-external-requires 0775 W: seamonkey prereq-use fileutils perl W: seamonkey prereq-use /usr/bin/killall W: seamonkey prereq-use desktop-file-utils >= %{desktop_file_utils_version} binary: W: seamonkey invalid-license NPL/MPL W: seamonkey dangerous-command-in-%preun rm W: seamonkey uncompressed-zip /usr/lib/seamonkey-1.0/chrome/messenger.jar E: seamonkey no-jar-manifest /usr/lib/seamonkey-1.0/chrome/messenger.jar W: seamonkey uncompressed-zip /usr/lib/seamonkey-1.0/chrome/en-mac.jar E: seamonkey no-jar-manifest /usr/lib/seamonkey-1.0/chrome/en-mac.jar W: seamonkey uncompressed-zip /usr/lib/seamonkey-1.0/chrome/en-US.jar E: seamonkey no-jar-manifest /usr/lib/seamonkey-1.0/chrome/en-US.jar W: seamonkey uncompressed-zip /usr/lib/seamonkey-1.0/chrome/toolkit.jar E: seamonkey no-jar-manifest /usr/lib/seamonkey-1.0/chrome/toolkit.jar W: seamonkey uncompressed-zip /usr/lib/seamonkey-1.0/chrome/en-win.jar E: seamonkey no-jar-manifest /usr/lib/seamonkey-1.0/chrome/en-win.jar W: seamonkey uncompressed-zip /usr/lib/seamonkey-1.0/chrome/help.jar E: seamonkey no-jar-manifest /usr/lib/seamonkey-1.0/chrome/help.jar W: seamonkey uncompressed-zip /usr/lib/seamonkey-1.0/chrome/sroaming.jar E: seamonkey no-jar-manifest /usr/lib/seamonkey-1.0/chrome/sroaming.jar W: seamonkey uncompressed-zip /usr/lib/seamonkey-1.0/chrome/content-packs.jar E: seamonkey no-jar-manifest /usr/lib/seamonkey-1.0/chrome/content-packs.jar W: seamonkey uncompressed-zip /usr/lib/seamonkey-1.0/chrome/classic.jar E: seamonkey no-jar-manifest /usr/lib/seamonkey-1.0/chrome/classic.jar W: seamonkey uncompressed-zip /usr/lib/seamonkey-1.0/chrome/US.jar E: seamonkey no-jar-manifest /usr/lib/seamonkey-1.0/chrome/US.jar W: seamonkey uncompressed-zip /usr/lib/seamonkey-1.0/chrome/comm.jar E: seamonkey no-jar-manifest /usr/lib/seamonkey-1.0/chrome/comm.jar W: seamonkey uncompressed-zip /usr/lib/seamonkey-1.0/chrome/modern.jar E: seamonkey no-jar-manifest /usr/lib/seamonkey-1.0/chrome/modern.jar W: seamonkey uncompressed-zip /usr/lib/seamonkey-1.0/chrome/inspector.jar E: seamonkey no-jar-manifest /usr/lib/seamonkey-1.0/chrome/inspector.jar W: seamonkey uncompressed-zip /usr/lib/seamonkey-1.0/chrome/pippki.jar E: seamonkey no-jar-manifest /usr/lib/seamonkey-1.0/chrome/pippki.jar W: seamonkey uncompressed-zip /usr/lib/seamonkey-1.0/chrome/pipnss.jar E: seamonkey no-jar-manifest /usr/lib/seamonkey-1.0/chrome/pipnss.jar W: seamonkey uncompressed-zip /usr/lib/seamonkey-1.0/chrome/venkman.jar E: seamonkey no-jar-manifest /usr/lib/seamonkey-1.0/chrome/venkman.jar W: seamonkey uncompressed-zip /usr/lib/seamonkey-1.0/chrome/en-unix.jar E: seamonkey no-jar-manifest /usr/lib/seamonkey-1.0/chrome/en-unix.jar W: seamonkey uncompressed-zip /usr/lib/seamonkey-1.0/chrome/chatzilla.jar E: seamonkey no-jar-manifest /usr/lib/seamonkey-1.0/chrome/chatzilla.jar Actually I asked the SeaMonkey developers again regarding the license, and the NPL/MPL was a mistake. I'll change that to be same as firefox: MPL/LGPL Is action required for the other items? - No need to Require: nss, nspr. RPM will auto generate those requires. - Please use %{?_smp_mflags} instead of -j$CPUS and you can get rid of the relevant getconf calls. - It would be good to get those scripts that rpmlint complained about changed to 0755 instead of 0775. - Also, I think it would be good to try and use %ghost to avoid having to do the rm stuff in %preun - %define _unpackaged_files_terminate_build 0 I just noticed this on. Can you turn that off and make sure it still builds? If there are unpackaged files, you should explicitly rm them, or list them in your %files section with %exclude prepended. > No need to Require: nss, nspr. RPM will auto generate those requires.
I can't confirm that. (Remember that we use the find-external-requires tool, if
that has any influence on the auto generate that you expected.).
With the old version of the spec (that explicitly requires nspr/nss):
[kaie@kaiefast x86_64]$ rpm -qp --requires seamonkey-1.0-5.x86_64.rpm | egrep -i
'nspr|nss'
nspr >= 4.6
nss >= 3.11
libnspr4.so()(64bit)
libnss3.so()(64bit)
Using your proposal to remove the Requires I get:
[kaie@kaiefast x86_64]$ rpm -qp --requires seamonkey-1.0-6.x86_64.rpm | egrep -i
'nspr|nss'
libnspr4.so()(64bit)
libnss3.so()(64bit)
Is that what you want?
> Please use %{?_smp_mflags} instead of -j$CPUS and you can get rid of the > relevant getconf calls. Ok, changed and removed. > - %define _unpackaged_files_terminate_build 0 > I just noticed this on. Can you turn that off and make sure it still builds? > If there are unpackaged files, you should explicitly rm them, or list them in > your %files section with %exclude prepended. Talked to Chris off-bugzilla. As there are 3500 unpackaged files, it would be a lot of work to get this cleaned up. As the existing Mozilla package has this flag turned on, too, we decided to use it for SeaMonkey, too. > - It would be good to get those scripts that rpmlint complained about > changed to 0755 instead of 0775. Done. Note that rpmlint still complains. But again this warning is produced on the existing Mozilla packages, too, so that's not new a bug I'm introducing, but it would be great if we could postpone that. > - Also, I think it would be good to try and use %ghost to avoid having to do > the rm stuff in %preun I added two more explicitly listed rdf files. There were no unlisted overlayinfo dirs left, so no need to "rm". I added the greprefs %dir, so it gets uninstalled properly. I removed the %preun section. I did an "install - execute - uninstall" cycle and verified that all files get removed. I will upload an updated SPEC when I know your answer to comment 14, thanks. Yeah, comment 14 is correct. Since the sonames will be bumped if the ABI breaks, we should be fine with just generating the requires on the actual libraries. This is what we do for other packages. (This helps for when libraries change packagenames as things did recently with mozilla-nss -> nss .. it probably won't change anymore, but the prinicple is still the same) Chris, can you please look whether you need any additional changes? http://kuix.de/mozilla/seamonkey/1.0-7/src/seamonkey.spec http://kuix.de/mozilla/seamonkey/1.0-7/src/spec.diff http://kuix.de/mozilla/seamonkey/1.0-7/src/seamonkey-1.0-7.src.rpm > As there are 3500 unpackaged files,
May I ask why 3500 files are not covered by any %files section?
> May I ask why 3500 files are not covered by any %files section?
The mozilla install target installs a lot of various files for development and
testing, some files that seem to belong to calendar, etc.
Whoever wrote the original spec files (probably a long while ago) for the
mozilla package seems to have made the decision that it's too much work to sort
out everything, manually pick the files we need to ship, and ignore the rest.
Pure guessing from my part.
We decided to use MPL as the license tag, because that's the only license that covers everything. The license tag is the only difference to the previous spec file. http://kuix.de/mozilla/seamonkey/1.0-8/src/seamonkey.spec http://kuix.de/mozilla/seamonkey/1.0-8/src/seamonkey-1.0-8.src.rpm Chris, is this version acceptable for the initial seamonkey package release? 4 weeks passed since I made the previous comment. Could you please approve, or do you want me to make further changes? Thanks. Okay, looks good enough. There's a few issues still outstanding, but those are large hairy issues, and those exist in the mozilla package itself as well. Package APPROVED Thanks for approving!!! Before importing, I will create a new release. The only thing that will be new: I will increase the release number and add suffix %{?dist} as suggested on http://fedoraproject.org/wiki/Extras/BuildRequests Package built into Development and FC-5. Package Change Request ====================== Package Name: seamonkey New Branches: el6 Owners: buc gecko-maint members, any objections? Owners, any objections of trademark ACL issues here? No objections. Thanks. Dmitry, re-set the cvs flag, then, unless someone objects. Re-set again. Git done (by process-git-requests). Thanks! |