Bug 1141711 - Review Request: fusionforge - collaborative development tool
Summary: Review Request: fusionforge - collaborative development tool
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Lubomir Rintel
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-09-15 09:53 UTC by Sylvain Beucler
Modified: 2014-11-28 02:40 UTC (History)
8 users (show)

Fixed In Version: fusionforge-5.3.2-3.fc21
Clone Of:
Environment:
Last Closed: 2014-11-10 06:48:23 UTC
Type: ---
Embargoed:
lkundrak: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Sylvain Beucler 2014-09-15 09:53:29 UTC
Spec URL: http://fusionforge.fusionforge.org/fedora/fusionforge.spec
SRPM URL: http://fusionforge.fusionforge.org/fedora/fusionforge-5.3.50+201409151132-1.fc20.src.rpm
Description:

Hi,

FusionForge is a extensive collaborative environment for development, used at e.g. https://alioth.debian.org/ , http://pgfoundry.org/ or http://gforge.inria.fr/ .  The official .spec description is:

  FusionForge provides many tools to aid collaboration in a
  development project, such as bug-tracking, task management,
  mailing-lists, SCM repository, forums, support request helper,
  web/FTP hosting, release management, etc. All these services are
  integrated into one web site and managed through a web interface.

The project has historically provided custom RPMs, but after a massive rewrite of the build system during last month, we now have a decent .spec file and hence we wish to submit and maintain it officially at Fedora :)

The packaging is split on multiple packages to ease selecting which feature the user wants to install with its dependencies (Git, SVN, MediaWiki, Mailman...), and also allow split installation on multiple servers (e.g. one for the web front-end, one for the SCM repos).

I'd also like to know how to proceed for co-maintaining the package: I do have a few packages in Fedora already, but I'd like to give privileges to other packagers who don't - do they need to go through a personal validation process and can I give upload privileges directly?

Cheers!

Fedora Account System Username: beuc

Comment 1 Christopher Meng 2014-09-17 05:03:51 UTC
(In reply to Sylvain Beucler from comment #0)
> I'd also like to know how to proceed for co-maintaining the package: I do
> have a few packages in Fedora already, but I'd like to give privileges to
> other packagers who don't - do they need to go through a personal validation
> process and can I give upload privileges directly?

Just do this in new pkgdb by give package.

Comment 2 Lubomir Rintel 2014-09-23 13:08:32 UTC
0.) You seem to be packaging a version that does not exist?

At https://fusionforge.org/frs/?group_id=6 the latest release seems to be 5.3.2. If you're packaging a pre-release version you should have a good reason and use the release tag properly: http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Package_Versioning

Also, the version in %changelog is inconsistent.

1.) You seem to disable the python precompilation:

# Don't compile the .py utils from plugin-moinmoin, the pyc/pyo files are hard to properly package
%global __os_install_post %(echo '%{__os_install_post}' | sed -e 's!/usr/lib[^[:space:]]*/brp-python-bytecompile[[:space:]].*$!!g')

That is ordinarily not a good idea and would need a better explanation.

2.) Your %post scriptlets seem to be somewhat fragile (some, maybe not all)

They seem to be able to fail (e.g. assume a database running, which is definitely not the case during image creation or anaconda installation) therefore are not a good candidates for package scriptlets. You may want to add a documentation file that would explain what to do after package installation instead.

3.) You use line breaks in weird places.

Please format your whitespace consistently and don't do more than two line breaks in sequence.

4.) Your section ordering is rather unusual.

RPM won't care but humans may be confused. The SPEC files usually are ordered like this:

Name: package
%description
%package subpackage1
%description subpackage1
%package subpackage2
%description subpackage2
%prep
%build
%check
%post
%post subpackage1
%post subpackage2
%files
%files subpackage1
%files subpackage2
%changelog

Comment 3 Lubomir Rintel 2014-09-23 13:12:45 UTC
5.) rpmlint is very unhappy.

There are definitely some false positives, but also things that need to be looked into:

