Bug 505360
Summary: | Review Request: JSCookMenu - Javascript GUI-like web menus | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Patrick Monnerat <patrick> |
Component: | Package Review | Assignee: | Caius Chance <K9> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | fedora-package-review, kevin, notting, petersen |
Target Milestone: | --- | Flags: | K9:
fedora-review+
kevin: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | JSCookMenu-2.0.4-3.fc13 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2010-05-10 17:02:06 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: |
Description
Patrick Monnerat
2009-06-11 16:12:08 UTC
Hi I'm here for prereview: ===== $ rpmlint SPECS/JSCookMenu.spec 0 packages and 1 specfiles checked; 0 errors, 0 warnings. ===== Please either include license file and mention in every source file, or include license contents in every source file. Also, please persuade author to adopt MIT license rather than MIT-compatible license for legal concerns. The current license is OK: this is "MIT modern style with sublicense" https://fedoraproject.org/wiki/Licensing/MIT#Modern_Style_with_sublicense Upstream request for license clarification has been submitted: http://yuanheng.org/forum/viewtopic.php?t=1690 Cool! I will follow up then. I got a reply on http://yuanheng.org/forum/viewtopic.php?t=1690 The theme files are public domain. Unfortunately this clarification only came on the forum page. As you can see there, I'm insisting... hang on ! License clarified by e-mail. E-mail added in %doc. SRPM: http://monnerat.fedorapeople.org/JSCookMenu-2.0.4-2.fc12.src.rpm rpmlint output: JSCookMenu-2.0.4-2.fc12.src.rpm: none JSCookMenu.spec: none JSCookMenu-2.0.4-2.fc12.noarch.rpm: W: file-not-utf8 /usr/share/doc/JSCookMenu-2.0.4/JSCookMenu_theme_license_clarification.mail.txt The license clarification e-mail as downloaded contains an ISO-8859-1 part (with a single ISO8859-1 non-paddable space). Great work Patrick! I have checked the .spec again and please improve the commands into Macro ones: https://fedoraproject.org/wiki/Extras/ReferenceMandrakeRPMMacros Sorry to tell you, but I do not agree with this suggestion for the following reasons: _ Although these macros are effectively defined in the /usr/lib/rpm/macros file, they are mainly intended to ease porting specs from the old Mandrake distribution. The 2 years-old page you reference to is annotated "This page could probably be deleted... Doesn't seem to be much useful information" and the referring page (https://fedoraproject.org/wiki/Packaging:RPMMacros#Reference) lists it only as a conversion aid from Mandrake spaces (altough not forbidding use of this macros, I conceed to you). _ I did not find usage references to these macros in the packaging guidelines, neither as a requirement nor even as a recommandation. _ The use of these macros is not very frequent in current Fedora spec files. _ Finally, I do not like them: they lower the spec file readability and they introduce a dependency to their definitions while it is unlikely that their defined value changes sometime. MUST: Each package must consistently use macros. I was misunderstood about this on review guidelines. Now I would interpret that as "either use that at highest degree or just don't use that at all". Given that you haven't used it almost. This should be recognized as proper in your .spec file. The .spec and srpm looks okay to me, but you need to get a sponsor for this new package approved. I agree with your new opinion about macro usage consistency: if a particular macro (or macro "class") is used in a package, it must be used everywhere it can ! Thanks for reviewing. However I do not need a sponsor: I'm a Fedora developer for 18 months now! If you're still OK to accept this review, please set the flag accordingly. Thanks $ rpmlint SPECS/JSCookMenu.spec SRPMS/JSCookMenu-2.0.4-2.fc12.src.rpm JSCookMenu.src: W: spelling-error Summary(en_US) Javascript -> JavaScript, Java script, Java-script 1 packages and 1 specfiles checked; 0 errors, 1 warnings. # Ignorable. All MUST are passed. Approved. Many thanks for your work on this, Kaio. New Package CVS Request ======================= Package Name: JSCookMenu Short Description: Javascript GUI-like web menus Owners: monnerat Branches: F-12 F-13 InitialCC: I can't seem to find the guideline now, but I don't think you should reload httpd in pre/post here. Think about a web server where the admin is installing some packages and a reload disrupts their server (or there are changes that are incomplete and it doesn't restart correctly). I would advise not restarting httpd here. Well... this is a philosophical problem that could well degenerate into a troll :-( In the case you mention, you're probably right, but this implies system updates are done while someone changes the server configuration, saves the (bad) configuration and does not test/apply the changes immediately... IMHO, this situation will occur very seldom. However, I'm not wrong because there is an /etc/httpd/conf.d/*.conf file in this package and a server reload is needed to enable/disable it. phpPgAdmin (for example) reloads the server at post time for the same reason. And by the way, this is just a reload, not a restart. This ideal situation would be to have an Apache-enabled /usr/share/js directory for those kind of packages (a bit like /usr/share/php for php packages). This would allow new js packages to get rid of a specific httpd configuration file. But Fedora does not (yet) include such a directory in its standard base, nor there is specific guidelines for javascript packaging. In the meantime, I suggest we meet halfway by reloading the server only at first package install and last uninstall. @Kevin: would it be satisfactory to you ? Well, the only packages in the distro that do this are: phpPgAdmin, postgresql-pgpoolAdmin and w3c-markup-validator. There are 134 packages in rawhide that provide a /etc/httpd/conf.d/*.conf file. I would suggest that the 134 packages are doing the right thing here and the other 3 should get fixed to not do this. ;) I won't hold up your review any further, but I think consulting the packaging mailing list might be a good idea before importing? CVS done (by process-cvs-requests.py). JSCookMenu-2.0.4-3.fc12 has been submitted as an update for Fedora 12. http://admin.fedoraproject.org/updates/JSCookMenu-2.0.4-3.fc12 JSCookMenu-2.0.4-3.fc13 has been submitted as an update for Fedora 13. http://admin.fedoraproject.org/updates/JSCookMenu-2.0.4-3.fc13 Thanks Kevin. Typo fixed and post/postun scriptlets removed. JSCookMenu-2.0.4-3.fc12 has been pushed to the Fedora 12 stable repository. If problems still persist, please make note of it in this bug report. JSCookMenu-2.0.4-3.fc13 has been pushed to the Fedora 13 stable repository. If problems still persist, please make note of it in this bug report. |