Bug 1121601

HI, this is an informal review

fedora-review was not able to find the following files:
INFO: No upstream for (Source3): rt.conf.in
INFO: No upstream for (Source1): README.tests
INFO: No upstream for (Source4): README.fedora.in
INFO: No upstream for (Source5): rt.logrotate.in

so I was not able to check them with the build (note that there is no Source2).

and here is the annotated output of fedora-review:

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed

==== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "Unknown or generated". 202 files have unknown license. Detailed output
     of licensecheck in /export/home/dnichols/fr/review-rt/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.

the rt-tests and perl-RT-Test packages don't contain the license file, but they are test packages, and since this is based on the already-approved rt3 package, I assume that this is OK.

[x]: Package must own all directories that it creates.
     Note: Directories without known owners: /etc/logrotate.d,
     /usr/libexec/perl5-tests

these are OK:
https://fedoraproject.org/wiki/User:Johannbg/Packaging/LogFiles
also the perl5-tests directory is a shared directory designed using macros in the spec file

[ ]: Package does not own files or directories owned by other packages.
     Note: Dirs in package are owned also by:
     /usr/share/perl5/vendor_perl/RT/Interface/Web(rt3),
     /usr/share/perl5/vendor_perl/RT/Shredder/Plugin/Base(rt3),
     /usr/share/perl5/vendor_perl/RT/Interface/Web/QueryBuilder(rt3),
     /usr/share/perl5/vendor_perl/RT/Condition(rt3),
     /usr/share/perl5/vendor_perl/RT/Crypt(rt3),
     /usr/share/perl5/vendor_perl/RT/Interface/Email(perl-RT-Extension-
     CommandByMail, rt3), /usr/share/perl5/vendor_perl/RT/Approval/Rule(rt3),
     /usr/share/perl5/vendor_perl/RT/Graph(rt3),
     /usr/share/perl5/vendor_perl/RT/Interface(perl-RT-Extension-
     CommandByMail, rt3), /usr/share/perl5/vendor_perl/RT/Approval(rt3),
     /usr/share/perl5/vendor_perl/RT/Interface/Email/Auth(rt3),
     /usr/share/perl5/vendor_perl/RT/Shredder/Plugin(rt3),
     /usr/share/perl5/vendor_perl/RT/Search(rt3),
     /usr/share/perl5/vendor_perl/RT/I18N(rt3),
     /usr/share/perl5/vendor_perl/RT/Shredder(rt3),
     /usr/share/perl5/vendor_perl/RT/CustomFieldValues(rt3),
     /usr/share/perl5/vendor_perl/RT/Report(rt3),
     /usr/share/perl5/vendor_perl/RT/URI(rt3),
     /usr/share/perl5/vendor_perl/RT/Report/Tickets(rt3),
     /usr/share/perl5/vendor_perl/RT/Action(rt3),
     /usr/share/perl5/vendor_perl/RT(perl-RT-Client-REST, perl-RT-Test, perl-
     RT-Extension-CommandByMail, perl-RT-Authen-ExternalAuth, rt3)

rt replaces rt3, so these are OK


[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[ ]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.

rt.spec:
Obsoletes:      rt3 < %{version}-%{release}

I understand that it's better to hardcode the version in the Obsoletes line - to quote a more experienced fedora reviewer (Michael Schwendt): "typically one hardcodes a specific maximum version-release in the Obsoletes tag to be really accurate and not obsolete more than necessary (e.g. if %version increments, the Obsoletes tag would adjust and also obsolete newer versions than what had been specified originally)."

Therefore maybe this should be
Obsoletes:      rt3 < 4.0.21

[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 931840 bytes in 39 files.
[x]: Package complies to the Packaging Guidelines

I base my opinion to the previous successful package review for rt3 for this and other issues:
https://bugzilla.redhat.com/show_bug.cgi?id=169247

Perl:
[x]: Package contains the mandatory BuildRequires and Requires:.

I'm not sure why fedora-review flags this issue.
According to:
http://fedoraproject.org/wiki/Packaging:Perl#Testing_and_Test_Suites

the spec file should be OK, and also since it's based on rt3, I assume it's OK.

===== SHOULD items =====

Generic:
[!]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
     Note: Incorrect Requires : /usr/share/fonts/google-droid/DroidSans.ttf,
     /usr/share/fonts/google-droid/DroidSansFallback.ttf
     See: http://fedoraproject.org/wiki/Packaging/Guidelines#FileDeps

I guess I don't understand this, because the rt soure package provides these files, so I don't understand why they are listed as Requires and BuildRequires as well.

for example, I could not build the RPMs due to the following error:

dnichols@xbox:~/fr$ rpmbuild --recompile rt-4.0.21-1.fc22.src.rpm 
Installing rt-4.0.21-1.fc22.src.rpm
	/usr/share/fonts/google-droid/DroidSansFallback.ttf is needed by rt-4.0.21-1.fc20.noarch
	/usr/share/fonts/google-droid/DroidSans.ttf is needed by rt-4.0.21-1.fc20.noarch


[ ]: Avoid bundling fonts in non-fonts packages.
     Note: Package contains font files

If these are useful fonts, then maybe they should be packaged with other fedora fonts or in a separate font package?

[x]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.

COPYING (GPLv2+) is included, not sure why fedora-review doesn't see it.

[ ]: Final provides and requires are sane (see attachments).

I assume they are but was unable to test the build.

[x]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in rt-mailgate

I assume this is OK since it's Perl-based.

[ ]: Package functions as described.

I was not able to test the build due to issues reported above.

[x]: Latest version is packaged.

I get your logic for packaging an older build, and I trust your judgement :).

[x]: Package does not include license text files separate from upstream.
[ ]: Patches link to upstream bugs/comments/lists or are otherwise justified.

your patch lines do not have any comments:
http://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

I'm not sure how important this is because other more experienced packagers have also ignored this directive.  Anyway the patch names are self-explanatory.  As a newbie I would comment these lines, but as an experienced packager, I assume you know what you're doing :)