fusionforge.noarch: W: summary-not-capitalized C collaborative development tool
fusionforge.noarch: W: incoherent-version-in-changelog 5.3.50 ['5.3.50+201409151132-1.fc22', '5.3.50+201409151132-1']
fusionforge.src: W: invalid-url Source0: http://fusionforge.org/frs/download.php/file/XX/fusionforge-5.3.50+201409151132.tar.bz2 HTTP Error 404: Not Found
fusionforge-common.noarch: W: file-not-utf8 /usr/share/doc/fusionforge-common/README.NSS-pgsql
fusionforge-common.noarch: W: hidden-file-or-dir /usr/share/doc/fusionforge-common/docbook/build/.keepme
fusionforge-common.noarch: E: zero-length /usr/share/doc/fusionforge-common/docbook/build/.keepme
fusionforge-common.noarch: W: dangling-relative-symlink /usr/share/fusionforge/common/include/xhtml10t-rdfa10.dtd ../../www/DTD/xhtml10t-rdfa10.dtd
fusionforge-common.noarch: E: zero-length /usr/share/doc/fusionforge-common/docbook/xsl/include/common_html_chunk.xsl
fusionforge-common.noarch: E: zero-length /usr/share/doc/fusionforge-common/docbook/xsl/include/common_html.xsl
fusionforge-common.noarch: W: one-line-command-in-%post /usr/share/fusionforge/post-install.d/common/common.sh
fusionforge-web.noarch: W: dangling-relative-symlink /usr/share/fusionforge/www/plugins/authbuiltin ../../plugins/authbuiltin/www
fusionforge-web.noarch: W: dangling-relative-symlink /usr/share/fusionforge/www/DTD ../common/DTD

Comment 4 Sylvain Beucler 2014-09-24 15:27:36 UTC
Hi Lubomir!

> 0.) You seem to be packaging a version that does not exist?

Yes, we revamped the whole build system for the next release, and this is why we now have a decent .spec file to propose.

I submitted it for review before the release so we can spot issues ahead :)


> 1.) You seem to disable the python precompilation:
> 
> # Don't compile the .py utils from plugin-moinmoin, the pyc/pyo files are hard to properly package
> %global __os_install_post %(echo '%{__os_install_post}' | sed -e 's!/usr/lib[^[:space:]]*/brp-python-bytecompile[[:space:]].*$!!g')
> 
> That is ordinarily not a good idea and would need a better explanation.

As you saw the core of this packaging relies on 'make install' and doesn't attempt to manually relist all the files from all the different packages and plugins at every release.

Among these files is are 3 MoinMoin glue/config files:
/usr/share/fusionforge/plugins/moinmoin/lib/fusionforge.py
/usr/share/fusionforge/plugins/moinmoin/lib/farmconfig.py
/usr/share/fusionforge/plugins/moinmoin/lib/ff_groups.py

Recent RPM now attempts to compile them automatically, creating new files, and then it complains they not packaged!

I'd like to avoid making exception and relisting explicitely those files, which incidentally are never compiled in any other FusionForge install or packaging, hence why I disabled the Python bytecode compilation for these 3 files.

I would welcome a generic approach to tackle this issue.


> 2.) Your %post scriptlets seem to be somewhat fragile (some, maybe not all)

Indeed the installation requires a working database connection.
I'll add a README to explain how to relaunch /usr/share/fusionforge/post-install.d/xxx if there's a problem during install.


> 3.) You use line breaks in weird places.

You mean I sometimes use "\n\n"?  This clearly separates the different sub-packages (since there is an empty line in the description, one line only is not enough for my brain).


> 4.) Your section ordering is rather unusual.

The plugins sections gets automatically included in the .spec file, see rpm/gen_spec.sh.
It will be pretty complex and far less readable to insert all the plugins bits everywhere through the .spec file, rather than in a single bloc at the end.
(also I think it suits this package better :))


> 5.) rpmlint is very unhappy.

I had tried on my stable F20 but apparently quite a new tests were added.
What version are you using?

> fusionforge-web.noarch: W: dangling-relative-symlink /usr/share/fusionforge/www/plugins/authbuiltin ../../plugins/authbuiltin/www
> fusionforge-web.noarch: W: dangling-relative-symlink /usr/share/fusionforge/www/DTD ../common/DTD

The targets are in fusionforge-common, which the package depends on.

I'll prepare a new srpm to fix the other warnings.

Comment 6 Sylvain Beucler 2014-09-26 14:17:40 UTC
I had forgotten the README about relaunch the post-install scripts - sorry about that.  It's in now.

Spec URL: http://fusionforge.fusionforge.org/fedora/fusionforge.spec
SRPM URL: http://fusionforge.fusionforge.org/fedora/fusionforge-5.3.50+201409261609-1.fc20.src.rpm

Comment 7 Sylvain Beucler 2014-09-29 09:30:02 UTC
Ping? :)

Comment 8 Sylvain Beucler 2014-10-01 16:11:32 UTC
From IRC conversation today, I understand that there are 2 issues remaining:

- Don't disable Python byte-compilation

  I manually referenced the wrapper/conf dir with the .py files, so now the .pyc/.poc are generated *and* rpm doesn't complain about unpackaged files.

- Mark the packages are being a pre-release

  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.

Spec URL: http://fusionforge.fusionforge.org/fedora/fusionforge.spec
SRPM URL: http://fusionforge.fusionforge.org/fedora/fusionforge-5.3.50+201410011756-1.fc20.src.rpm

