-Provide full path to source
-Don't use Requires(hint)
-Inconsitant use of $RPM_BUILD_ROOT (also used as ${RPM_BUILD_ROOT})
-defattr in %files is way off.  You'll end up with dir's that aren't executable.
 put one %defattr(-,root,root,-) right under %files, get rid of the two that are
there.
-empty %build section (remove it)
-Don't use hard coded paths, use macros:
http://fedoraproject.org/wiki/Extras/RPMMacros
-Group Applicaton/System is not a valid Fedora group
-Put a version/release tag in your change logs
-If this is a gallery2-minimal tarbal why not make a gallery2-minimal rpm?
-Are you sponsored?
-Please download rpmlint and run it against the rpm you generate
-Take a look at: http://fedoraproject.org/wiki/Packaging/ReviewGuidelines It'll
help if you know all the criteria that we review on.

Clarification requested:
1. Full path to source - should this be in the URL tag or the Source0 tag - and
should I refer to the Gallery home page or to the Sourceforge downloadable tarball?

2. How do I specify that the RPM requires (MySQL or PostgreSQL) as opposed to
needing both?  The only documentation I could find involved using Requires(hint).

3. Should the installation path macro be defined in the specfile itself or in
~/.rpmmacros ?

4. My intent in using the gallery-minimal tarall was to package just the base
set of files, creating add-on packages for additional themes / modules that
would otherwise be in upstream's -typical, -full, or -developer tarball
distributions.  This would allow for removal of optional themes/modules normally
contained in upstream's -typical -full and -developer packages without causing
rpm -V to complain.

5. I am currently not sponsored; this is the first Fedora package I've attempted.

"URL" tag for the web page, "Source" for the tarball download location.

> %defattr(-,root,root-)

Typo. There is no system group "root-". You want "root".

