Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 226431 - Merge Review: squid
Summary: Merge Review: squid
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Adam Tkac
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On: 198251
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-31 21:01 UTC by Nobody's working on this, feel free to take it
Modified: 2013-04-30 23:35 UTC (History)
6 users (show)

Fixed In Version: squid-3.1.0.17-3.fc14
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-03-02 14:39:31 UTC
atkac: fedora-review+


Attachments (Terms of Use)

Description Nobody's working on this, feel free to take it 2007-01-31 21:01:33 UTC
Fedora Merge Review: squid

http://cvs.fedora.redhat.com/viewcvs/devel/squid/
Initial Owner: mbacovsk@redhat.com

Comment 1 Rex Dieter 2007-02-16 15:10:58 UTC
MUST blocker (imo, anyway), 
samba RFE: "samba: make /var/cache/samba/windind_privledged group owned", bug
#198251 (and related squid bug #198253),
changing owner/group/permissions on a file/dir owned by another pkg is
unacceptable (ie, squid's existing 
%triggerin -- samba-common
/usr/sbin/usermod -a -G winbind squid >/dev/null 2>&1 || \
    chgrp squid /var/cache/samba/winbindd_privileged >/dev/null 2>&1 || :



Comment 2 Dmitry Butskoy 2007-03-26 13:49:29 UTC
Since samba has changed the group name to "wbpriv", and already accepted the
idea of such a group, the triggerin script should be now:

%triggerin -- samba-common
/usr/sbin/usermod -a -G wbpriv squid >/dev/null 2>&1 || :


Please, remove "chgrp" fall-back, it is no more needed and too hackish...

Comment 3 Dmitry Butskoy 2007-04-28 13:35:56 UTC
Again,
Remove "chgrp" fall-back in %triggerin, it is not needed. Moreover, it uses
/var/cache/samba, whereas samba do not use such a dir anymore now.

Comment 4 Adam Tkac 2009-12-15 15:40:59 UTC
Formal review of squid-3.1.0.15-2.fc13:

"+" means OK, "-" means not OK

+ MUST: The package must be named according to the Package Naming Guidelines
+ MUST: The spec file name must match the base package %{name}, in the format %{name}.spec
- MUST(1): The package must meet the Packaging Guidelines
+ MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .
- MUST(2): The License field in spec match the actual license
- MUST(3): If (and only if) the source package includes the text of the license(s) in its own file, then that file must be included in %doc
+ MUST: The spec file written in American English
+ MUST: The spec file for the package is legible
+ MUST: The sources used to build the package must match the upstream source, as provided in the spec URL
+ MUST: The package successfully compile
+ MUST: All build dependencies must be listed in BuildRequires
- MUST(4): The spec file handle locales properly
+ MUST: Every package which stores shared library files in any of the dynamic linker's default paths, must call ldconfig in %post and %postun
+ MUST: Packages does not bundle copies of system libraries
+ MUST: Package own all directories that it creates
+ MUST: Package does not list a file more than once in the spec file
+ MUST: Permissions on files must be set properly. Every %files section must include a %defattr(...) line
+ MUST: Package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT)
- MUST(5): Package use macros consistently
+ MUST: Package contains code, or permissable content
+ MUST: Large documentation files must go in a -doc subpackage
+ MUST: If a package includes something as %doc, it must not affect the runtime of the application
+ MUST: Header files in a -devel package
+ MUST: Static libraries in a -static package
+ MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'
+ MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package
+ MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}
+ MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built
+ MUST: Packages containing GUI applications must include a %{name}.desktop file
+ MUST: Packages must not own files or directories already owned by other packages
+ MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT)
+ MUST: All filenames in rpm packages must be valid UTF-8.

1: Use versioned Sources, please (s/Source/Source0/)
2: It seems package is distributed under GPLv2 only, not GPLv2+
3: Include COPYING and COPYRIGHT files in %doc, please
4: use %find_lang macro, please. Check
https://fedoraproject.org/wiki/Packaging:Guidelines#Handling_Locale_Files for
more information
5: Use $RPM_OPT_FLAGS macro instead of %{optflags}