Comment 9 Lubomir Rintel 2014-10-08 15:56:23 UTC
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.

Comment 10 Thorsten Glaser 2014-10-08 16:33:47 UTC
(In reply to Lubomir Rintel from comment #9)

> Some tips:
> * Do _NOT_ assume database is running
> * Do not fail under any circumstances

These two *directly* contradict each other.

Either a working PostgreSQL instance is available for installation,
or the installation MUST fail.

Comment 11 Robert Scheck 2014-10-08 16:39:52 UTC
(In reply to Thorsten Glaser from comment #10)
> Either a working PostgreSQL instance is available for installation,
> or the installation MUST fail.

I think Lubomir means that the %post scriptlet is not allowed to exit with
anything else except 0. So if needed, write e.g. a small log file for later 
examination - given that any scriptlet should not cause output to STDOUT/
STDERR.

Comment 12 Lubomir Rintel 2014-10-08 16:57:27 UTC
(In reply to Thorsten Glaser from comment #10)
> (In reply to Lubomir Rintel from comment #9)
> 
> > Some tips:
> > * Do _NOT_ assume database is running
> > * Do not fail under any circumstances
> 
> These two *directly* contradict each other.
> 
> Either a working PostgreSQL instance is available for installation,
> or the installation MUST fail.

They don't if you don't attempt to connect to the database upon package installation in the first place.

The %post is possibly the most wrong place you can attempt to do that kind of stuff -- you can't be sure about anything running, you can't do anything about that and you can't abort the package installation. Returning a non-zero status means you tried to do that -- a packaging bug.

If a package needs a running system to set it up it needs to be done _after_ the package is installed. Adding a README with instructions for the operator is one estabilished pracitce for that. Doing that from withing a service during system startup is another one (and systemd makes it rather convenient too).

Comment 13 Sylvain Beucler 2014-10-09 14:13:55 UTC
(In reply to Lubomir Rintel from comment #9)
> 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.

Done.

> 1.) Your %descriptions for subpackage do not make much sense
> 
> They're all the same. Please describe what the package is for.

They begin with a general stanza about FusionForge but continue with a description of the sub-package.

> 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

"CAS authentication" is an existing gettext string that is directly extracted from the plugin.

I changed to "FusionForge plugin - CAS authentication" to be able to reuse translated strings.

> 2.) Please try to keep the whitespace less crazy
> 
> Please crop the four line breaks above %changelog to two.

Done.

> 3.) Your scriptlets seem to do insane things

Following the discussion above we can conclude that post-install scripts need to be removed.  README.Fedora still explains what to do after install.

Done.

Please let me know directly when a fix is incomplete - this issue was adressed 2 weeks ago (comment #4) by adding documentation, and we only knew that this eventually wasn't enough yesterday.

> 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

For the record, the upstream installation script didn't use "useradd -M" which forces disabling homedir creation, which itself is normally disabled by default already but enabled in Fedora's /etc/login.defs.

> 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

3.x comments obsoleted by post-install scripts removal.

> 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]# 

Currently install/uninstall is clean, given that no post-install script is run.
Most of these files are generated passwords, potentially customized configuration files and user-uploaded content, so if we also need an unconditionnal "rm -rf /etc/fusionforge /usr/share/fusionforge /var/lib/fusionforge" on uninstall, no problem with removing critical user data without notice, but I'll need to see the official guideline about it.

> (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:

I now disconnected from IRC to keep the conversation here.

> 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.

Done.

http://fusionforge.fusionforge.org/fedora/fusionforge-5.3.50+201410091603-1.fc20.src.rpm
http://fusionforge.fusionforge.org/fedora/fusionforge.spec

Comment 14 Lubomir Rintel 2014-10-11 07:37:06 UTC
* Package named correctly
* Correct version tag
* License tag mostly fine (see below)
* License OK for Fedora
* SPEC file clean and legible
* Builds fine in mock
* RPMlint reasonably happy
* Filelists sane
* Dependency chain sane

0.) You seem to bundle jQuery

Bundling is generally no-go, but this is already done by many packages, with some plan to move forward but no appropriate solution at present:

http://fedoraproject.org/wiki/Changes/jQuery

Therefore I'd say this one is good to go, but please change your License tag stating that you also ship MIT-licensed code (and add a comment above the License tag explaining which license is for jQuery and which for FusionForge code).

I'm not holding the review back for this; please do that upon import.

The package is APPROVED

Comment 15 Lubomir Rintel 2014-10-11 07:42:52 UTC
Whoops, sorry; cancelling the approval for now; there's more bundled code it seems:

Please use distribution php-nusoap and php-simplepie and figure out what do about the rest: http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries

I believe most can be packaged separately. As for the JQuery stuff I guess it can get a FESCo exception (see above) until the jQuery packaging gets sorted out.

Comment 16 Sylvain Beucler 2014-10-11 21:18:11 UTC
I'm out of town for the commit week so this will have to wait.

Comment 17 Sylvain Beucler 2014-10-24 13:02:25 UTC
For the dependencies, fusionforge-web now depends on the php-simplepie and php-nusoap packages.

The 'php-snoopy' code was replaced by php's curl.

I'm not sure if you request that all libraries we ship should be packaged even if there's no matching fedora package. In particular, Graphite/Arc, which handle RDF parsing, are shipped but not packaged anywhere AFAICS, and not active, so there's no value in packaging them for Fedora. If you insist, we'll just remove the plugins that depend on these from the packaging.

http://fusionforge.fusionforge.org/fedora/fusionforge-5.3.50+201410241500-1.fc20.src.rpm
http://fusionforge.fusionforge.org/fedora/fusionforge.spec

Comment 18 Lubomir Rintel 2014-10-24 13:38:35 UTC
(In reply to Sylvain Beucler from comment #17)
> For the dependencies, fusionforge-web now depends on the php-simplepie and
> php-nusoap packages.
> 
> The 'php-snoopy' code was replaced by php's curl.
> 
> I'm not sure if you request that all libraries we ship should be packaged
> even if there's no matching fedora package. In particular, Graphite/Arc,
> which handle RDF parsing, are shipped but not packaged anywhere AFAICS, and
> not active, so there's no value in packaging them for Fedora. If you insist,
> we'll just remove the plugins that depend on these from the packaging.

Well, I don't insist on a particular solution; any is fine with me as long as it's fine with the guidelines. Thus I believe it's one of:

1.) Packaging the libraries separately
2.) Dropping them (and stuff that depends on those)
3.) Getting an exception from FESCO

