Bug 569833

Is there a reason why you don't just update drupal in EL-5 to drupal 6?

(Couldn't find something relevant on the blocker bug.)

Because we try to do major updates to EPEL packages as little as possible, especially incompatible ones (modules, plugins, etc).  Having a parallel installable version allows admins to migrate more easily.  I anticipate using version 6 for the main drupal package in EL-6.

Also, I've updated the version :

SPEC: http://zanoni.jcomserv.net/fedora/drupal6/drupal6.spec
SRPM: http://zanoni.jcomserv.net/fedora/drupal6/drupal6-6.16-1.fc12.src.rpm

Not related to this review, but I have potential module and theme specfiles available here: https://fedoraproject.org/wiki/Drupal_module_specfile_template

If anyone is willing to look and/or edit to meet the requirements of this parallel installable drupal6 plus the existing drupal in EL-5, I'd appreciate it.

Not saying this is wrong, but why is the drupal-files-migrator.sh included as a %doc? 

Also - any thought on moving .htaccess functionality to the .conf files already included? There's also a problem with that .htaccess file, a closing quote is missing on line 22. 

FIX: rpmlint must be run on every package. The output should be posted in the review.
[ke4qqq@nalleyx60 SPECS]$ rpmlint drupal6.spec 
drupal6.spec:52: W: macro-in-comment %{_localstatedir}
drupal6.spec:52: W: macro-in-comment %{name}
drupal6.spec:52: W: macro-in-comment %{buildroot}
drupal6.spec:52: W: macro-in-comment %{drupaldir}
0 packages and 1 specfiles checked; 0 errors, 4 warnings.
[ke4qqq@nalleyx60 SPECS]$ rpmlint ../SRPMS/drupal6-6.16-1.fc13.src.rpm 
drupal6.src: W: spelling-error %description -l en_US weblogs -> we blogs, we-blogs, web logs
drupal6.src: W: spelling-error %description -l en_US Drupal -> Drupelet, Drupe, Druidical
drupal6.src: W: spelling-error %description -l en_US skinnable -> winnable, scannable, skinniness
drupal6.src:52: W: macro-in-comment %{_localstatedir}
drupal6.src:52: W: macro-in-comment %{name}
drupal6.src:52: W: macro-in-comment %{buildroot}
drupal6.src:52: W: macro-in-comment %{drupaldir}
1 packages and 0 specfiles checked; 0 errors, 7 warnings.
[ke4qqq@nalleyx60 SPECS]$ rpmlint ../RPMS/noarch/drupal6-6.16-1.fc13.noarch.rpm 
drupal6.noarch: W: spelling-error %description -l en_US weblogs -> we blogs, we-blogs, web logs
drupal6.noarch: W: spelling-error %description -l en_US Drupal -> Drupelet, Drupe, Druidical
drupal6.noarch: W: spelling-error %description -l en_US skinnable -> winnable, scannable, skinniness
drupal6.noarch: W: non-etc-or-var-file-marked-as-conffile /usr/share/drupal6/.htaccess
drupal6.noarch: E: non-standard-dir-perm /var/lib/drupal6/files/default 0775L
drupal6.noarch: E: executable-marked-as-config-file /etc/cron.hourly/drupal6
drupal6.noarch: E: non-standard-dir-perm /var/lib/drupal6 0775L
drupal6.noarch: E: non-standard-dir-perm /var/lib/drupal6/files 0775L
drupal6.noarch: E: htaccess-file /usr/share/drupal6/.htaccess
1 packages and 0 specfiles checked; 5 errors, 4 warnings

Why is the .htaccess file marked as a conf file?? Generally .htaccess stuff is frowned on in favor of using the .conf files. Not a blocker, but just for your consideration. 
The spelling errors are false positivies. 
Macro in comment can be silenced by removing the commented out symlink line in %install. Doesn't bother me terribly though.   


OK: The package must be named according to the Package Naming Guidelines .

Seems ok per http://fedoraproject.org/wiki/PackageNamingGuidelines#Multiple_packages_with_the_same_base_name

OK: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption.
MUST: The package must meet the Packaging Guidelines
OK: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .

Source says only GPL 
There is a license file that contains the content of LGPLv2
I think that makes it GPL+


FIX: The License field in the package spec file must match the actual license.
Source code only says GPL - which when reading the GPL+ entry here:
http://fedoraproject.org/wiki/Licensing
Seems to indicate that GPLv2+ is an incorrect entry. 


OK: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc.
OK: The spec file must be written in American English. 
OK: The spec file for the package MUST be legible. 
OK: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this.

[ke4qqq@nalleyx60 SOURCES]$ md5sum drupal-6.16.tar.gz*
bb27c1f90680b86df2c535b2d52e8021  drupal-6.16.tar.gz
bb27c1f90680b86df2c535b2d52e8021  drupal-6.16.tar.gz.1

OK: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
NA: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line.
OK: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense.
NA: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.
NA: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. 
OK: Packages must NOT bundle copies of system libraries.

Note jquery is bundled, but it is a javascript library, which based on my understanding since no packaging guidelines for javascript exist at the moment, this is permissible. However, going forward this may change. 

NA: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker. 
OK: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. 
FIX: A Fedora package must not list a file more than once in the spec file's %files listings. 
warning: File listed twice: /usr/share/drupal6/.htaccess

OK: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line. 
OK: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). 
OK: Each package must consistently use macros. 
OK: The package must contain code, or permissable content. 
NA: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity).
OK: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. 
NA: Header files must be in a -devel package. 
NA: Static libraries must be in a -static package. 
NA: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability). 
NA: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. 
NA: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} 
NA: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.
NA: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. 
OK: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. 
OK: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT). 
OK: All filenames in rpm packages must be valid UTF-8.

