[Bug 1141711] Review Request: fusionforge - collaborative development tool

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1141711

Lubomir Rintel <lrintel@xxxxxxxxxx> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |lrintel@xxxxxxxxxx



--- Comment #9 from Lubomir Rintel <lrintel@xxxxxxxxxx> ---
Sorry it took so long and thanks for the patience.

Reviewing a package of this complexity requires some extra setup, especially as
you seem to do very dangerous things from the scriptlets.

So, here we go; it still needs some amount of work:

0.) % characters in comments:

# Marking /etc as conffiles and exclude locales (cf. mandatory %find_lang)
# Not using recursive dirs listing because that is processed when all
#   packages are mixed in the common install dir (so using %dir instead)

Please escape them: s/%/%%/. Otherwise RPM attempts to expand them.

1.) Your %descriptions for subpackage do not make much sense

They're all the same. Please describe what the package is for.
The summaries can be improved a bit too. E.g.:

Summary: Collaborative development tool - CAS authentication

Can be better written as:

Summary: CAS authentication plugin for Fusionforge

2.) Please try to keep the whitespace less crazy

Please crop the four line breaks above %changelog to two.

3.) Your scriptlets seem to do insane things

Some tips:
* Do _NOT_ assume database is running
* Do not fail under any circumstances
* Do not spew any output
To verify, try to use install your packages from a kickstart -- do an
unattended installation or run livecd-creator and ensure your log is empty.

3.0.) Do not attempt to create directories that you already ship in the
package.

  Installing  : fusionforge-common-5.3.50+201410011756-1.fc20.noarch           
                                                                               
                                            77/136
useradd: warning: the home directory already exists.
Not copying any file from skel directory into it.

3.1.) Do not enable or disable sevices

  Installing  : fusionforge-db-local-5.3.50+201410011756-1.fc20.noarch         
                                                                               
                                            99/136 Note: Forwarding request to
'systemctl enable postgresql.service'.
ln -s '/usr/lib/systemd/system/postgresql.service'
'/etc/systemd/system/multi-user.target.wants/postgresql.service'
...
Note: Forwarding request to 'systemctl enable httpd.service'.
...

That is system operator's duty. With systemd, you may want to create a target
that depends on services you need.

3.2.) Avoid debugging output

  Installing  : fusionforge-db-local-5.3.50+201410011756-1.fc20.noarch         
                                                                               
                                            99/136
...
Importing initial database...
Running script: 20100308-drop-forum-attachment-type.sql
/usr/share/fusionforge/db/20100308-drop-forum-attachment-type.sql ran correctly

  Installing  : fusionforge-plugin-scmsvn-5.3.50+201410011756-1.fc20.noarch    
                                                                               
                                           132/136
Running /usr/share/fusionforge/plugins/scmsvn/bin/install.sh configure
Modifying inetd for Subversion server
TODO: xinetd support

Direct it to a log if needed.

3.3.) Note that there might not be enough entropy available

Generating RSA private key, 1024 bit long modulus
.++++++
.........++++++
e is 65537 (0x10001)

Ensure this can not lock up system forever. It's probably better done in a
separate service, on startup and asynchronously. See what sshd does.

3.4.) Use systemd integration properly

  Installing  : fusionforge-plugin-admssw-5.3.50+201410011756-1.fc20.noarch    
                                                                               
                                           102/136
Redirecting to /bin/systemctl reload  httpd.service

If you need to refresh a service, do not call /sbin/service. See:
https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd

3.5.) Do not assume a database is running
  Installing  : fusionforge-shell-5.3.50+201410011756-1.fc20.noarch            
                                                                               
                                           109/136
...
Could not connect to database

Scriptlet might be running from a kickstart or during system installation.

3.6.) Do not .... do this:

  Installing  : fusionforge-shell-5.3.50+201410011756-1.fc20.noarch            
                                                                               
                                           109/136
...
useradd: cannot create directory /nonexistent

The nonexistent directory is supposedly nonexistent. Also, it makes the
security policy angry:

type=AVC msg=audit(1412781994.848:906): avc:  denied  { create } for  pid=28546
comm="useradd" name="nonexistent"
scontext=unconfined_u:system_r:useradd_t:s0-s0:c0.c1023
tcontext=system_u:object_r:etc_runtime_t:s0 tclass=dir permissive=0

3.6.) Never let the scriptlets fail:

  Installing  : fusionforge-shell-5.3.50+201410011756-1.fc20.noarch            
                                                                               
                                           109/136
...
warning: %post(fusionforge-shell-5.3.50+201410011756-1.fc20.noarch) scriptlet
failed, exit status 12
non-fatal postin scriptlet failure in rpm package fusionforge-shell

even worse, they fail during uninstall (always a bug):

useradd: cannot create directory /nonexistent
error: %preun(fusionforge-shell-5.3.50+201410011756-1.fc20.noarch) scriptlet
failed, exit status 12

4.) Don't leave garbage in after uninstall:

[root@belphegor SPECS]# find /etc/fusionforge /usr/share/fusionforge
/var/lib/fusionforge |xargs rpm -qf 
file /etc/fusionforge is not owned by any package
file /etc/fusionforge/ssl-cert.pem is not owned by any package
file /etc/fusionforge/config.ini.d is not owned by any package
file /etc/fusionforge/config.ini.d/post-install.ini is not owned by any package
file /etc/fusionforge/config.ini.d/sysauthldap-secrets.ini is not owned by any
package
file /etc/fusionforge/config.ini.d/post-install-secrets-ssh_akc.ini is not
owned by any package
file /etc/fusionforge/config.ini.d/post-install-secrets.ini is not owned by any
package
file /etc/fusionforge/ssl-cert.key is not owned by any package
file /etc/fusionforge/httpd.conf.d is not owned by any package
file /etc/fusionforge/httpd.conf.d/vhost-list.inc is not owned by any package
file /etc/fusionforge/httpd.conf.d/auth-main.inc is not owned by any package
file /etc/fusionforge/httpd.conf.d/plugin-scmsvn.inc is not owned by any
package
file /etc/fusionforge/httpd.conf.d/post-install-secrets.inc is not owned by any
package
file /etc/fusionforge/httpd.conf.d/plugin-moinmoin.inc is not owned by any
package
file /etc/fusionforge/httpd.conf.d/vhost-main.inc is not owned by any package
file /etc/fusionforge/httpd.conf.d/ssl-really-on.inc is not owned by any
package
file /etc/fusionforge/httpd.conf.d/block-trace.inc is not owned by any package
file /etc/fusionforge/httpd.conf.d/04-config-vendor.conf is not owned by any
package
file /etc/fusionforge/httpd.conf.d/plugin-generic.inc is not owned by any
package
file /etc/fusionforge/httpd.conf.d/plugin-scmbzr.inc is not owned by any
package
file /etc/fusionforge/httpd.conf.d/plugin-ckeditor.inc is not owned by any
package
file /etc/fusionforge/httpd.conf.d/40-vhosts-extra.conf is not owned by any
package
file /etc/fusionforge/httpd.conf.d/auth-projects.inc is not owned by any
package
file /etc/fusionforge/httpd.conf.d/ssl-off.inc is not owned by any package
file /etc/fusionforge/httpd.conf.d/plugin-scmdarcs.inc is not owned by any
package
file /etc/fusionforge/httpd.conf.d/20-vhosts-lists.conf is not owned by any
package
file /etc/fusionforge/httpd.conf.d/50-wsgi-scmbzr.conf is not owned by any
package
file /etc/fusionforge/httpd.conf.d/01-namevhost.conf is not owned by any
package
file /etc/fusionforge/httpd.conf.d/projects-in-mainvhost.inc is not owned by
any package
file /etc/fusionforge/httpd.conf.d/50-vhosts-scm.conf is not owned by any
package
file /etc/fusionforge/httpd.conf.d/30-vhosts-projects.conf is not owned by any
package
file /etc/fusionforge/httpd.conf.d/plugin-scmgit.inc is not owned by any
package
file /etc/fusionforge/httpd.conf.d/10-vhosts-main.conf is not owned by any
package
file /etc/fusionforge/httpd.conf.d/plugin-sysauthldap-secrets.inc is not owned
by any package
file /etc/fusionforge/httpd.conf.d/50-wsgi-moinmoin.conf is not owned by any
package
file /etc/fusionforge/httpd.conf.d/vhost-projects.inc is not owned by any
package
file /etc/fusionforge/httpd.conf.d/02-config-main.conf is not owned by any
package
file /etc/fusionforge/httpd.conf.d/plugin-mediawiki.inc is not owned by any
package
file /etc/fusionforge/httpd.conf.d/httpd.vhosts is not owned by any package
file /etc/fusionforge/httpd.conf.d/log.inc is not owned by any package
file /etc/fusionforge/httpd.conf.d/ssl-on.inc is not owned by any package
file /etc/fusionforge/httpd.conf.d/vhost-scm.inc is not owned by any package
file /etc/fusionforge/httpd.conf.d/plugin-authhttpd.inc is not owned by any
package
file /etc/fusionforge/httpd.conf is not owned by any package
file /usr/share/fusionforge is not owned by any package
file /usr/share/fusionforge/www is not owned by any package
file /usr/share/fusionforge/www/plugins is not owned by any package
file /usr/share/fusionforge/www/plugins/authhttpd is not owned by any package
file /usr/share/fusionforge/www/plugins/projectlabels is not owned by any
package
file /usr/share/fusionforge/www/plugins/message is not owned by any package
file /usr/share/fusionforge/www/plugins/extsubproj is not owned by any package
file /usr/share/fusionforge/www/plugins/compactpreview is not owned by any
package
file /usr/share/fusionforge/www/plugins/headermenu is not owned by any package
file /usr/share/fusionforge/www/plugins/mediawiki is not owned by any package
file /usr/share/fusionforge/www/plugins/admssw is not owned by any package
file /usr/share/fusionforge/www/plugins/globalsearch is not owned by any
package
file /usr/share/fusionforge/www/plugins/webanalytics is not owned by any
package
file /usr/share/fusionforge/www/plugins/blocks is not owned by any package
file /usr/share/fusionforge/www/plugins/contribtracker is not owned by any
package
file /usr/share/fusionforge/www/plugins/scmgit is not owned by any package
file /usr/share/fusionforge/www/plugins/scmhook is not owned by any package
file /usr/share/fusionforge/www/plugins/moinmoin is not owned by any package
file /usr/share/fusionforge/www/plugins/online_help is not owned by any package
file /usr/share/fusionforge/www/plugins/authldap is not owned by any package
file /usr/share/fusionforge/www/plugins/hudson is not owned by any package
file /usr/share/fusionforge/www/plugins/authcas is not owned by any package
file /var/lib/fusionforge is not owned by any package
file /var/lib/fusionforge/etc is not owned by any package
file /var/lib/fusionforge/etc/postfix-transport.db is not owned by any package
[root@belphegor SPECS]# 

(In reply to Sylvain Beucler from comment #8)

>   Here I rely on the upstream snapshoting system which produces independent
> tarballs with versions such as 5.3.50+201410011756, and I don't intend to
> upload a pre-release to Fedora.  If you want to witnessing a modified .spec
> from me with a -1.20141001git in the Release field, let me know.

We're reviewing exactly what goes into Fedora and that has to be aligned with
guidelines be it a snapshot or not.

As I've stated on IRC: Everyone neds to be able to get the very same tarball as
you've used. So either include a proper _public_ link or a comment on how is
the tarball created from the VCS snapshot. And always either use an official
release version number or a proper pre-release tag. "5.3.50+201410011756" does
not seem to exist publicly. Your link is dead. On the other hand,
"-1.20141001git" looks like a proper pre-release tag.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review





[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]