Comment 19 Sylvain Beucler 2014-10-24 13:55:08 UTC
I'm going to remove the RDF plugins from the Fedora packaging.
Is the packaging otherwise alright, so we avoid another round-trip?

Comment 20 Lubomir Rintel 2014-10-24 13:57:51 UTC
Yep, packaging is all fine.

Comment 22 Rex Dieter 2014-10-24 15:42:14 UTC
That Version: tag doesn't seem to comply with
https://fedoraproject.org/wiki/Packaging:NamingGuidelines

which is pretty clear that Version: tag should not contain non-numeric items (like a '+')

If you want to include somewhere 201410241700 , you can put it into the Release: tag, using something like:
Version: 5.3.50
Release: 1.201410241700%{?dist}
instead.

Comment 23 Sylvain Beucler 2014-10-24 15:58:34 UTC
The upstream version I packaged is 5.3.50+201410241700:
http://fusionforge.fusionforge.org/snapshots/fusionforge-5.3.50+201410241700.tar.bz2

Comment 24 Rex Dieter 2014-10-24 16:03:53 UTC
So?  That Version includes a non-numeric character, which is something not allowed.  Per the guideline I referenced,
"If the version is non-numeric (contains tags that are not numbers), you may need to include the additional non-numeric characters in the release field."



and, since I'm looking at it, I can be picky... :)

subpkg deps of form:
Requires: %{name}-... = %{version}
should include %{release} too,
Requires: %{name}-... = %{version}-%{release}

See,
https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

Comment 25 Rex Dieter 2014-10-24 16:08:06 UTC
Per Version: , I guess this option may also be strictly acceptable (omits, the '+'),

Version:  5.3.50.201410241700

But I think it follows at least the spirit of the NamingGuidelines to put it into the Release tag (since it resembles a date/snapshot).   Or I suppose you could omit it from being visible in Version/Release tags altogether.  whatever you prefer.

Comment 27 Lubomir Rintel 2014-10-29 08:39:32 UTC
APPROVED

Comment 28 Sylvain Beucler 2014-10-29 10:21:48 UTC
Thanks!

New Package SCM Request
=======================
Package Name: fusionforge
Short Description: Collaborative development tool
Upstream URL: http://fusionforge.org/
Owners: beuc
Branches: f21
InitialCC: beuc

Comment 29 Gwyn Ciesla 2014-10-29 11:45:54 UTC
Git done (by process-git-requests).

Comment 30 Fedora Update System 2014-11-03 16:49:56 UTC
fusionforge-5.3.2-3.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/fusionforge-5.3.2-3.fc21

Comment 31 Fedora Update System 2014-11-03 19:40:00 UTC
fusionforge-5.3.2-3.fc21 has been pushed to the Fedora 21 testing repository.

Comment 32 Fedora Update System 2014-11-10 06:48:23 UTC
fusionforge-5.3.2-3.fc21 has been pushed to the Fedora 21 stable repository.


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