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 ReviewAssignee: Michael Schwendt <bugs.michael>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: 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
Spec URL: http://jfsaucier.fedorapeople.org/packages/font-manager.spec
SRPM URL: http://jfsaucier.fedorapeople.org/packages/font-manager-0.4.2-1.fc12.src.rpm
Description: 
Font Manager is an application that allows users to easily manage fonts
on their system.

Although designed with the GNOME desktop environment in mind, it should
work well with most major desktop environments such as XFCE,
Enlightenment, and even KDE.


Here is the koji scratch build result:
F11 : https://koji.fedoraproject.org/koji/taskinfo?taskID=1899011
F12 : https://koji.fedoraproject.org/koji/taskinfo?taskID=1899009
F13 : https://koji.fedoraproject.org/koji/taskinfo?taskID=1899013


The only error rpmlint gave me is on the SPEC file and I think it can be safely ignored after my research :

font-manager.noarch: E: explicit-lib-dependency libxml2-python



I need someone to sponsor me for this package.

Thank you!

Comment 1 Terje Røsten 2010-01-03 10:55:51 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?

Comment 2 Jean-Francois Saucier 2010-01-03 18:18:51 UTC
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!

Comment 3 Michael Schwendt 2010-01-03 18:33:35 UTC
* 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.

Comment 4 Jean-Francois Saucier 2010-01-03 18:55:53 UTC
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.

Comment 5 Terje Røsten 2010-01-03 20:18:28 UTC
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.

Comment 6 Jean-Francois Saucier 2010-01-03 20:40:01 UTC
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.

Comment 7 Michael Schwendt 2010-01-03 20:49:54 UTC
> %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. ;)

Comment 8 Terje Røsten 2010-01-03 21:05:27 UTC
(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.

Comment 9 Jean-Francois Saucier 2010-01-03 21:23:58 UTC
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.

Comment 10 Michael Schwendt 2010-01-06 21:07:41 UTC
* 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.

Comment 11 Jean-Francois Saucier 2010-01-07 19:51:40 UTC
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!

Comment 12 Michael Schwendt 2010-01-08 23:33:21 UTC
 > 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.

Comment 13 Jean-Francois Saucier 2010-01-11 02:35:34 UTC
> 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!

Comment 14 Jean-Francois Saucier 2010-01-17 20:45:40 UTC
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!

Comment 15 Michael Schwendt 2010-01-25 21:35:12 UTC
* 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

Comment 16 Jean-Francois Saucier 2010-01-25 21:48:09 UTC
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:

Comment 17 Jason Tibbitts 2010-01-27 05:05:08 UTC
CVS done (by process-cvs-requests.py).

Comment 18 Fedora Update System 2010-01-28 02:56:01 UTC
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

Comment 19 Fedora Update System 2010-01-28 02:58:58 UTC
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

Comment 20 Fedora Update System 2010-01-29 03:22:43 UTC
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.

Comment 21 Fedora Update System 2010-01-29 03:30:58 UTC
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.