[x]: Scriptlets must be sane, if used.
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[ ]: Package should compile and build into binary rpms on all supported
     architectures.

I was not able to test this due to issues reported above.

[ ]: %check is present and all tests pass.

I was not able to test this due to issues reported above.

[x]: Packages should try to preserve timestamps of original installed files.

perms are generally set manually, the testsuite does not try to preserve permissions, but I guess that's OK

Built/smoke-tested the 4.0.21 package on EL7 and it looks good.  My EL7 packages are here:

  https://www.bfccomputing.com/downloads/fedora/rt/el7/rt4/

>	/usr/share/fonts/google-droid/DroidSansFallback.ttf is needed by rt-4.0.21-1.fc20.noarch
>	/usr/share/fonts/google-droid/DroidSans.ttf is needed by rt-4.0.21-1.fc20.noarch

Should we just require google-droid-sans-fonts, which contains these font files?

Also, Ralf, I think you were going for this:

66c66
< Provides:       rt3 = %{version}-%{release}
---
> Provides:       rt = %{version}-%{release}
270c270
< Provides:       rt3-mailgate = %{version}-%{release}
---
> Provides:       rt-mailgate = %{version}-%{release}
298c298
< Provides:       rt3-tests = %{version}-%{release}
---
> Provides:       rt-tests = %{version}-%{release}