Thanks for looking at this.  I've updated to 6.17.

WRT .htaccess, I'm just basically trying to stay close to upstream.  I've heard a lot from Drupal users on both sides of the issue.

WRT the files migrator script, it's in doc because it's only to be run be admins who've read the directions, essentially.  It's actually not even really to be run in most cases, simply to be used as a guideline for manual action.  I'm actually not entirely sure it's needed here, in a new package.

WRT the license, the code doesn't specify, but refers the reader to the LICENSE.TXT, which specifies GPLv2.  I could see using GPLv2, but not just GPL.  Updated.

SPEC: http://zanoni.jcomserv.net/fedora/drupal6/drupal6.spec
SRPM: http://zanoni.jcomserv.net/fedora/drupal6/drupal6-6.17-1.fc13.src.rpm

Are drupal6 modules going to be handled through namespace, like drupal6-cck?

That was my plan, in lieu of any alternative.  That seems to work acceptably for the drupal modules, and allows for individual updates.

Of course, since I maintain all the drupal modules (I think) as well as drupal, "plan" may be a bit strong.  I'm always open to suggestions. :)  I took a gander at the templates you linked to, and they look useful.  I don't think we currently package any themes, as of this writing.

(In reply to comment #5)
> Thanks for looking at this.  I've updated to 6.17.
> 
> WRT .htaccess, I'm just basically trying to stay close to upstream.  I've heard
> a lot from Drupal users on both sides of the issue.

worksforme


> 
> WRT the files migrator script, it's in doc because it's only to be run be
> admins who've read the directions, essentially.  It's actually not even really
> to be run in most cases, simply to be used as a guideline for manual action. 
> I'm actually not entirely sure it's needed here, in a new package.

Completely up to your discretion, I was just curious. 

> 
> WRT the license, the code doesn't specify, but refers the reader to the
> LICENSE.TXT, which specifies GPLv2.  I could see using GPLv2, but not just GPL.
>  Updated.
   


So http://fedoraproject.org/wiki/Licensing
Contains the following text: 

A GPL or LGPL licensed package that lacks any statement of what version that it's licensed under in the source code/program output/accompanying docs is technically licensed under *any* version of the GPL or LGPL, not just the version in whatever COPYING file they include. 


So grepping (-ir) source code for license or GPL  and ignoring LICENSE.txt I get the following results: 
./includes/xmlrpc.inc: * This version is made available under the GNU GPL License
./index.php: * All Drupal code is released under the GNU General Public License.
./misc/jquery.form.js: * Dual licensed under the MIT and GPL licenses:  (NOTE: this is a completely different program)
./misc/jquery.js: * and GPL (GPL-LICENSE.txt) licenses.  (NOTE: this is a completely different program)

Based on the above from the Licensing page (which I understand came from the FSF) the license field should GPL+ (I also installed to see if perhaps the program referenced LICENSE.txt and pulled in the license in 'program output' but to the best of my knowledge it does not.) And the fact that in source code/program output/accomplanying docs that there is no version declaration is how I arrived there. I fully agree that their intention is GPLv2, however the implementation has been GPL+. 


Also I still see this when building the RPM:
warning: File listed twice: /usr/share/drupal6/.htaccess

Fixed license tag, you make a good case.  Do you know an elegant way to fix the duplicate listing, or just drop the wildcard and enumerate the rest of the contents of the directory?

Butting in since there's been no response. I think dropping the wildcard and enumerating is probably the cleanest route.

(In reply to comment #9)
> Fixed license tag, you make a good case.  Do you know an elegant way to fix the
> duplicate listing, or just drop the wildcard and enumerate the rest of the
> contents of the directory?    


Paul: Thanks for bumping this back up on my radar. 

Jon: I don't know a nice elegant way to do so - but I do honestly wonder about saying it's config file which is a possible source of the problem. While I don't necessarily disagree - I wonder if in the future (since you are relying on the .htaccess file for some measure of security, if it's not a better idea to drop %config(noreplace) Directory structure would then be left open to problems. 

If you think that it is a config file, then I think you'd have to move it to /etc/ and symlink 

http://fedoraproject.org/wiki/PackagingGuidelines#Configuration_files

The above says /usr shouldn't contain config files. 

Also can you post a link to updated spec and srpm?

SPEC: http://zanoni.jcomserv.net/fedora/drupal6/drupal6.spec
SRPM: http://zanoni.jcomserv.net/fedora/drupal6/drupal6-6.17-2.fc13.src.rpm    

David, you raise an interesting point.  Looking at said file, I don't really see a reason for it to be a config file.  If you(the admin) want to add directives, could you not craft your own file and drop it in /etc/httpd/conf.d/?

That's certainly what I'd do.

With the full knowledge that Jon is indeed doing the work here and I'm just a bystander (although an interested one!) :-) I would note that if this .htaccess file is meant to be altered, it really should live in %{_sysconfdir}.  Out of the box, I would guess a substantial number of users will edit it to allow URL rewrites, which is really desirable in lots of places.

Symlinking this to something like %{_sysconfdir}/httpd/conf.d/drupal6-site.conf would suffice, right?  I sure wouldn't advocate trying to munge the content into the existing drupal.conf file, seems like an unnecessary maintenance load.

Any further progress on this one?

Jon: 

Just trying to push this back up. Any thoughts on this? 

I'll add that if I were to vote on a solution, encapsulating the .htaccess in the .conf file would be my choice, but I am not doing the work.

Sorry for the delays, life and all.  I also deliberated on the merits of Plan Frields vs. Plan Nalley for a bit too long.

After much internal bikeshedding, here's an updated version, implementing Plan Frields.

SPEC: http://zanoni.jcomserv.net/fedora/drupal6/drupal6.spec
SRPM: http://zanoni.jcomserv.net/fedora/drupal6/drupal6-6.19-1.fc13.src.rpm    

I hate packaging webapps.  But someone has to. :(

Well, now I get to both eat crow and be hoist with my own petard.  This error now happens using the new /etc/httpd/conf.d/drupal6-site.conf:

Syntax error on line 103 of /etc/httpd/conf.d/drupal6-site.conf:
RewriteBase: only valid in per-directory config files

Which, AIUI, means the rewrite *has* to be done in /usr/share/drupal6.  Jon, my apologies for not realizing that and for causing you extra work.  I hereby lash myself 50 times with a wet noodle and encourage you to ignore me in this bug from here on out. :-)

So the problem is that the .htaccess file bears .conf at the tail of the file name and is in /etc/httpd/conf.d/ Any file bearing .conf in that directory apache will treat as a part of it's configuration, and try to parse it on startup. If you want to continue on this route (and nothing wrong with it) essentially your choices are to move it to /etc/drupal6 or rename the file to end in something other than .conf (perhaps drupal6-site.htaccess)

Thanks for figuring out this problem, David.  So this spec file should work then?

http://pfrields.fedorapeople.org/temp/drupal6.spec
http://pfrields.fedorapeople.org/temp/drupal6-6.19-1.fc13.src.rpm

(Posted these to save Jon 5 minutes... every little bit helps, right?)

Also, one other thing -- we probably need to get file contexts added to the SELinux targeted policy for the contents of /usr/share/drupal6, /etc/drupal6, and /var/lib/drupal6.  Dan, should I file a separate bug for that?  And is this something you can add in advance of, or during, a package approval?

(In reply to comment #20)
> Thanks for figuring out this problem, David.  So this spec file should work
> then?
> 
> http://pfrields.fedorapeople.org/temp/drupal6.spec
> http://pfrields.fedorapeople.org/temp/drupal6-6.19-1.fc13.src.rpm
> 
> (Posted these to save Jon 5 minutes... every little bit helps, right?)

That looks sane on first blush - I'll try it out tomorrow.

(In reply to comment #21)
> Also, one other thing -- we probably need to get file contexts added to the
> SELinux targeted policy for the contents of /usr/share/drupal6, /etc/drupal6,
> and /var/lib/drupal6.  Dan, should I file a separate bug for that?  And is this
> something you can add in advance of, or during, a package approval?


since there are only three directories, would it be better/easier to use semanage like this in %pre?? (I am partially asking this for my own edification, I don't claim to know the correct answer)

semanage fcontext -a -t httpd_var_run_t %{_sysconfdir}/%{name}/ > /dev/null 2>&1 || :
semanage fcontext -m -t httpd_var_run_t %{_sysconfdir}/%{name}/ > /dev/null 2>&1 || :

I don't see this addressed in the packaging guidelines anywhere.

No these should be in the policy package.


Fixed in selinux-policy-3.8.8-17

Jon: 

Thoughts on Paul's changes?? They look sane to me and work, and provided you have no objection, I am inclined to approve the package.

I approve, they look good.  If you'll do the honors, I'll move forward.  Thanks for the review, and your patience therewith.

APPROVED

New Package SCM Request
=======================
Package Name: drupal6
Short Description: An open-source content-management platform
Owners: limb
Branches: EL-6 EL-5
InitialCC: 

Does this need to have a devel branch?  I mean, it won't hurt, but. . .


Thanks all!

Git done (by process-git-requests).

There will be a devel branch automatically. 
You should mark it a dead.package and get it blocked there, since this is only for
EPEL.

FYI, this is waiting on this:

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

A small change that has to be made to the package before its imported is that the package needs to make and own /etc/drupal6/all/{modules,themes} and what ever extra directories may be required there.

Good timing, I was just trying the manual fix from 619979, and I can't get it to build.  I'll add that change.

drupal6-6.19-1.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/drupal6-6.19-1.el5

Built for EL-5 and EL-6, Bodhi'd for EL-5.  Thanks Jesse!

drupal6-6.19-1.el5 has been pushed to the Fedora EPEL 5 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update drupal6'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/drupal6-6.19-1.el5

drupal6-6.19-1.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.