> %{installprefix}/gallery2/*

With that, directory "gallery2" is not included. To include the directory
and everything in it recursively, use just:

  %{installprefix}/gallery2/

> I made this package relocatable, as it is intended upstream to
> be installable to a ~/public_html directory as well as /var/www/html,

Questionable. The package has several dependencies. A normal user with
an own RPM database could not simply install the package without using
the --nodeps option.

> %install
> [ "${RPM_BUILD_ROOT}" != "/" ] && rm -rf ${RPM_BUILD_ROOT}

This check doesn't make much sense. You provide a default build root at
the top. There is no evidence that anybody ever builds rpms as superuser
and overrides the build root at the same time.

> GraphicsMagick>=1

Is this available in Core or Extras?

Source tag now contains the full link to the sourceforge download tarball, with
the caveat that sourceforge will ask you what mirror you want to use.

%defattr typo has been fixed, removed trailing * from %{installprefix}/gallery2/

${RPM_BUILD_ROOT} check removed from %install section

GraphicsMagick package removed from Requires - not provided in Core or Extras -
it was mentioned on the Gallery home page.

SRPM and spec file have been updated.

(In reply to comment #4)
> Source tag now contains the full link to the sourceforge download tarball, with
> the caveat that sourceforge will ask you what mirror you want to use.

If you change Source0: to:
http://dl.sf.net/gallery/gallery-%{version}-minimal.tar.gz
then it will work properly, without the mirror selection page.

Since you're making the package relocatable by including a Prefix: tag, you
should add comments in the spec to justify this (relocatable packages are not
the norm in Extras).

Comment added, hostname for URL in Source0 tag changed, SRPM/spec file updated.

Not a review yet, but some comments: 

- Make sure to increment your release each time you make changes, even in this
pre-accepted state. It helps keep track of what issues were fixed and what
changes were made. 

- Removing the "Requires(hint)" is good (it's not supported by any shipping
Fedora rpm (AFAICT). 

- Not sure what the best solution is to requiring a database backend. Since you
must manually setup the database in any case I would think just noting it in the
description as you have should be enough. Anyone else have a more clever solution? 

(In reply to comment #7)
> 
> - Not sure what the best solution is to requiring a database backend. Since you
> must manually setup the database in any case I would think just noting it in the
> description as you have should be enough. Anyone else have a more clever
solution? 

You could have a "Requires: gallery-database" and then have gallery-mysql and
gallery-postgresql submodules (possibly empty, except for maybe some READMEs)
that both have "Provide: gallery-database".  The submodules would then require
php-mysql or pgp-pgsql, which is a more appropriate requirement anyway.

Maybe do not require a database at all, because the user must configure it
anyway after installation?  This avoids some needless complication with little
benefit of having dependencies that may not actually be needed.  MySQL and
PostgreSQL are not the only options.

NEEDSWORK

- Relocatable in this RPM has absolutely no value, because normal users don't
have a RPM database.  Am I incorrect?
- Please add "-q" to the %setup line.
- You simply wildcard everything in the %files section.  Isn't part of
installing this usually modifying one of those files to configure the database
and other stuff?  Your package will overwrite and blow away manual changes upon
every upgrade.

There may be other problems... I don't know yet because I haven't tried the
software itself.

- http://codex.gallery2.org/index.php/Downloads:GHCC
Including this in the package might be useful, because default settings of PHP
and the environment are not compatible with gallery2.
- You should probably change the package Name to "gallery2" because some people
might still be using gallery as the 1.x version, and they are very incompatible
with each other.

Fixes:
- Removed package relocatability
- Added -q to %setup
- Added ghosted config files.  These files are actually created at setup time,
after installation (which simply consists of unrolling the tarball in place),
they are not present in the download tarball at all.

Issues:
- GHCC is a compatibility checker for Gallery 1.x, not 2.x.  Gallery2 includes
compatibility checking as part of the initial setup process, so I'm not sure
there's any value to adding GHCC to this package
- I've changed the name to gallery2, but I don't think there would be a major
compatibility problem as G2 installs to a different directory, and can
transparently import G1 albums into G2.  I'd prefer the name be 'gallery' but
I'm not adamant about that.

New spec file / srpm locations:
SPEC:  http://www.berningeronline.net/gallery2.spec
SRPM:  http://www.berningeronline.net/gallery2-2.0.2-2.src.rpm

Rebuild failed due to not creating ghosted config files.  Error fixed in updated
packages:

SPEC:  http://www.berningeronline.net/gallery2.spec
SRPM:  http://www.berningeronline.net/gallery2-2.0.2-3.src.rpm

* %ghost and %config are mutually exclusive.

* Verify your package changes: rpm --query --configfiles gallery2

* Are you happy with the results?
  See: rpm --query --list gallery2-2.0.2-3.i386.rpm | grep htaccess

* Also take a look at your build log:

warning: File listed twice: /var/www/html/gallery2/.htaccess
warning: File listed twice: /var/www/html/gallery2/config.php
warning: File listed twice: /var/www/html/gallery2/login.txt

Removed %config from %ghost files - The queries on rpm -ql and rpm -qf are
behaving as I intended now.  This also removes the "File listed twice" warnings.

I've also split off the docs/ directory into a -docs package, since it seems to
belong there rather than in the main package.

New files:
SPEC:  http://www.berningeronline.net/gallery2.spec
SRPM:  http://www.berningeronline.net/gallery2-2.0.2-4.src.rpm

Opinions requested: should the imagemagick, gd, netpbm, matrix, and siriux
modules/themes also be broken out into separate packages, or simply left as
separate 'provides' as they are now?  Without at least one of each, the gallery
will be fairly useless, but it's also useless without a database or image
library backend.  Breaking it out will allow me to require ImageMagick for the
gallery2-imagemagick package, etc., helping to ensure the image library
requirements are actually met.

The FE packaging guidelines suggest using "Documentation" as the Group for a
separate -doc package.

However, given that the gallery2-docs package only contains 2 files, I'm not
sure it's really worth splitting into a separate package.

Realignment of package:
- fold -docs package back into main package.
- use -developer tarball from upstream
- split out all modules/themes into individual subpackages
- realign requires in core package to include at least one graphics module and
one theme, graphics modules require the corresponding library packages

New files:
SPEC:  http://www.berningeronline.net/gallery2.spec
SRPM:  http://www.berningeronline.net/gallery2-2.0.2-6.src.rpm

Still needs formal review / sponsor.

When building the package I noticed that /usr/local/bin/perl got listed as a
dependency.  Grepping the source, I see a number of references for binaries in
/usr/local/bin.  These should all be changed to /usr/bin.  

Also, I run into this problem:
$ sudo rpm -ivh gallery2-2.0.2-6.noarch.rpm gallery2-gd-2.0.2-6.noarch.rpm
gallery2-classic-2.0.2-6.noarch.rpm 
error: Failed dependencies:
	gd <= 2.0 is needed by gallery2-gd-2.0.2-6.noarch
$ rpm -q gd
gd-2.0.33-2
$ cat /etc/fedora-release 
Fedora Core release 4 (Stentz)

Dependency issue fixed - typo.  "<=" should have been ">="
/usr/local/bin/perl references in /lib/tools/ changed to /usr/bin/perl

New files:
SPEC:  http://www.berningeronline.net/gallery2.spec
SRPM:  http://www.berningeronline.net/gallery2-2.0.2-7.src.rpm

I tried a minimal install using the default values from the installer.

$ sudo rpm -ivh gallery2-2.0.2-7.noarch.rpm gallery2-gd-2.0.2-7.noarch.rpm
gallery2-classic-2.0.2-7.noarch.rpm 

During Step 2 of the installer, the following files failed the "Integrity Chack"
because they were modified:

lib/tools/bin/extractClassXml.pl
lib/tools/bin/filterManifests.pl
lib/tools/bin/getIllegalFunctions.pl
lib/tools/bin/makeManifest.pl
lib/tools/po/extract.pl
lib/tools/uml/make-java-classes.pl

Step 4: Can a /srv/gallery2 directory (owned by the apache user/group) be
included in the gallery2 RPM and used as the default here?

Step 7: A blank /var/www/html/gallery2/config.php could be included in the RPM

Step 8: The first time I ran the install I got this error:

Error: Failed to load matrix theme, this is the error stack trace; Error
(ERROR_BAD_PARAMETER) :
/var/www/html/gallery2/modules/core/classes/helpers/../../../../themes/matrix/theme.inc<b>in</b>
modules/core/classes/helpers/GalleryPluginHelper_simple.class <b>at line</b> 105
(CoreModule::error)

I then installed gallery2-matrix and reran the install.  This time, at step 8 I got:

Fatal error: Allowed memory size of 8388608 bytes exhausted (tried to allocate
125 bytes) in /var/www/html/gallery2/lib/adodb/adodb-lib.inc.php on line 926

It had warned me about this in step 2 of the installer.

http://codex.gallery2.org/index.php/Gallery1:FAQ#Why_do_I_get_the_error_Allowed_memory_size_of_Xxx_bytes_exhausted.3F

We can create a gallery2-specific value for this by adding an entry (e.g.
php_value memory_limit 17000000) in /var/www/html/gallery2/.htaccess.  This also
required creating a /etc/httpd/conf.d/gallery2.conf file with the following
contents:

<Directory /var/www/html/gallery2>
  AllowOverride Options
</Directory>

On the first issue, this is due to changing /usr/local/bin/perl to
/usr/bin/perl.  The MAIFEST file can only be rebuilt against a CVS checked-out
tree, as opposed to a versioned tarball.  Is using a CVS tree (bundled up into a
tarball, of course) acceptable for the source?  If so, how do I specify a full
URL for that in the Source0 tag?

I'm working on the other issues noticed now.

* gallery2-gd must "Requires: php-gd" as else the GD module cannot
be installed successfully within Gallery2 modules admin section.

* "yum install gallery2" adds gallery2-gd and gallery2-tile, but:
file /var/www/html/gallery2/modules is not owned by any package

* Further, since "gallery2-matrix" is needed and provides
gallery2-display just like gallery2-tile does, the current virtual
dependency mechanism "gallery2-display" is questionable. gallery2-tile
does not suffice. gallery2-matrix is not optional.

Updated packages.  I've had to switch to using a CVS tarball in order to rebuild
the MANIFEST properly, which is reflected in the specfile changelog.  CVS also
gave me a bunch more modules, those are separated as the standard ones are. 
This build seems to install and configure properly, with the caveat that the
database has to be created independently.  New package locations:

SPEC:  http://www.berningeronline.net/gallery2.spec
SRPM:  http://www.berningeronline.net/gallery2-2.0.2-0.7cvs20060223.src.rpm

Updated %post to set SELinux security context on /srv/gallery2 when
/usr/bin/chcon is detected as executable.

SPEC:  http://www.berningeronline.net/gallery2.spec
SRPM:  http://www.berningeronline.net/gallery2-2.0.2-0.8cvs20060223.src.rpm

As far as I know, SELinux contexts are something that should be done in the
selinux-policy package (until drop-in policy modules from packages are
possible), not chcon'ing in packages.

Without the chcon or modification of the selinux-policy package, the web server
will be unable to do anything in the default gallery directory (/srv/gallery2)
if SELinux is in enforcing mode.  Since I don't have ability or access to change
the policy package, I simply chcon'ed as a workaround until the policy is
changed.  As the gallery2 package creates the /srv/gallery2 directory, I'm not
sure changing the policy would have any effect - it may have to be either a
"chcon" in %post or wait until drop-in policy modules are possible/accepted.

Just file a bug/RFE against the selinux-policy package with details on what's
needed.  According to my experience, those requests tend to get fulfilled very
quickly especially in simple cases such as this one.

Assuming the RFE is accepted, which based on above comments I think is a safe
assumption, how does the policy get applied when the gallery2 package is
installed?  Do I need to run 'restorecon' in %post, or is there magic I'm
unaware of the apply the correct contexts automatically when the directory is
created?

No need to run anything, that magic exists and is how other files in the
filesystem get their contexts too.

Updated packages to remove chcon in %post, filed RFE for policy change.  New
packages:

SPEC:  http://www.berningeronline.net/gallery2.spec
SRPM:  http://www.berningeronline.net/gallery2-2.0.2-0.9cvs20060223.src.rpm

selinux-policy RFE:  https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=183140

After having a discussion with some folks, I think /var/www/gallery2 would be a
better base directory.  It was pointed out that this ensures that the content
gets the right SELinux context.  Also, files under /var/www/html are
user-managed content and are susceptible to accidentally get erased or moved by
someone administering website content.

We would also want to include /etc/httpd/conf.d/gallery2.conf, which could be as
simple as:

Alias /gallery2 /var/www/gallery2

Files updated to install into /var/www/gallery2.  The default data directory is
still set to be /srv/gallery2 per suggestion in comment #20, but is overridable
at setup time by the user.

New packages:
SPEC:  http://www.berningeronline.net/gallery2.spec
SRPM:  http://www.berningeronline.net/gallery2-2.0.2-0.10cvs20060223.src.rpm

Multiple vulnerabilities in < 2.0.3:

http://www.gulftech.org/?node=research&article_id=00106-03022006
http://gallery.menalto.com/gallery_2.0.3_released

I'm not sure this vulnerability applies since I'm using a CVS snapshot, but I've
updated the packages anyway to today's snapshot.  I've also changed the version
to 2.0 (from 2.0.2) to reflect the fact that I'm pulling from the trunk, not
from the 2.0.2 or 2.0.3 branch.  I'd like to get a formal review and approve or
reject for this package - I believe I've met all of the packaging requirements /
guidelines, and to be honest I'm getting a bit frustrated by the time it's
taking.  I appreciate the help in getting it into shape, but I'd like to get a
formal accept or reject (and a reason why if the latter).

New packages:
SPEC:  http://www.berningeronline.net/gallery2.spec
SRPM:  http://www.berningeronline.net/gallery2-2.0-0.11cvs20060303.src.rpm

Any chance of a sponsor review on this package?

I just rebuilt the SRPM from comment #34.  The build and install were fine,
except the file /etc/httpd/conf.d/gallery2.conf contains:

<Directory /var/www/html/gallery2>
  AllowOverride Options
</Directory>

 rather than containing the alias directive from comment #31:

Alias /gallery2 /var/www/gallery2

Once I changed this file it worked perfectly.

[Sorry I am just an appreciative user, so I can't approve or sponsor you]

Fixed in new packages:

SPEC:  http://www.berningeronline.net/gallery2.spec
SRPM:  http://www.berningeronline.net/gallery2-2.0-0.12cvs20060303.src.rpm

Created attachment 126327 [details]
diff of spec file with suggested changes

After doing a clean install and running rpmlint, I have a few more items
reflected in the attach spec file patch:

* Removed gallery2-exif dependency on libexif as a) rpmlint complained about it
and b) it doesn't appear to be necessary

* rpmlint complained that some package description lines were too long, so I
wrapped them

* rpmlint complained that some scripts were marked as excutable but did not
start with #!.	This list included a lot of .po files and others that clearly
shouldn't be executable.  So, I added a step in the %build section to clean
this up.

* made apache the owner of /srv/gallery2 and removed the chmod 777 of the
directory.  By the way, I noticed after the install and uploading some photos
that gallery uses that directory for a whole bunch of stuff, not just pictures.
 Ideally, this should be fixed to use the correct directories (e.g.
/var/cache/gallery2 instead of /srv/gallery2/cache).  However, that could be a
large project and not something worth blocking on IMO.

* added "%dir %{installprefix}/gallery2" to the file list
* made apache the owner of /var/www/gallery2/config.php as it's needed for
installation
* marked /etc/httpd/conf.d/gallery2.conf as a config file

Also, in /etc/httpd/conf.d/gallery2.conf there is still a reference to
/var/www/html/gallery2 that needs to be fixed.

Created attachment 126328 [details]
list of errors from rpmlint

This is the list of errors from rpmlint after making the changes listed in my
previous attachment.

Here's my commentary grouped by error type:

* "non-executable-script" - These should be benign, but probably the real
question is if they need to be included in the RPMs at all.
* "htaccess-file" - gallery2-rewrite is the largest offender here.  
* "non-standard-uid" - I think we're fine with the apache user
* "wrong-script-interpreter" - Another reference to /usr/local/bin/perl
* "zero-length" - config.php is intentionally empty.  Maybe add a comment to
the file that says 'This file intentionally empty.  It gets populated during
the gallery install process'
* "no-jar-manifest" - problem with the upstream jar file.  Not a blocking
issue.

I don't get any of those errors from rpmlint when running against the 0.12cvs
SRPM - I'm looking for rpmlint 'config' (e.g. ~/.rpmlintrc or similar) file
information on the wiki now, any suggestions off the top of anyone's head? 
However, I have made the suggested changes and re-rolled a 0.13 package.

The usage of the /srv/gallery2 directory is determined by Gallery, and it would
be extremely difficult to split up the directory usage.  If I had more time, I'd
be willing to try to develop a patch to do so, but it would probably be more
effort than it's worth.

New packages:
SPEC:  http://www.berningeronline.net/gallery2.spec
SRPM:  http://www.berningeronline.net/gallery2-2.0-0.13cvs20060303.src.rpm

Any chance of a review on this?

Sorry this has been sitting in NEW for so long... 

Can you update to 2.1.1 (latest stable) and I will be happy to do a full review.

Updated to SVN snapshot of today's BRANCH_2_1 and re-rolled packages:

SPEC:  http://www.berningeronline.net/gallery2.spec
SRPM:  http://www.berningeronline.net/gallery2-2.1-0.14cvs20060524.src.rpm

Caught a problem with mock builds, need to list subversion as a BuildRequires so
we can regenerate the MANIFEST properly.  New package:

SPEC:  http://www.berningeronline.net/gallery2.spec
SRPM:  http://www.berningeronline.net/gallery2-2.1-0.15cvs20060524.src.rpm

Oh, just noticed your comment about subversion. I ran into that with the
following review as well. It looks like you didn't update your spec/src.rpms
with the new BuildRequires: subversion in comment #44? (ie, the spec seems to be 
-13 still?)

See below - Rpmlint output.
See below - Package name.
OK - Spec file name matches.
OK - Package guidelines.
OK - Licsense.
OK - License field matches in spec.
OK - License included in files
OK - Spec in american english
OK - Spec legible
See below - Md5sum of source from upstream
See below - Compiles and builds on one arch at least.
OK - NO Forbidden buildrequires included
OK - All required buildrequires included
See below - Locale handling/find_lang.
OK - Owns all directories it creates.
OK - No duplicate files in %files listing.
OK - Permissions on files correct.
OK - Clean section correct.
OK - Macros consistant.
OK - Code not content.
OK - Docs must not affect runtime.
OK - Doesn't own any files/dirs that are already owned by others.

See below - Package builds in mock.
See below - Subpackages require base package.

1. Is there any further need to base the package off a snapshot instead of
the now current stable release?

2. If the answer to 1 is yes, should the package have svn in the release
instead of cvs?
See:
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-d97a3f40b6dd9d2288206ac9bd8f1bf9b791b22a

3. The Requires for the subpackages should probibly be:
Requires: %{name} = %{version}-%{release}
Otherwise you can get diffrent 2.1 versions of the various packages installed
at the same time.

4. Doesn't build in mock here... I get the following at the end of
the build.log:

+ perl makeManifest.pl
Finding all files...Died at makeManifest.pl line 153.
error: Bad exit status from /var/tmp/rpm-tmp.63104 (%build)

which is a call to svn:
  open(FD, "svn propget --non-interactive -R svn:mime-type |") or die;

Perhaps a missing BuildRequires: subversion
I can't tell for sure if this will hit the network during a build.
If so, it will fail in the buildsystem. Basing on a non snapshot might
fix this issue as well.

5. Can you provide a script used to generate the svn tar.gz snapshot
or otherwise describe how to duplicate it? Switching back to the stable
version would remove this requirement as well. ;)

6. Perhaps a README.fedora that has info on how to setup
the database easily? Or pointers to the docs to do that?

7. You need to use the find_lang macro/setup for locales, you can't
just include them in files sections, See:

http://fedoraproject.org/wiki/Packaging/Guidelines?#head-8c605ebf8330f6d505f384e671986fa99a8f72ee

8. Lots of rpmlint output:

W: gallery2 incoherent-version-in-changelog 2.0-0.13.cvs20060303
2.1-0.14cvs20060524.fc6
E: gallery2 htaccess-file /var/www/gallery2/lib/tools/po/.htaccess
E: gallery2 htaccess-file /var/www/gallery2/.htaccess
E: gallery2 htaccess-file /var/www/gallery2/lib/tools/bin/.htaccess
E: gallery2 htaccess-file /var/www/gallery2/lib/tools/stubs/.htaccess
E: gallery2-rewrite htaccess-file
/var/www/gallery2/modules/rewrite/data/mod_rewrite_no_options/gallery/.htaccess
E: gallery2-rewrite htaccess-file
/var/www/gallery2/modules/rewrite/data/mod_rewrite/gallery/.htaccess
E: gallery2-rewrite htaccess-file
/var/www/gallery2/modules/rewrite/data/mod_rewrite_no_options/custom/.htaccess
E: gallery2-rewrite htaccess-file
/var/www/gallery2/modules/rewrite/data/mod_rewrite_no_options/gallery2/.htaccess
E: gallery2-rewrite htaccess-file
/var/www/gallery2/modules/rewrite/data/mod_rewrite/gallery2/.htaccess
E: gallery2-rewrite htaccess-file
/var/www/gallery2/modules/rewrite/data/mod_rewrite/custom/.htaccess

Suggest from rpmlint:

htaccess-file :
You have individual apache configuration .htaccess file(s) in your package.
Replace them by a central configuration file in /etc/httpd/conf.d.
Is it possible to fold them into the main gallery2.conf?

E: gallery2 non-executable-script
/var/www/gallery2/lib/tools/po/premerge-messages.pl 0644
E: gallery2 non-executable-script
/var/www/gallery2/lib/tools/po/update-all-translations.pl 0644
E: gallery2 non-executable-script
/var/www/gallery2/lib/tools/bin/getIllegalFunctions.pl 0644
E: gallery2 non-executable-script
/var/www/gallery2/lib/tools/uml/make-java-classes.pl 0644
E: gallery2 non-executable-script /var/www/gallery2/lib/tools/po/header.pl 0644

Should those scripts be 755?

E: gallery2 non-standard-uid /srv/gallery2 apache
E: gallery2 non-standard-uid /var/www/gallery2/config.php apache

I think those can be ignored.

E: gallery2 no-jar-manifest
/var/www/gallery2/modules/core/classes/GalleryStorage/g2_db2.jar

Something to complain to upstream about?

There are tons of these... basically for each subpackage:

W: gallery2-albumselect summary-not-capitalized albumselect module for Gallery 2
W: gallery2-albumselect no-documentation

No documentation can be ignored.
You should go ahead and capitalize the first letter of each Summary.

1. Yes, we still need to use the snapshot.  The upstream source uses
/usr/local/bin/perl (for example) which needs to be changed to /usr/bin/perl;
this requires regeneration of the MANIFEST, which can only done with an SVN
snapshot (makeManifest.pl requires the .svn dirs)

2. Yes, I've changed it to svn vs cvs.

3. Fixed

4. Fixed (BuildReq: subversion added) - build will not hit the network - I build
the -15 package successfully on an isolated system with no network cable plugged in.

5. Creating the tarball is a four step process:
- svn checkout
https://svn.sourceforge.net/svnroot/gallery/branches/BRANCH_2_1/gallery2 gallery2
- svn checkout https://svn.sourceforge.net/svnroot/gallery/trunk/gallery2
gallery2-trunk
- cp gallery2-trunk/lib/tools/bin/makeManifest.pl gallery2/lib/tools/bin/
- tar zcvf gallery-2.1.svn.tar.gz gallery2

6. Will add this file a bit later, working on the other issues right now.

7. The %find_lang macro in %install is causing a build failure:
+ /usr/lib/rpm/redhat/find-lang.sh
/var/tmp/gallery2-2.1-0.15svn20060524-root-jwb gallery2
No translations found for gallery2 in /var/tmp/gallery2-2.1-0.15svn20060524-root-jwb
error: Bad exit status from /var/tmp/rpm-tmp.57785 (%install)

Any suggestions?

8.
- htaccess files - these have to be where they are, as gallery2 rewrites them
via the admin web interface.  We can't move them into the gallery2.conf in
httpd/conf.d/
- non-executable-scripts fixed
- Capitalization fixed.

New files:
SPEC:  http://www.berningeronline.net/gallery2.spec
SRPM:  http://www.berningeronline.net/gallery2-2.1-0.16cvs20060524.src.rpm

Please note the SRPM above will not rebuild - I need assistance with the
%find_lang macro.

ok. So, sounds like we are down to just the find_lang issues... 
The SRPM link in comment #46 is wrong... s/cvs/svn/ to get the valid link. 

find_lang looks for .mo files under the buildroot. It needs to know the $name.mo
though to find them. Looks like the .mo files here are not named 'gallery2.mo'. 
Do a 'rpm -qlip *.noarch.rpm | grep .mo$ ' on the noarch rpms and you will see
the .mo files. 

For the main package they are named 'gallery-install' 'modules_core'
'gallery2_upgrade'. So, I think you need 3 find_lang calls with each of those
names. You also need to add to the files section: 
%files -f gallery-install.lang modules_core.lang gallery2_upgrade.lang

All the modules and themes have their own .mo files, named: 
modules_$modulename or themes_$themename. So, they will have to be added to each
subpackage's %files section... 

Does that make sense? Can you try adding that and see if you can get it working?
I can try testing it here later tonight as well...

It makes sense, but I'm still getting the same error message.  SPEC snippet:
%find_lang themes_siriux

Error:
+ /usr/lib/rpm/redhat/find-lang.sh
/var/tmp/gallery2-2.1-0.16svn20060524-root-jwb themes_siriux
No translations found for themes_siriux in
/var/tmp/gallery2-2.1-0.16svn20060524-root-jwb
error: Bad exit status from /var/tmp/rpm-tmp.42330 (%install)

But there *are* themes_siriux.mo files in the buildroot...

fwiw, the multiple subpackages can be created shorter by a macro like

---
%global spc_nil		%nil
%global spc		%spc_nil %spc_nil
%global true		1
%trace
%{?true:%global	pkgdcl(n:N:d:R:)		\
%package %%{-n*}				\
Summary: %%{-N*} module for Gallery 2		\
Group: Applications/Publishing			\
Requires: gallery2 = %%{version}-%%{release}	\
%%{?-R:Requires:%%{-R*}}			\
	\
%description %%{-n*}				\
Gallery 2 module - %%{-d*}			\
	\
%files %%{-n*}					\
%defattr(-,root,root,-)				\
%{installprefix}/gallery2/modules/%%{-n*}	\
}
---

which can be used like

---
%{pkgdcl -n albumselect -N Albumselect \
	 -d Jump%{spc}directly%{spc}to%{spc}any%{spc}album%{spc}using%{spc}a%{spc}select%{spc}box%{spc}or%{spc}tree%{spc}view}
%{pkgdcl -n netpbm - N NetPBM \
	 -d NetPBM%{spc}Graphics%{spc}Toolkit
	 -R netpbm>=9.0}
...
---

This creates
* the '%package ...' + related headers
* the '%description'
* the '%files'


Generating spaces is a little bit tricky; for description you can use
perhaps '%*' too. It's up to the exercise of the reader to write such a
modified macro ;)

Thanks to rdieter on the irc channel for looking into the find_lang issue... 

It looks like find_lang only finds .mo files that are properly installed under
/usr/share/locale/<lang>/<name>.mo

How hard would it be to patch gallery to install those .mo's there and use them
correctly there? 

Almost impossible, as each individual theme / module would have to be patched;
the Gallery codebase is deliberately designed to be completely self-contained
within it's install root.  Although technically possible, the patch to do this
may very well end up half as large as Gallery itself, if not larger.

In answer to comment #51: 

I was afraid of that. I looked and saw that the debian package had it's locale
files in /usr/share/locale, but then I saw it was 1.5.x and they had contributed
their own locale files, so that doesn't help us at all. ;( 

Can you generate a new spec/src.rpm without find_lang? (ie, back to just
packaging them as regular files). I do see several other packages having private
lang files, but it's unclear if those are packaging bugs or not (mailman, PyXML,
vim), in any case thats the only alternative I see here. 

If you can put up a new spec/src.rpm I will try and do some functionality
testing tonight and go over anything else thats outstanding. Hopefully we can
get this approved soon... 

Done.  New package:

SPEC:  http://www.berningeronline.net/gallery2.spec
SRPM:  http://www.berningeronline.net/gallery2-2.1-0.17svn20060524.src.rpm

I can't seem to get it to work here. 

(possibly related to the /srv/gallery2 path? )

Either installing multi-site or single site, it gives me the same error in step #8:

Error (ERROR_PLATFORM_FAILURE) : /srv/gallery2/locks/0/1/1

    * in modules/core/classes/FlockLockSystem.class at line 77
(GalleryCoreApi::error)
    * in modules/core/classes/GalleryLockSystem.class at line 101
(FlockLockSystem::_acquireLock)
    * in modules/core/classes/helpers/GalleryLockHelper_simple.class at line 66
(GalleryLockSystem::acquireReadLock)
    * in modules/core/classes/GalleryCoreApi.class at line 2263
(GalleryLockHelper_simple::acquireReadLock)
    * in modules/core/classes/helpers/GalleryPermissionHelper_advanced.class at
line 1154 (GalleryCoreApi::acquireReadLock)
    * in modules/core/classes/helpers/GalleryPermissionHelper_advanced.class at
line 247 (GalleryPermissionHelper_advanced::_getAccessListCompacterLock)
    * in modules/core/classes/helpers/GalleryPermissionHelper_advanced.class at
line 78 (GalleryPermissionHelper_advanced::_changePermission)
    * in modules/core/classes/GalleryCoreApi.class at line 732
(GalleryPermissionHelper_advanced::addGroupPermission)
    * in modules/core/CoreModuleExtras.inc at line 2486
(GalleryCoreApi::addGroupPermission)
    * in ??? at line 0 (CoreModuleExtras::_createRootAlbumItem)
    * in modules/core/CoreModuleExtras.inc at line 265
    * in modules/core/module.inc at line 487 (CoreModuleExtras::upgrade)
    * in modules/core/classes/GalleryModule.class at line 157 (CoreModule::upgrade)
    * in install/steps/InstallCoreModuleStep.class at line 131
(GalleryModule::installOrUpgrade)
    * in install/index.php at line 201 (InstallCoreModuleStep::processRequest)

I've just retested against the -17svn packages and successfully created the
gallery.  I'm on a fairly vanilla FC5 machine, no outlandish configs.  I'm going
to take a wild guess here, and I may be way off my rocker, but this looks like
it could be either disk space or SELinux getting in the way.  I don't set any
SELinux contexts on either /var/www/gallery2 or /srv/gallery2 per earlier
comments in this bug, and the bit about the error coming from
GalleryCoreApi::addGroupPermission is setting off red flags and clanging bells
for me...

Re: comment #54, I couldn't get a multisite to work either (same error as 
you...), but *did* get single-site to work.  I was testing 16svn on a rhel4 
box though...

Small nitpick: Packaging Guidelines say to use (something like)
Release: 0.17.svn20060524
instead of
Release: 0.17svn20060524
(ie, put a . between 0.x and the cvs/svn bits)

I tried both multisite and single site and couldn't get either to work. 
I did try the multisite first tho, and it might have left some junk around that
prevented the single site from working. Selinux is off. Lots of disk space. 

I will try and investigate further later tonight. 

I just did the following steps: 

- removed all gallery packages. 
- rm -rf /srv/gallery2
- rm -rf /var/www/gallery2
- removed all users from mysql. 
- dropped all mysql db's. 
- Reinstalled the 0.17 gallery rpms. 
- Went thru the install steps for single site. Then tried multi site. 

I got the same error both times in step 8...

Warning: fopen(/srv/gallery2/locks/0/1/1) [function.fopen]: failed to open
stream: No such file or directory in
/var/www/gallery2/modules/core/classes/GalleryPlatform.class on line 369

Any ideas? 
I can try later on a devel/rawhide machine. 

I have no idea. I've tried the -17 packages on FC5 and FC3 and both have worked;
I've pasted the error to upstream and asked if they have any ideas.

I ran into this problem configuring for multisite when I tried out the earlier
RPMS. The problem for me seemed to be that no matter what I put in for the data
directory (the non web accessible data area), after the config file was written,
it had a different (i.e., not the one I specified) /srv based data directory.
After the config file writing step I had to edit the just written config file to
point to the correct data directory or else I would get the same error on the
next step when the install procedure copied over the files to the new multisite.
Hopefully not to obtuse...

Re comment #61: 

Yeah, that was the problem. It was defaulting to /srv/gallery2 instead of the
data dir that I gave it. This must be a bug in the installer... ;( 
Multi-site is working here if I go to that step and manually edit the config.php
to put in the right path. 

So, outstanding issues I see: 

- Making a README.fedora would be nice (but not required). 

- There was some talk at a recent FESCo meeting about requiring web apps to
install in /usr/share/$name instead of /var/www/$name, but I don't see any hard
requirement on it currently, and I don't think the selinux stuff is in place for
that currently either. 

I want to do a final rebuild/rpmlint/sanity check here before approval. 

If anyone else sees any blockers, speak up now...

/usr/share/%{name} was chosen by the committee; use of /var/www/%{name} should
be a blocker.

http://fedoraproject.org/wiki/Extras/SteeringCommittee/Meeting-20060518

Scroll down to the Free Discussion section, time 20:11 UTC.  Unfortunately it
doesn't look like this made it into the guidelines.

Note the end of http://fedoraproject.org/wiki/Packaging/Guidelines

Updated package; I think this one solves the multisite install errors (at least
it did for me).  I also changed the installation path to /usr/share/gallery2 per
the updated guidelines.

SPEC:  http://www.berningeronline.net/gallery2.spec
SRPM:  http://www.berningeronline.net/gallery2-2.1-0.18.svn20060524.src.rpm

Yeah, multisite looks good now here too. 

So with the install under /usr/share/gallery2, further selinux changes will need
to be made to allow it to work with selinux on, right?

Also, since it changes files under that tree, read-only /usr won't work with it,
but not sure how big a deal that is anymore. 

One rpmlint issue in this latest version: 

E: gallery2 non-executable-script /usr/share/gallery2/lib/tools/po/header.pl 0644

Should that be not shipped? or made 755?

(In reply to comment #66)
> Yeah, multisite looks good now here too. 
> 
> So with the install under /usr/share/gallery2, further selinux changes will need
> to be made to allow it to work with selinux on, right?

PHP applications should run just fine out of /usr/share with SELinux on,
provided that the httpd_internal_scripting boolean is on.

> Also, since it changes files under that tree, read-only /usr won't work with it,
> but not sure how big a deal that is anymore.

That would be a blocker if I was reviewing the package. Can't the places where
writable data goes be made symlinks into /srv or /var?

> One rpmlint issue in this latest version: 
> 
> E: gallery2 non-executable-script /usr/share/gallery2/lib/tools/po/header.pl 0644
> 
> Should that be not shipped? or made 755?

Is it needed for anything at runtime?

The package will work fine with a read-only /usr given that:
- The admin makes /usr rw to install the package (needed for all packages)
- The gallery store is outside /usr (e.g. /srv/gallery2)

Additionally, /usr being ro can't be a blocker because that would conflict with
the updated packaging guidelines stating web apps must go into /usr/share

Script perms have been corrected, new packages:

SPEC:  http://www.berningeronline.net/gallery2.spec
SRPM:  http://www.berningeronline.net/gallery2-2.1-0.19.svn20060524.src.rpm

It looks like the only thing that would need to get written in
/usr/share/gallery2 is the login.txt file that the installer requires you to put
there to authenticate. Is there any chance you could patch it to look for that
in /srv/gallery2/ instead? 

Thats the only final issue I see before approval. ;) 

(In reply to comment #68)
> The package will work fine with a read-only /usr given that:
> - The admin makes /usr rw to install the package (needed for all packages)
> - The gallery store is outside /usr (e.g. /srv/gallery2)
> 
> Additionally, /usr being ro can't be a blocker because that would conflict with
> the updated packaging guidelines stating web apps must go into /usr/share

The application needs to go in /usr/share; its data doesn't need to. Anything
that's writable is data.

(In reply to comment #69)
> It looks like the only thing that would need to get written in
> /usr/share/gallery2 is the login.txt file that the installer requires you to put
> there to authenticate. Is there any chance you could patch it to look for that
> in /srv/gallery2/ instead? 

It might not even need patching; could the file be replaced by a symlink to
somewhere like /etc/gallery2/login.txt, and install the original file there?

I've made /usr/share/gallery2/login.txt a symlink to /etc/gallery2/login.txt. 
There are two things I'm not quite sure I like about this, either may end up
blocking approval (which I'd be fine with and will then try to patch):

1. When /etc/gallery2/login.txt is marked %config(noreplace), the setup
complains that the login.txt doesn't match (as opposed to simply asking you to
create it).

2. When %ghost-ed, setup complains that it couldn't be read accessed.

In both these situations, the complaint looks like there was a problem with a
previous login.txt, but simply creating the correct one (in both cases) will
resolve the situation.

New packages:
SPEC:  http://www.berningeronline.net/gallery2.spec
SRPM:  http://www.berningeronline.net/gallery2-2.1-0.20.svn20060524.src.rpm

I don't really like putting the login.txt in /etc/gallery2... it's not really a
config file, it's more of a temp auth token IMHO. 

Each login.txt only works for one install run, right?

Any login.txt you generate at build time will be wrong when the app runs the
installer the first time, so shipping any login.txt isn't going to work I don't
think. 

How about a link from /usr/share/gallery2/login.txt to /srv/gallery2/login.txt?

Then that /srv/gallery2/login.txt could just be a %ghosted file, but make sure
it gets created with root.root and 600 perms (to prevent a normal user from
writing it). 

Thoughts?

(In reply to comment #72)
> Any login.txt you generate at build time will be wrong when the app runs the
> installer the first time, so shipping any login.txt isn't going to work I don't
> think. 
> 
> How about a link from /usr/share/gallery2/login.txt to /srv/gallery2/login.txt?
> 
> Then that /srv/gallery2/login.txt could just be a %ghosted file, but make sure
> it gets created with root.root and 600 perms (to prevent a normal user from
> writing it). 
> 
> Thoughts?

If it's ghosted, no file will get installed there when the rpm is installed, so
the permissions won't make any difference. Sounds like a patch will be needed
rather than a symlink unfortunately.

Well, I was thinking a %ghost for that file, then in create it in %post...
either an an empty file or as one containing information about why it's there.
It would then be overwritten by the install running user when they are running
the installer. 

A patch would be nicer, not sure how difficult it will be to patch however. 
In addition to the path to login.txt, you need to make sure and change the
install to refer to the new correct directory to upload the file to. 

Turns out it's quite easy to patch.  I've done so, %ghost'ed the login.txt, and
re-rolled the package:

SPEC:  http://www.berningeronline.net/gallery2.spec
SRPM:  http://www.berningeronline.net/gallery2-2.1-0.21.svn20060524.src.rpm

Excellent. I am able to now do new multisite installs without changing anything
under /usr/share/gallery2. The updates seem to work just fine with existing
sites from previous versions. 

I don't see any further blockers... I'm happy to (finally) call this package
APPROVED. 

Thanks for all your hard work John (and all the good comments from everyone). 

Don't forget to close this bug with NEXTRELEASE once it's imported and built. 

Package imported, waiting on CVS sync for FC4/5 now.

Probably a bit late now, but if config.php is really a config file, shouldn't it
live under /etc rather than /usr/share ?

Yes, it probably should...  crap, and it's going to run afoul of a ro /usr as
well, since the setup writes it out.  For now, I've simply noted in the README
that /usr must be rw when running through setup, I'll look into patching to put
config.php in a different location later.

Well, that file (/usr/share/gallery2/config.php) is only written to if you run
the installer and select that directory to install in, right? 

You shouldn't ever install a gallery there, you should install under
/srv/gallery2/ which should write out a new config.php in whatever directory you
choose. Or am I confusing multisite mode with standard mode installs?

That might be the case in multisite mode, but single site it drops config.php
into /usr/share/gallery2 whether you like it or not.

Packages have been built for FC4/5/devel - as soon as I can file a bug regarding
the config.php against the component (read: as soon as gallery2 gets imported as
a component) I'll close this feature request.  Keeping it open for now so I
don't forget to look at that.