Bug 551878
Summary: | Review Request: font-manager - A font management application for the GNOME desktop environment | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Jean-Francois Saucier <jsaucier> |
Component: | Package Review | Assignee: | Michael Schwendt <bugs.michael> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | low | ||
Version: | rawhide | CC: | bugs.michael, fedora-package-review, notting, panemade, terje.rosten |
Target Milestone: | --- | Flags: | bugs.michael:
fedora-review+
j: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | 0.4.3-1.fc12 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2010-01-29 03:22:48 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
Jean-Francois Saucier
2010-01-02 20:12:21 UTC
Some comments: a) you can remove python from req, pygtk2 will pull that, drop commas too. b) spell out the file names in %files c) you have to handle the desktop file: https://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files d) Drop INSTALL from %doc e) why is these needed: %attr(0755,root,root) and %attr(0644,root,root) in %files needed? f) never seen _datarootdir used before, why not _datadir? Spec URL: http://jfsaucier.fedorapeople.org/packages/font-manager.spec SRPM URL: http://jfsaucier.fedorapeople.org/packages/font-manager-0.4.2-2.fc12.src.rpm > a) you can remove python from req, pygtk2 will pull that, drop commas too. Done. > b) spell out the file names in %files Done for some files (bindir and .desktop). I have a small question here. For example, I have %{_datadir}/font-manager/* that glob many files. Do I need to list all of them or the glob is alright in this case? > c) you have to handle the desktop file Didn't know of that one. Thanks for the pointer, I think it is solved now. > d) Drop INSTALL from %doc Done. > e) why is these needed: %attr(0755,root,root) and %attr(0644,root,root) in %files needed? I removed the %attr(0755,root,root) because this was not necessary. But if I remove the %attr(0644,root,root) one, the .desktop file get installed 0755. > f) never seen _datarootdir used before, why not _datadir? Fixed, I didn't see _datadir when I grep rpmbuild --showrc the first time. Also, I added this in %file section because the folder was not removed cleanly after a rpm -e : %dir %{_datadir}/font-manager/ Thank you very much for the help! * https://fedoraproject.org/wiki/Packaging:UnownedDirectories#Wildcarding_Files_inside_a_Created_Directory * Instead of using %attr you could simply chmod -x the file in %prep or %install. Spec URL: http://jfsaucier.fedorapeople.org/packages/font-manager.spec SRPM URL: http://jfsaucier.fedorapeople.org/packages/font-manager-0.4.2-3.fc12.src.rpm I removed the %dir and the wildcards and I followed the guidelines in the link you gave me. I also used chmod -x in place of %attr. Is there a reason one is preferred? I thought that %attr would be better option than a direct chmod. Thank you for taking the time to review this package. Nice work, I like to use only blank line between %-sections and %{optflags} for $RPM_BUILD_ROOT, however that's me. I can do a formal review, however you need a sponsor. Pardon me if it's a stupid question but I don't really understand what you mean by "blank line between %-sections and %{optflags} for $RPM_BUILD_ROOT". I will try to continue my work to get a sponsor. Thanks a lot for your time. > %attr vs. chmod
Hmm... there are no guidelines for everything. :) Though, %attr in the %files section is very explicit and reduces readability of the %files section. It's better to limit it to the special cases when you cannot avoid using it. That is when you need to set username, groupname, or security relevant permission bits. You want such special cases in the %files section to catch your eyes. As in "Caution! %attr is used, there's something special about that file/directory".
On the contrary, using %attr only to set ordinary file access permissions is overuse of a macro. Imagine you would need to fix incorrect permissions of more files and also set special security relevant permissions on a few important files. Overuse of %attr adds "noise". The %prep section is the more convenient place where to fix ordinary access bits of files in a source tarball, e.g. using "find" commands.
The .desktop file is 0755 because of a bug in the tarball's Makefile.am. Better fix it and submit the patch upstream. ;)
(In reply to comment #6) > Pardon me if it's a stupid question but I don't really understand what you mean > by "blank line between %-sections and %{optflags} for $RPM_BUILD_ROOT". > > I will try to continue my work to get a sponsor. I am sorry, a typo there, I mean I prefer %{buildroot} over $RPM_BUILD_ROOT. Both are valid. You use two blank lines to separate e.g. prep and build: %prep %setup -q # first blank line # second blank line %build I prefer just one line. Anyway, these are just some pedantic hangups from my side. Spec URL: http://jfsaucier.fedorapeople.org/packages/font-manager.spec SRPM URL: http://jfsaucier.fedorapeople.org/packages/font-manager-0.4.2-4.fc12.src.rpm I patched the Makefile to fix the permission bug and I removed one blank line between the sections. Thanks to both of you, I really like what I learn during this review process. * License is "GPLv3+" because of the "any later version" in the GPL header of the source files. * Why are the *.py source files not included in the font-manager-0.4.2-4.fc12.noarch build? That's so untypical for Python. It may be necessary to patch the Makefile to install the *.py files and possibly take the chance to compile with -O1 instead of -O0. * Here are some Python specific packaging guidelines: https://fedoraproject.org/wiki/Packaging:Python * The "tmpdir" code in /usr/share/font-manager/export.py* is reason to worry. While shutil.rmtree does not follow symlinks, an attacker could cause another user's font-manager to crash (raising an OSError exception) by creating arbitrary symlinks in /tmp. Spec URL: http://jfsaucier.fedorapeople.org/packages/font-manager.spec SRPM URL: http://jfsaucier.fedorapeople.org/packages/font-manager-0.4.2-5.fc12.src.rpm > * License is "GPLv3+" Fixed. > * Why are the *.py source files not included Fixed. I patched the upstream Makefile to correct this. The weird thing is that static.py must not be included in the final package, so I cannot make the *.py[co] regexp into *.py* in the Makefile. I found a good regexp for this yesterday but it didn't work in Makefile : install -m 644 !(static).py?(c|o) $(DESTDIR)$(prefix)/share/font-manager/ It work fine in a shell but the Makefile didn't like the parenthesis. I have talked about this in #fedora-devel and not found a solution. For the moment, I resign myself to include the name of all the files I want packaged. > and possibly take the chance to compile with -O1 instead of -O0. Maybe there is something I don't understand here (I am kind of new to python) but in the Makefile, I have : @python -OO -m compileall . So, it seems to compile with -O2 if I understand correctly. Am I wrong here? > * The "tmpdir" code in /usr/share/font-manager/export.py* is reason to worry. Like I said before, I am kind of new in python, so I will try to look at this issue in collaboration with upstream. I will come back as soon as I have something on this. Thank you! > install -m 644 !(static).py?(c|o) $(DESTDIR)$(prefix)/share/font-manager/ install -m 644 *.py{,c,o} $(DESTDIR)$(prefix)/share/font-manager/ rm $(DESTDIR)$(prefix)/share/font-manager/static.py* :-) > but in the Makefile, I have : > > @python -OO -m compileall . > > So, it seems to compile with -O2 if I understand correctly. Am I wrong here? $ grep compile src/Makefile.am @python -m compileall . @python -OO -m compileall . That creates .pyc files in first pass and .pyo files in second pass. Then with regard to $ python --help|grep OO -OO : remove doc-strings in addition to the -O optimizations and https://fedoraproject.org/wiki/Packaging:Python#Including_pyos https://fedoraproject.org/wiki/Packaging:Python#Byte_Compiled_Files I thought you could compile with plain -O as rpmbuild's /usr/lib/rpm/brp-python-bytecompile script does it. Not a blocker, though. It's not a MUST in the guidelines. > install -m 644 *.py{,c,o} $(DESTDIR)$(prefix)/share/font-manager/
> rm $(DESTDIR)$(prefix)/share/font-manager/static.py*
I didn't think of that way, yours ease the reading of the Makefile and facilitate the maintenance.
Also, thanks for the explanation on Python optimization. I just bought a book on Python and plan to gain some skills on it in the next few weeks.
I didn't submit any new SPEC and SRPM because the author plan a new release this week fixing the .desktop file permission, the Makefile problem, some other bugs and more importantly, the "tmpdir" problem.
As soon as 0.4.3 is out, I will submit an updated package here.
Thank you!
Spec URL: http://jfsaucier.fedorapeople.org/packages/font-manager.spec SRPM URL: http://jfsaucier.fedorapeople.org/packages/font-manager-0.4.3-1.fc12.src.rpm Here is the new version of font-manager. I want to take the time to thanks Jerry Casiano, the upstream author of font-manager. He released this new version which fix the Makefile problems and the tmpdir issue. I also correct (I think!) the python optimization. All the other patches serve no purpose now and have been removed. Here is the only error rpmlint output on the final rpm : font-manager.noarch: E: explicit-lib-dependency libxml2-python font-manager.noarch: E: zero-length /usr/share/doc/font-manager-0.4.3/TODO I think we can ignore the libxml2-python as before. The TODO one is a new error but I think I would like to leave this file in the spec as future release will probably have something in there. The package build fine in my local mock instance. Thank you! * That does it! * Relocating the temp dir into a private directory below $HOME is not ideal (with regard to environments where TMP is faster), but more secure than the prev.release. * libxml2-python : yes, a false positive * Empty %doc files : not really worth the increased effort to deal with them. Though, note that it _would_ be possible to examine the extracted source tarball in %prep and place guards after %setup. E.g. terminate the build if special conditions are met or not. Not specific to empty %doc files, can be helpful with optional source directories and other conditions, too. APPROVED Thank you! New Package CVS Request ======================= Package Name: font-manager Short Description: A font management application for the GNOME desktop environment Owners: jfsaucier Branches: F-11 F-12 InitialCC: CVS done (by process-cvs-requests.py). font-manager-0.4.3-1.fc11 has been submitted as an update for Fedora 11. http://admin.fedoraproject.org/updates/font-manager-0.4.3-1.fc11 font-manager-0.4.3-1.fc12 has been submitted as an update for Fedora 12. http://admin.fedoraproject.org/updates/font-manager-0.4.3-1.fc12 font-manager-0.4.3-1.fc11 has been pushed to the Fedora 11 stable repository. If problems still persist, please make note of it in this bug report. font-manager-0.4.3-1.fc12 has been pushed to the Fedora 12 stable repository. If problems still persist, please make note of it in this bug report. |