Other:
- please remove Source1 (the .asc signature). I don't see any reason to include
  it in the package
- don't use -fPIE flag on architectures where -fpie is sufficient because -fpie
  generates faster code. I suggest to use this in the specfile:

%ifarch sparcv9 sparc64 s390 s390x
export CXXFLAGS="$RPM_OPT_FLAGS -fPIE"
export CFLAGS="$RPM_OPT_FLAGS -fPIE"
%else
export CXXFLAGS="$RPM_OPT_FLAGS -fpie"
export CFLAGS="$RPM_OPT_FLAGS -fpie"
%endif

export LDFLAGS="-pie"

- I recommend to drop -Os, -g, -pipe and -fsigned-char parameters
- don't export CFLAGS, CXXFLAGS and LDFLAGS twice, the first export is
  sufficient
- please use macros instead of hardcoded paths:
  - use %{_sysconfdir} instead of /etc
  - use %{_datadir} instead of /usr/share
- fix (or explain) all rpmlint warnings
- consider to add LSB header to initscript (not required, check
  https://fedoraproject.org/wiki/Packaging:SysVInitScript#LSB_Header)
- remove the %triggerin as written in comments #1, #2 and #3

Comment 5 Jiri Skala 2010-02-03 13:13:47 UTC
squid.x86_64: W: name-repeated-in-summary Squid
COMMENT: intention

squid.x86_64: W: conffile-without-noreplace-flag /etc/squid/errorpage.css.default
squid.x86_64: W: conffile-without-noreplace-flag /etc/squid/mime.conf.default
squid.x86_64: W: conffile-without-noreplace-flag /etc/squid/squid.conf.default
squid.x86_64: W: conffile-without-noreplace-flag /etc/squid/cachemgr.conf.default
squid.x86_64: W: conffile-without-noreplace-flag /etc/squid/msntauth.conf.default
COMMENT: sample files don't use no-replace

squid.x86_64: E: zero-length /usr/share/squid/errors/ca/ERR_CACHE_ACCESS_DENIED
COMMENT: tarball issue - complained to upstream

squid.x86_64: E: non-readable /etc/squid/squid.conf 0640
squid.x86_64: E: non-standard-dir-perm /var/spool/squid 0750
squid.x86_64: E: non-standard-dir-perm /var/log/squid 0750
COMMENT: intention - no access for others

squid.x86_64: W: dangerous-command-in-%pre chown
squid.x86_64: W: dangerous-command-in-%preun rm
COMMENT: correct-intended

squid.x86_64: W: missing-lsb-keyword Required-Start in /etc/rc.d/init.d/squid
squid.x86_64: W: missing-lsb-keyword Required-Stop in /etc/rc.d/init.d/squid
squid.x86_64: W: missing-lsb-keyword Default-Stop in /etc/rc.d/init.d/squid
COMMENT: no intention to define (check for chkconfig tag in initscript)

squid-debuginfo.x86_64: W: hidden-file-or-dir /usr/src/debug/squid-3.1.0.16/src/.libs
squid-debuginfo.x86_64: W: hidden-file-or-dir /usr/src/debug/squid-3.1.0.16/src/.libs
COMMENT: strange behaviour somewhere between find-debuginfo.sh and rpmlint. probably filing a bug against find-debugingo.sh

Comment 6 Adam Tkac 2010-03-02 14:39:31 UTC
I checked squid-3.1.0.17-3.fc14 and it looks fine for me, approved.

Comment 7 Adam Tkac 2010-03-02 14:41:56 UTC
(In reply to comment #6)
> I checked squid-3.1.0.17-3.fc14 and it looks fine for me, approved.    

There is only one non-blocking issue:
squid.spec:137: W: mixed-use-of-spaces-and-tabs (spaces: line 5, tab: line 137)

Please correct it in the next commit.


Note You need to log in before you can comment on or make changes to this bug.