(In reply to Bill McGonigle from comment #2)
> Built/smoke-tested the 4.0.21 package on EL7 and it looks good.  My EL7
> packages are here:
> 
>   https://www.bfccomputing.com/downloads/fedora/rt/el7/rt4/
> 
> >	/usr/share/fonts/google-droid/DroidSansFallback.ttf is needed by rt-4.0.21-1.fc20.noarch
> >	/usr/share/fonts/google-droid/DroidSans.ttf is needed by rt-4.0.21-1.fc20.noarch
> 
> Should we just require google-droid-sans-fonts, which contains these font
> files?
No. rt accesses the *.ttf files directly through hard-coded paths, 
i.e. just requiring google-droid-sans-fonts would not be sufficient, because the package providing the *ttf files could change at any time and because the path these *ttf files are being installed could change at any time.

> Also, Ralf, I think you were going for this:
> 
> 66c66
> < Provides:       rt3 = %{version}-%{release}
> ---
> > Provides:       rt = %{version}-%{release}
> 270c270
> < Provides:       rt3-mailgate = %{version}-%{release}
> ---
> > Provides:       rt-mailgate = %{version}-%{release}
> 298c298
> < Provides:       rt3-tests = %{version}-%{release}
> ---
> > Provides:       rt-tests = %{version}-%{release}
Could you elaborate? I do not understand.

Each package is supposed to provide "rt* = %{version}-%{release}"
and the corresponding "rt3-* = %{version}-%{release}".

This is what they do:
rt-4.0.21-1.fc22.noarch.rpm:
rt = 4.0.21-1.fc22
rt3 = 4.0.21-1.fc22

rt-mailgate-4.0.21-1.fc22.noarch.rpm:
rt-mailgate = 4.0.21-1.fc22
rt3-mailgate = 4.0.21-1.fc22

rt-tests-4.0.21-1.fc22.noarch.rpm:
rt-tests = 4.0.21-1.fc22
rt3-tests = 4.0.21-1.fc22

(In reply to David Nichols from comment #1)

> I was not able to test the build due to issues reported above.
> 
> [x]: Latest version is packaged.
> 
> I get your logic for packaging an older build, and I trust your judgement :).
It's simply that I haven't yet managed to get rt-4.2 working, apparently due to httpd configuration (and SELinux) issues, though I repeatedly have tried.

As I am not an expert on these topics, I would really interested in hearing from somebody who has. I guess, the origins of my problems with 4.2 are trivial.

However, provided f21 is near, this has caused me to "pull the emergency break" on rt3 and to try pushing "rt-4.0", because shipping "rt3" doesn't make any sense, anymore.

Update:

Spec URL: http://corsepiu.fedorapeople.org/packages/rt.spec
SRPM URL: http://corsepiu.fedorapeople.org/packages/rt-4.0.21-2.fc22.src.rpm

(In reply to Ralf Corsepius from comment #3)
> No. rt accesses the *.ttf files directly through hard-coded paths, 
> i.e. just requiring google-droid-sans-fonts would not be sufficient, because
> the package providing the *ttf files could change at any time and because
> the path these *ttf files are being installed could change at any time.

Wouldn't we need to patch RT then, to live in the Fedora ecosystem?  Duplicate installation of the fonts wouldn't be desirable.

> Could you elaborate? I do not understand.
> 
> Each package is supposed to provide "rt* = %{version}-%{release}"
> and the corresponding "rt3-* = %{version}-%{release}".

Ah, I think I see now that you're trying to provide a smooth upgrade path.  These are the errors from my scrollback buffer, but it may have just been because I had the last packages installed and I mis-interpreted:

[bill@apps noarch]$ sudo rpm -Uhv perl-Plack-Middleware-Test-StashWarnings-0.08-1.el7.centos.noarch.rpm rt-\*4.0.21* perl-RT-Test-4.0.21-1.el7.centos.noarch.rpm 
error: Failed dependencies:
        rt3 conflicts with (installed) rt4-4.0.8-0.20121228.0.el7.centos.noarch
        rt3-mailgate conflicts with (installed) rt4-mailgate-4.0.8-0.20121228.0.el7.centos.noarch
        rt3-tests conflicts with (installed) rt4-tests-4.0.8-0.20121228.0.el7.centos.noarch


> shipping "rt3" doesn't make any sense, anymore.

totally agree - shipping rt3 is way worse than shipping 4.0.21.  Perfect/enemy/good.

Just a note, 4.0.21 seems to have caused some breakage for me over 4.0.8 - getting ticket creation failures with Pg errors:

rt4=> SELECT main.* FROM CustomFields main JOIN ObjectCustomFields ObjectCustomFields_1  ON ( ObjectCustomFields_1.CustomField = main.id )  WHERE (ObjectCustomFields_1.ObjectId = '5' OR ObjectCustomFields_1.ObjectId = '0') AND (main.Disabled = '0') AND (main.LookupType = 'RT::Queue-RT::Ticket-RT::Transaction')  GROUP BY main.id  ORDER BY MIN(ObjectCustomFields_1.SortOrder) ASC
rt4-> ;
ERROR:  column "main.name" must appear in the GROUP BY clause or be used in an aggregate function
LINE 1: SELECT main.* FROM CustomFields main JOIN ObjectCustomFields...
               ^

4.0.8 was perfect for about a week.  I'm going to see if I can chase that one down - maybe need a newer DBIx::SearchBuilder?

Just in case anybody runs into this, my install of 4.0.21 has a problem working with Pg - mysql code paths are called in some cases (reason currently unknown) which causes sql failures and various things don't work or work intermittently(?!).  I don't see any problems in the SPEC, but Alex at bestpractical has suggested looking at the packaging.

I rolled back to 4.0.8 for now while I look into it and that's perfect.

(In reply to Bill McGonigle from comment #7)
> Just in case anybody runs into this, my install of 4.0.21 has a problem
> working with Pg - mysql code paths are called in some cases (reason
> currently unknown) which causes sql failures and various things don't work
> or work intermittently(?!).
My package does not support Pg.

Next update:
Spec URL: http://corsepiu.fedorapeople.org/packages/rt.spec
SRPM URL: http://corsepiu.fedorapeople.org/packages/rt-4.0.21-3.fc22.src.rpm


Note: I have retired the rt3 package for f21 and rawhide. Unless this package makes it into Fedora, there won't be any rt in Fedora's next release.

Next update:
Spec URL: http://corsepiu.fedorapeople.org/packages/rt.spec
SRPM URL: http://corsepiu.fedorapeople.org/packages/rt-4.0.21-4.fc22.src.rpm

Another update:

Spec URL: http://corsepiu.fedorapeople.org/packages/rt.spec
SRPM URL: http://corsepiu.fedorapeople.org/packages/rt-4.0.22-1.fc22.src.rpm

Hi, I just upgraded to F21 and (obviously) it broke my rt(3), so to my dismay I come here and see there is no rtX in F21 (yet?).  That may be a bit disturbing and suprising for upgraders.

Anyhow, what's the easiest way for me to rt back on my system.  I realize it will be rt4.  Should I just grab your src/spec you provide here and build it up myself?  Will the fc22 srpms work on fc21?

What's going to be the ongoing plan for rt on F21?  What's the best way for me to ensure I stay patched?

Thanks!

(In reply to Trevor Cordes from comment #12)
> Hi, I just upgraded to F21 and (obviously) it broke my rt(3), so to my
> dismay I come here and see there is no rtX in F21 (yet?).  That may be a bit
> disturbing and suprising for upgraders.
Correct. rt3 is dead, gone and abandoned upstream, and should be removed anywhere (comprising EPEL and Fedora), because running it is a security risk.

I had hoped this package was ready in time to replace rt3, unfortunately there doesn't seem to be sufficient interest in rt in Fedora to provide a review :(

> Anyhow, what's the easiest way for me to rt back on my system.  I realize it
> will be rt4.  Should I just grab your src/spec you provide here and build it
> up myself?
That's what I do - I am using them ;)

>  Will the fc22 srpms work on fc21?
There also are fc21 versions on https://corsepiu.fedorapeople.org/packages

I also have older versions of them on the packman repo (ftp://packman.links2linux.de/pub/packman/fedora).
Likely I'll sync them with my Fedora submission.

> What's going to be the ongoing plan for rt on F21?
There is no plan - It's just that this review is stuck ;)

>  What's the best way for
> me to ensure I stay patched?
It certainly would be best to provide a review, such that these packages can be provided through Fedora :)

Ralf:

I happen to have a need for an up-to-date RT in Fedora, and I have some time.  I will work on this review, but if there's anything else that needs to be reviewed before the RT stack can make it into Fedora, please let me know.

It's been some time since I did a package review, so please bear with me.

And while I dig into the package, I guess my first question would be whether you've had any better luck with 4.2 in the intervening months?  Is the issue just selinux or is it more complicated?  (Assuming it gets more complicated than selinux....)  Once this is done, I would be happy to test 4.2 packages if you have them.

Sorry for replying a little late, but I missed this posting ;)

(In reply to Jason Tibbitts from comment #15)
> And while I dig into the package, I guess my first question would be whether
> you've had any better luck with 4.2 in the intervening months?
Yes, I had. I am still having selinux problems,
c.f. https://bugzilla.redhat.com/show_bug.cgi?id=1167412
but things look much better now.

>  Is the issue
> just selinux or is it more complicated?  (Assuming it gets more complicated
> than selinux....)  Once this is done, I would be happy to test 4.2 packages
> if you have them.
I also have 4.2.x rpms (Actually for quite a while):
https://corsepiu.fedorapeople.org/rt-4.2.x/

Oh, good.  For me personally it's no big deal to write a bunch of selinux rules to make it work.  A full list of even the less useful command-truncated AVCs might help, though.  I guess I can have a look after I get the packages installed.

Anyway, regarding this package, most of the review work is actually done.  The package is extremely clean for its complexity, though given currently supported Fedora versions, I would probably strip some of the version conditionals.  I guess, though, there's always a chance that someone will try to build on RHEL4 or something and the conditionals could make it obvious that it won't work.

The only other thing I think I'd point out is the use of __rm is kind of odd, since you don't use any other macro-ized executables besides __perl.  Why not just use rm?  Or is this something related to the usrmove thing?  (I.e. you don't know if you can require /bin/rm or /usr/bin/rm.)

So, some other random bits:

rpmlint complains about a few macros in comments.  It looks like on at least some of the comments you tried to escape the macros, so I'm not sure if these were merely oversights or if I'm missing something.  For example:

  # Install upgrade/ into %%{_datadir}/%{name}/upgrade

(these are on lines 395, 465 and 466).

You place one file in /etc/httpd/conf.d, but the package has no dependency on httpd.  Not sure what to do here; I guess I'd suggest owning /etc/httpd and /etc/httpd/conf.d if indeed the package is capable of functioning without a web server.  (The included standalone_httpd command would suggest that's the case.)  Or, I suppose, split that file into a separate apache subpackage that also depends on apache.  I guess that seems like a mess, but the rt-mailgate package is pretty small as well.

In any case, this is extremely close.

(In reply to Jason Tibbitts from comment #17)
> Anyway, regarding this package, most of the review work is actually done. 
> The package is extremely clean for its complexity, though given currently
> supported Fedora versions, I would probably strip some of the version
> conditionals.  I guess, though, there's always a chance that someone will
> try to build on RHEL4 or something and the conditionals could make it
> obvious that it won't work.
Well, chances to build the package on any RHEL releases are quite low, because due to the amount of deps on fairly new versions of perl-modules, chances are high to trip over a perl-module whose version is stuck at a particular version in RHEL, thanks to the Core <-> Extra split.

In its current shape the packages should be applicable to Fedora 20, 21 and rawhide, with chances to make them functional on EPEL7 being high.

> The only other thing I think I'd point out is the use of __rm is kind of
> odd, since you don't use any other macro-ized executables besides __perl. 
> Why not just use rm?  Or is this something related to the usrmove thing?
I don't recall the details - Could be historical cruft ;)
But I guess the primary reason is me considering non-absolute paths in scriptlets to be unsafe and unreliabile (I don't know if this consideration is still valid).

(In reply to Jason Tibbitts from comment #18)
> rpmlint complains about a few macros in comments.
Which version of the package are you looking into? 

>   # Install upgrade/ into %%{_datadir}/%{name}/upgrade
> 
> (these are on lines 395, 465 and 466).
OK, you seem to be looking at the 4.0.x version.

> You place one file in /etc/httpd/conf.d, but the package has no dependency
> on httpd.
httpd is indirectly being pulled in, via some perl/apache-module/plugin.

>  Not sure what to do here; I guess I'd suggest owning /etc/httpd
> and /etc/httpd/conf.d if indeed the package is capable of functioning
> without a web server.
In its current configuration, the package requires httpd (More precisely: apache).

>  (The included standalone_httpd command would suggest
> that's the case.)  Or, I suppose, split that file into a separate apache
> subpackage that also depends on apache.
Never tried this, not sure it this is feasible ;)

>  I guess that seems like a mess, but
> the rt-mailgate package is pretty small as well.
rt-mailgate is a bit special. It was split out from the main-package on public demand many years ago, because it can be run on a different machine than rt itself and people actually were using this.

I'll update package and keep you posted.

FYI rt-4.0.22 does not build for EPEL7:
...
Error: No Package found for /usr/share/fonts/google-droid/DroidSans.ttf
Error: No Package found for /usr/share/fonts/google-droid/DroidSansFallback.ttf
Error: No Package found for perl(CGI::PSGI)
Error: No Package found for perl(Cache::Simple::TimedExpiry)
Error: No Package found for perl(Class::ReturnValue) >= 0.40
Error: No Package found for perl(Convert::Color)
Error: No Package found for perl(DBIx::SearchBuilder) >= 1.59
Error: No Package found for perl(Data::ICal)
Error: No Package found for perl(HTML::Mason) >= 1.43
Error: No Package found for perl(HTML::Mason::PSGIHandler)
Error: No Package found for perl(HTML::Quoted)
Error: No Package found for perl(HTML::RewriteAttributes) >= 0.02
Error: No Package found for perl(HTTP::Server::Simple::Mason) >= 0.09
Error: No Package found for perl(Imager)
Error: No Package found for perl(Imager::File::GIF)
Error: No Package found for perl(Imager::File::JPEG)
Error: No Package found for perl(Imager::File::PNG)
Error: No Package found for perl(Locale::Maketext::Fuzzy)
Error: No Package found for perl(Locale::Maketext::Lexicon) >= 0.32
Error: No Package found for perl(Log::Dispatch::Perl)
Error: No Package found for perl(Plack::Handler::Starlet)
Error: No Package found for perl(Plack::Middleware::Test::StashWarnings) >= 0.06
Error: No Package found for perl(Regexp::Common::net::CIDR)
Error: No Package found for perl(Term::EditorEdit)
Error: No Package found for perl(Test::Email)
Error: No Package found for perl(Test::Expect) >= 0.31
Error: No Package found for perl(Test::HTTP::Server::Simple) >= 0.09
Error: No Package found for perl(Test::WWW::Mechanize)
Error: No Package found for perl(Test::WWW::Mechanize::PSGI)
Error: No Package found for perl(Text::Password::Pronounceable)
Error: No Package found for perl(Text::Quoted) >= 2.02
Error: No Package found for perl(Text::WikiFormat) >= 0.76
Error: No Package found for perl(Text::Wrapper)
Error: No Package found for perl(Tree::Simple) >= 1.04
Error: No Package found for perl(Web::Scraper)
...
Lots of perl-modules are missing.

Yet another update:

Spec URL: http://corsepiu.fedorapeople.org/packages/rt.spec
SRPM URL: http://corsepiu.fedorapeople.org/packages/rt-4.0.22-2.fc22.src.rpm

Changes/Remarks:
- The rt package now requires: /etc/http.d/conf.d
  (The same approach as eg. applied to perl-HTML-Mason)
- The running testsuite now only exposes 1 failure
  (Less than any other rt package did before!)
- The SELinux issue with rt-4.0.22/mysql now seems to be resolved
  (Cf. README.fedora[.in] and RHBZ#1184792)
- Many small packaging changes.
  Most visible: "rpmbuild --with pg" doesn't not require
  "--without mysql" anymore (as it used to do).

Created attachment 982893 [details]
Log from running rt-4.0.22-2.f21's testsuites

Did you want me to work on the 4.2.9 version instead?  If we can have it in Fedora it would be great, just so that we don't start out behind.

For selinux stuff, you _could_ make the relevant semanage call in %post, or make a separate selinux subpackage that does nothing but call that.  Not sure it's worth it; honestly I'd just document the one semanage call needed to fix the policy problem and move on.  The package has nontrivial setup requirements in any case.

Sorry, I didn't notice the indirect httpd dependency.  I didn't notice it was installed at build time.  (My test scripts install the built packages in the buildroot and notice any additional dependencies.)  Making it explicit helps in any case.

I think that one test fails because it assumes the files will be under */share/html but they're really under *share/rt/html.  I think if you just patch t/web/query_log.t and change this line:


$m->text_like(qr{share/html/autohandler:\d+}, "stack trace includes mason components");
to
$m->text_like(qr{share/rt/html/autohandler:\d+}, "stack trace includes mason components");

then all the tests will pass.  Pretty sure this is just a bug in the test suite, assuming details about how the package was installed which it shouldn't.

At this point I think the 4.0.22 version is good to go, and I've set the flag accordingly.  I will go ahead and look into the 4.2.9 version instead if you like.  I don't know if it needs the same tweaks you made in comment 21.

(In reply to Jason Tibbitts from comment #23)
> I think that one test fails because it assumes the files will be under
> */share/html but they're really under *share/rt/html.  I think if you just
> patch t/web/query_log.t and change this line:
> [snip]
> then all the tests will pass.  Pretty sure this is just a bug in the test
> suite, assuming details about how the package was installed which it
> shouldn't.

Good catch --thanks.  I've applied https://github.com/bestpractical/rt/commit/2d00627e on 4.2-trunk, so this will be fixed in the next upstream 4.2 release.

I've also pushed a branch to address the Class-Accessor dependency error (0003-Broken-test-dependencies.patch), which will be in the next 4.0 release.

 - Alex

Had no idea you were watching, Alex.  Glad to know you folks are paying attention, and hope you're happy that we're finally going to get an up-to-date RT into Fedora.

Ralf, also, do you have any idea about packaging RT plugins?  It might be too much of a mess to get into (as you'd have to worry about plugin compatibility for every RT update) but it might be nice to have some of them.

(In reply to Jason Tibbitts from comment #25)
> Had no idea you were watching, Alex.  Glad to know you folks are paying
> attention, and hope you're happy that we're finally going to get an
> up-to-date RT into Fedora.

Absolutely.  If there's anything we can do to make the packaging process easier -- or any other bugs you find that should be fixed upstream -- let us know.

> Ralf, also, do you have any idea about packaging RT plugins?  It might be
> too much of a mess to get into (as you'd have to worry about plugin
> compatibility for every RT update) but it might be nice to have some of them.

Within a series (4.0.x or 4.2.x) plugin compatibility will not change; we're committed to not breaking backwards compatibility within stable series.  All of the common plugins now publish metadata in their META.yml about which RT versions they're compatible with; see the "rt_too_new" and "requires_rt" keys in, for example, https://metacpan.org/source/ALEXMV/RT-Extension-SLA-1.04/META.yml

 - Alex

(In reply to Jason Tibbitts from comment #23)
> Did you want me to work on the 4.2.9 version instead?
No. The 4.0.x version is OK. It's in much better shape and better tested than the 4.2.x version, which was more or less snapshot from a quick private stab at packaging it. I only uploaded it, because people were asking me for it on PM.

>  If we can have it in
> Fedora it would be great, just so that we don't start out behind.
My plan is to revisit 4.2.x and to upgrade at least rawhide to it, ASAP.

[This all would not have been a problem if this review had worked out better than it did - One lesson learnt: Fedora's rename reviews are a PITA. Never use versioned package names - You'll regret it ;)]

> For selinux stuff, you _could_ make the relevant semanage call in %post,
Yep, but you surely know, we discourage people from doing ;)

> The package has nontrivial setup
> requirements in any case.
ACK, I'll give the selinux folks a couple of days to respond to my RFE and then will decide what to do.

> I think that one test fails because it assumes the files will be under
> */share/html but they're really under *share/rt/html.  I think if you just
> patch t/web/query_log.t and change this line:
Great catch - Patch applied!

> At this point I think the 4.0.22 version is good to go, and I've set the
> flag accordingly.
sigh .... finally ... thank you, very much!

>  I will go ahead and look into the 4.2.9 version instead
> if you like.  I don't know if it needs the same tweaks you made in comment
> 21.
It does. I keep branches of both in a local git and still have to merge some changes. Also, rt-4.2.x has seen much less testing than rt-4.0.x and IIRC, suffers from different SELinux issues.

New Package SCM Request
=======================
Package Name: rt
Short Description: Request tracker
Upstream URL: http://www.bestpractical.com/rt
Owners: corsepiu
Branches: f21
InitialCC: perl-sig

Git done (by process-git-requests).

rt-4.0.22-3.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/rt-4.0.22-3.fc21

rt-4.0.22 now is in rawhide and f21's package queue. Thanks to tibbs' finding, we're now at 0 testsuite failures on f21!

Should somebody still be interested in continuing with rt-4.2.9, I've uploaded new rt-4.2.9 rpms to http://corsepiu.fedorapeople.org/packages

There are 2 testsuite failures with these rpms on f21. Unfortunately, I can't spot anything obvious.

Created attachment 983414 [details]
Log from running rt-4.2.9-0.20150123.0.fc21's testsuites

Huzzah for 4.0 in Fedora!

The first test failure on 4.2 is due to something _not_ failing as it does everywhere else.  That is, RT uses the HTML::FormatText::WithLinks::AndTables module to render HTML to plain text -- which unfortunately fails spectacularly if there are tables involved.  Does Fedora have local patches applied to the HTML::FormatText module, or HTML::FormatText::WithLinks::AndTables ?

The second failure (with the web-based installer) is also odd -- it's a failure to create a test SQLite DB.  What does adding a "die $m->content;" on line 81 yield?
 - Alex

(In reply to Alex Vandiver from comment #33)
Thanks, for your hints, Alex!

> The first test failure on 4.2 is due to something _not_ failing as it does
> everywhere else.  That is, RT uses the
> HTML::FormatText::WithLinks::AndTables module to render HTML to plain text
> -- which unfortunately fails spectacularly if there are tables involved. 
> Does Fedora have local patches applied to the HTML::FormatText module, or
> HTML::FormatText::WithLinks::AndTables ?
You are right on the spot!

Fedora's perl-HTML-FormatText-WithLinks-AndTable has 2 patches applied:
c.f. http://pkgs.fedoraproject.org/cgit/perl-HTML-FormatText-WithLinks-AndTables.git/tree/

Reverting one of these (col_0_fix.patch), lets the t/mail/html-outgoing.t test succeed - Unfortunately, this patch isn't documented at all - No idea what it is trying to address nor about its origins :(

=> Something for me to bugzilla.


> The second failure (with the web-based installer) is also odd -- it's a
> failure to create a test SQLite DB.  What does adding a "die $m->content;"
> on line 81 yield?
A pretty complex html page with embedded js. I am going to attach it.

Created attachment 983480 [details]
$m->content as requested in comment#31

Contents of t/tmp/web-install.t-*/rt.debug.log:
[3022] [Fri Jan 23 17:35:34 2015] [warning]: DBI connect('dbname=rt4test;host=localhost','urt4test',...) failed: Unknown database 'rt4test' at /usr/share/perl5/vendor_perl/DBIx/SearchBuilder/Handle.pm line 105. (/usr/share/perl5/vendor_perl/Carp.pm:168)
[3022] [Fri Jan 23 17:35:35 2015] [warning]: DBI connect('dbname=rt4test;host=localhost','urt4test',...) failed: Unknown database 'rt4test' at /usr/share/perl5/vendor_perl/DBIx/SearchBuilder/Handle.pm line 105. (/usr/share/perl5/vendor_perl/Carp.pm:168)

(In reply to Ralf Corsepius from comment #34)
> Fedora's perl-HTML-FormatText-WithLinks-AndTable has 2 patches applied:
> c.f.
> http://pkgs.fedoraproject.org/cgit/perl-HTML-FormatText-WithLinks-AndTables.
> git/tree/
> 
> Reverting one of these (col_0_fix.patch), lets the t/mail/html-outgoing.t
> test succeed - Unfortunately, this patch isn't documented at all - No idea
> what it is trying to address nor about its origins :(

It's from https://rt.cpan.org/Public/Bug/Display.html?id=55919#txn-753466 , which is a (IMHO) worse fix than we attempted to supply to the module author in https://rt.cpan.org/Public/Bug/Display.html?id=63555

Reverting col_0_fix.patch in Fedora is likely not the correct fix here -- the patch in perl-HTML-FormatText-WithLinks-AndTables is absolutely fixing a bug, and one that the original CPAN author has failed to address.  The RT tests are merely being pessimistic and were expecting that the module would _always_ fail; the Fedora patched version doesn't so do, which isn't really a failure of the Fedora version of the module -- we should be fixing the test to mark those tests as TODO, or not test them at all.

For the short term, you can remove lines 85-106 of t/mail/html-outgoing.t in Fedora.  I'll ponder what the most right fix is for code.

> => Something for me to bugzilla.

As noted above, I disagree that 1185427 is a bug that Fedora needs to address.


> A pretty complex html page with embedded js. I am going to attach it.

Thanks.  The core of it is "Failed to connect to database: unable to open database file".  Can you think of anything that might prevent the webserver from writing to $RT::VarPath (which I believe is /var/lib/rt under the Fedora layout)?  Presumably selinux kicking in?

 - Alex

We could pretty easily mess with the packaging of that perl module if any of this makes a difference. It appears that it's used only by publican (our docbook publication system) so we'd have to talk to them.  Maybe someone just needs to fork the module.

And selinux would definitely keep the webserver from writing to an unlabeled location under /var (or anywhere else; the web server is rather strictly confined).  Now, when I look in the current F21 policy, I see the following rt-related labels:

/var/cache/rt(3|4)(/.*)?                           all files          system_u:object_r:httpd_cache_t:s0

/var/lib/rt(3|4)/data/RT-Shredder(/.*)?            all files          system_u:object_r:httpd_var_lib_t:s0

Which makes it pretty obvious where the problems lie.

Since we're using "rt" and not "rt4" for these directories, none of this matches, and even if it were fixed, the labeling for /var/lib/rt would be a bit too restrictive, I think.

The selinux folks are very happy to tweak policy and they usually do it rather quickly.  If we could just get a list of everywhere rt is expected to write, it would be pretty easy to get them to patch things up.  Alex, would you happen to know that off the top of your head?

(In reply to Jason Tibbitts from comment #37)
> We could pretty easily mess with the packaging of that perl module if any of
> this makes a difference. It appears that it's used only by publican (our
> docbook publication system) so we'd have to talk to them.  Maybe someone
> just needs to fork the module.

Unfortunately, all of the perl-based HTML -> text converters seem to be poorly-mainatined, and prone to crashing on fairly simple input.  As a result, RT 4.2 is moving towards instead adding an optional dependency on HTML::FormatExternal, which shells out to w3m or elinks -- we're not interested in forking, developing, and supporting a text-based HTML rendering engine when there already exist several in the wild (aka "browsers").  I expect that 4.4 will drop HTML::FormatText::WithTables::AndLinks entirely.

> And selinux would definitely keep the webserver from writing to an unlabeled
> location under /var (or anywhere else; the web server is rather strictly
> confined).  Now, when I look in the current F21 policy, I see the following
> rt-related labels:
> 
> /var/cache/rt(3|4)(/.*)?                           all files         
> system_u:object_r:httpd_cache_t:s0
> 
> /var/lib/rt(3|4)/data/RT-Shredder(/.*)?            all files         
> system_u:object_r:httpd_var_lib_t:s0
> 
> Which makes it pretty obvious where the problems lie.
> 
> Since we're using "rt" and not "rt4" for these directories, none of this
> matches, and even if it were fixed, the labeling for /var/lib/rt would be a
> bit too restrictive, I think.
> 
> The selinux folks are very happy to tweak policy and they usually do it
> rather quickly.  If we could just get a list of everywhere rt is expected to
> write, it would be pretty easy to get them to patch things up.  Alex, would
> you happen to know that off the top of your head?

/var/lib/rt needs to be writable for SQLite; the database is a file named /var/lib/rt/rt4  (assuming that $DatabaseName is set to rt4).  Since SQLite is defined to be "not for production" there's some slack here in how much we care, though.

If file-based logging is enabled, writing to /var/log/rt is also necessary.  The above rules (fixed for "rt" not "rt4") cover Mason's cache.  The shredder directories also need to be writable.  I _believe_ that to be sufficient -- in the past we've simply set httpd_sys_rw_content_t on all of /opt/rt4/var, which is a big-ish hammer.

 - alex

Shelling out will spell super fun for selinux, I'm sure.

So, basically we need to fix the policy to allow writes to /var/lib/rt and /var/log/rt (which shouldn't be too difficult).  I'll bug the selinux folks.

(In reply to Alex Vandiver from comment #36)
> For the short term, you can remove lines 85-106 of t/mail/html-outgoing.t in
> Fedora.  I'll ponder what the most right fix is for code.
Done this.

> Can you think of anything that might prevent the webserver
> from writing to $RT::VarPath (which I believe is /var/lib/rt under the
> Fedora layout)?
Yes (cf. below).

>  Presumably selinux kicking in?
No, the cause of the t/web/install.t failure this time isn't SELinux. 

/var/lib/rt (aka. RT_VAR_PATH) is entirely missing. Manually creating /var/lib/rt fixes the testsuite failure.

Seems to me, as if rt's configure script misses to create it. AFAIS, this also applies to 4.0.x, but 4.2.x seems to be more frequently use RT_VAR_PATH than 4.0.x did. ATM, I am not 100% sure if 4.0.x is actually using /var/lib/rt at all ;)

Open question: Which owner:group and permissions to use for /var/lib/rt?
From what I can gather from the source code, 4.2.x seem to be wanting to use RT_USER:RT_GROUP (i.e. apache:apache), but doesn't seem to have special requirements on permissions. May /var/lib/rt contain sensitive (e.g. user-submitted personal data) or security-relevant (e.g. passwords) information?

Next version of the rt-4.9.2 rpms (rt-4.2.9-0.20150124.0) available under 
http://corsepiu.fedorapeople.org/packages

Created attachment 983633 [details]
Log from running rt-4.2.9-0.20150124.0.fc21's testsuites

rt-4.2.9-0.20150124.0.fc21 testsuite completed without any failure.

- As recommended by Alex, t/mail/sendmail-plaintext.t was silenced by removing the sub-test triggering the compatibility issue with Fedora's perl-HTML-FormatText-WithLinks-AndTables.

- t/web/install.t appears to be fixed by having adding
drwxr-xr-x    2 apache  apache                      0 Jan 24 04:53 /var/lib/rt

No further SELinux changes applied.


Unless something else should pop up, I am considering to upgrade rt to this version on rawhide.

I won't argue with that.  I wish we could just go with 4.2.9 in F21 as well, but that's entirely up to you.  It would save you from having to maintain a 4.0 branch at all.

Package rt-4.0.22-3.fc21:
* should fix your issue,
* was pushed to the Fedora 21 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing rt-4.0.22-3.fc21'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2015-1110/rt-4.0.22-3.fc21
then log in and leave karma (feedback).

rt-4.2.9-2.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/rt-4.2.9-2.fc21

Just wanted to thank you for doing the heavy lifting here.  I'm sorry I didn't see this ticket much sooner; I only recently had occasion to mess with RT again and was surprised to find it had dropped out of the distribution.  I guess there's not any other way I could have known; if you made an announcement on one of the lists, I didn't see it.

I'll try to find some time soon to test the latest update and give karma.

rt-4.2.9-2.fc21 has been pushed to the Fedora 21 testing repository.

rt-4.2.9-2.fc21 has been pushed to the Fedora 21 stable repository.