Fedora Merge Review: fontconfig http://cvs.fedora.redhat.com/viewcvs/devel/fontconfig/ Initial Owner: besfahbo
Mock build for rawhide is fine. rpmlint on SRPM reported W: fontconfig prereq-use freetype >= %{freetype_version}, coreutils The use of PreReq is deprecated. In the majority of cases, a plain Requires is enough and the right thing to do. Sometimes Requires(pre), Requires(post), Requires(preun) and/or Requires(postun) can also be used instead of PreReq. W: fontconfig make-check-outside-check-section make check Make check or other automated regression test should be run in %check, as they can be disabled with a rpm macro for short circuiting purposes. W: fontconfig macro-in-%changelog 5x Macros are expanded in %changelog too, which can in unfortunate cases lead to the package not building at all, or other subtle unexpected conditions that affect the build. Even when that doesn't happen, the expansion results in possibly "rewriting history" on subsequent package revisions and generally odd entries eg. in source rpms, which is rarely wanted. Avoid use of macros in %changelog altogether, or use two '%'s to escape them, like '%%foo'. rpmlint on RPMS reported W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/10-autohint.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/10-no-sub-pixel.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/10-sub-pixel-bgr.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/10-sub-pixel-rgb.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/10-sub-pixel-vbgr.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/10-sub-pixel-vrgb.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/10-unhinted.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/20-fix-globaladvance.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/20-lohit-gujarati.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/20-unhint-small-vera.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/30-amt-aliases.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/30-urw-aliases.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/40-generic.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/49-sansserif.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/50-user.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/51-local.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/60-latin.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/65-fonts-persian.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/65-nonlatin.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/69-unifont.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/70-no-bitmaps.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/70-yes-bitmaps.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/80-delicious.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.avail/90-synthetic.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.d/20-fix-globaladvance.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.d/20-lohit-gujarati.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.d/20-unhint-small-vera.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.d/25-no-hint-fedora.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.d/30-aliases-fedora.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.d/30-amt-aliases.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.d/30-urw-aliases.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.d/40-generic-fedora.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.d/40-generic.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.d/49-sansserif.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.d/50-user.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.d/51-local.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.d/60-latin.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.d/64-nonlatin-fedora.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.d/65-fonts-persian.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.d/65-nonlatin.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.d/69-unifont.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.d/75-blacklist-fedora.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.d/80-delicious.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/conf.d/90-synthetic.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig conffile-without-noreplace-flag /etc/fonts/fonts.conf A configuration file is stored in your package without the noreplace flag. A way to resolve this is to put the following in your SPEC file: %config(noreplace) /etc/your_config_file_here W: fontconfig non-conffile-in-etc /etc/fonts/fonts.dtd A non-executable file in your package is being installed in /etc, but is not a configuration file. All non-executable files in /etc should be configuration files. Mark the file as %config in the spec file. W: fontconfig non-conffile-in-etc /etc/fonts/conf.avail/README A non-executable file in your package is being installed in /etc, but is not a configuration file. All non-executable files in /etc should be configuration files. Mark the file as %config in the spec file. W: fontconfig dangerous-command-in-%post rm
Created attachment 149010 [details] Some Changes to Original SPEC You SHOULD 1) Use attached SPEC 2) run rpmlint and check for further errors 3) I saw rpath is used for building this package. Remove rpath. 4) rpmlint reported rm is used in $post section. Can you give reasons for that? 5) Can't /etc/fonts/conf.avail/README be installed under /usr/share/doc
We don't want to mark those config files as noreplace, as they should not be touched. That has saved us when switching to fontconfig 2.4 already.
In case you have %config and %config(noreplace) in your SPEC then You may like to update the SPEC by removing that. Check http://fedoraproject.org/wiki/Packaging/Minutes20070313 and http://fedoraproject.org/wiki/PackagingDrafts/UsrConfigs
any updates?
1) as I said, I don't want %config(noreplace). 2) what's the point in patching changelog entries? Please attach a patch addressing this issues.
I, for one, agree with the above (ancient) commentary: as long as you justify not using noreplace you should be fine. All you have to do is briefly note it in the spec file. There isn't a whole lot to do for this package; most everything is a simple one-line tweak. If you need someone else to make a patch to fix these issues for some reason I can do so. A full review for this package is attached. Below are just the parts that need fixing. (Hopefully bugzilla won't mangle my spacing.) Mandatory review guidelines: NO - rpmlint output prereq is deprecated: Change PreReq to Requires make-check-outside-check-section: Just put make check in a %check section conffile-without-noreplace-flag: Justified in this bug; should also note in spec NO - Package's files and directories don't conflict with others' Both fontconfig and fontpackages-filesystem own /usr/share/fonts Packaging guidelines: NO - Spec file lacks Packager, Vendor, PreReq tags Please change PreReq to Requires. NO - %build honors applicable compiler flags or justifies otherwise Building with make %{?_smp_mflags} seems to work for me; if it actually doesn't please mention so in the spec file. NO - %config files marked noreplace or justified Justified in this bug; should also note in spec file
Created attachment 430527 [details] Review of fontconfig-2.8.0-1
(In reply to comment #7) > NO - %build honors applicable compiler flags or justifies otherwise > Building with make %{?_smp_mflags} seems to work for me; if it actually > doesn't please mention so in the spec file. Whoops, the description for this (optional) problem got matched with the wrong guideline. It isn't mandatory, but if it works it probably won't hurt to add it anyway.
Created attachment 455694 [details] spec cleanup This git patch will clean this package according to packaging guidelines and also try to resolve bug 477054 and bug 645080.
mclasen, Can you please check the attached patch and approve to commit this?
your patch seems to add %config(noreplace) despite comment #3 and comment #6
Thank you for your quick reply. My patch is also based on what Nicolas Mailhot suggested in https://bugzilla.redhat.com/show_bug.cgi?id=477054#c6 There are 2 changes in spec related to %config(noreplace) 1)%config(noreplace) %{_fontconfig_confdir}/*.conf which is same as written in current spec in master %config(noreplace) %{_sysconfdir}/fonts/conf.d/*.conf 2) %config(noreplace) %{_fontconfig_masterdir}/fonts.conf which is NOT the same as compared to current spec in master %config %{_sysconfdir}/fonts/fonts.conf Therefore, I will drop the second change only and will commit rest.
(In reply to comment #13) > Thank you for your quick reply. My patch is also based on what Nicolas Mailhot > suggested in https://bugzilla.redhat.com/show_bug.cgi?id=477054#c6 Note that this patch depends on an upstream fontconfig patch that makes fontconfig install its templates in /usr/share. So it works because with that patch fontconfig no longer installs files the user should not modify in /etc. If someone could ping Behdad to get the upstream fontconfig patch merged, then we could simplify the fedora spec file.
See also https://bugs.freedesktop.org/show_bug.cgi?id=29341
ping behdad
Leaving this review as there is no response to fix this package.
Matthias, I am not sure if anyone is actively working on Fedora fontconfig package maintenance. We have got new fontconfig project leader(Akira Tagoh) in upstream. Soon a new release of fontconfig will be available. If its ok with you can this package be reassigned to Akira?
fontconfig 2.9.0 is released now. Can we have it built for Fedora 17?
I think current package should be better now. can someone start review again?
let me see if I can review this.
Issues: ======= [!]: %config files are marked noreplace or the reason is justified. Note: %config %{_fontconfig_masterdir}/fonts.conf See: http://fedoraproject.org/wiki/Packaging/Guidelines#Configuration_files [!]: Large documentation must go in a -doc subpackage. Note: Documentation size is 1034240 bytes in 195 files. See: http://fedoraproject.org/wiki/Packaging/Guidelines#PackageDocumentation [!]: Fully versioned dependency in subpackages, if present. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in %package devel [!]: I don't think you need to explicitly "Requires: pkgconfig" in spec file [!]: Patches link to upstream bugs/comments/lists or are otherwise justified. Rpmlint ------- Checking: fontconfig-2.10.2-1.fc18.x86_64.rpm fontconfig-debuginfo-2.10.2-1.fc18.x86_64.rpm fontconfig-devel-2.10.2-1.fc18.x86_64.rpm fontconfig-2.10.2-1.fc18.src.rpm fontconfig.x86_64: W: conffile-without-noreplace-flag /etc/fonts/fonts.conf fontconfig.x86_64: W: non-conffile-in-etc /etc/fonts/conf.d/README 4 packages and 0 specfiles checked; 0 errors, 2 warnings.
any updates here?
(In reply to comment #22) > Issues: > ======= > [!]: %config files are marked noreplace or the reason is justified. > Note: %config %{_fontconfig_masterdir}/fonts.conf > See: http://fedoraproject.org/wiki/Packaging/Guidelines#Configuration_files I thought it has already been settled with the above comments... if you want to modify, you should use local.conf. > > [!]: Large documentation must go in a -doc subpackage. > Note: Documentation size is 1034240 bytes in 195 files. > See: http://fedoraproject.org/wiki/Packaging/Guidelines#PackageDocumentation > > [!]: Fully versioned dependency in subpackages, if present. > Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in %package > devel will fix the above two. > [!]: I don't think you need to explicitly "Requires: pkgconfig" in spec file Can you give me a pointer where it is mentioned? I can see for BR but R. > [!]: Patches link to upstream bugs/comments/lists or are otherwise justified. No plans to merge this one. it's a kind of hack and no reasonable point that it keeps working forever nor needs to keep applying forever for the issue. that said, I can't remove it because I have no idea how to figure out (before releasing) if the issue is really gone out without it. so I'm just keep applying.
applied the above fixes into git. please review again.
Looks good now :) Thank you for having a look at this review and fixing all the needed issues. And yes about pkgconfig, I found this reference https://fedoraproject.org/w/index.php?title=Packaging%3AReviewGuidelines&diff=160629&oldid=146860 and this also, https://fedoraproject.org/wiki/Archives:PackagingDrafts/PkgconfigAutoRequires APPROVED.
This is completed so lets close this review.