Spec URL: http://jkaluza.fedorapeople.org/passenger.spec SRPM URL: http://jkaluza.fedorapeople.org/passenger-4.0.37-1.fc21.src.rpm Description: Phusion Passenger™ — a.k.a. mod_rails or mod_rack — makes deployment of Ruby web applications, such as those built on the revolutionary Ruby on Rails web framework, a breeze. It follows the usual Ruby on Rails conventions, such as “Don’t-Repeat-Yourself”. Fedora Account System Username: jkaluza This is a re-review request for a package rename. Old package name is "rubygem-passenger". This rename has been discussed with both upstream and all current rubygem-passenger maintainers. It will happen only in rawhide. rpmlint output: > mod_passenger.x86_64: W: spelling-error %description -l en_US pluggable -> plug gable, plug-gable, plugged > passenger.x86_64: W: name-repeated-in-summary C Passenger > passenger.x86_64: W: spelling-error %description -l en_US Phusion -> Fusion > passenger.x86_64: W: devel-file-in-non-devel-package /usr/bin/passenger-config Those are false positives. > passenger.x86_64: W: no-manual-page-for-binary passenger > passenger.x86_64: W: no-manual-page-for-binary passenger-install-apache2-module > passenger.x86_64: W: no-manual-page-for-binary passenger-install-nginx-module Upstream does not provide man page for these binaries. > passenger-devel.x86_64: W: no-documentation Not a problem, passenger-devel requires main package which contains docs. > passenger-native-libs.x86_64: W: spelling-error Summary(en_US) Phusion -> Fusion > passenger.src: W: name-repeated-in-summary C Passenger > passenger.src: W: spelling-error %description -l en_US Phusion -> Fusion False positive. > passenger.src:274: E: hardcoded-library-path in %{_prefix}/lib/tmpfiles.d > passenger.src:275: E: hardcoded-library-path in %{_prefix}/lib/tmpfiles.d/%{name}.conf > passenger.src:326: E: hardcoded-library-path in %{_prefix}/lib/tmpfiles.d/%{name}.conf This is expected.
Fedora review issues: Issues: ======= > Package contains BR: python2-devel or python3-devel I have no idea what does that mean for passenger and why should I use this BR... > Static libraries in -static or -devel subpackage, providing -devel if > present. > Note: Package has .a files: passenger-devel. Does not provide -static: > passenger-devel. > See: http://fedoraproject.org/wiki/Packaging/Guidelines#StaticLibraries Passenger uses this static library to be able to build custom nginx version and run passenger using this version. This static library is in private directory and only passenger uses it when user manually asks passenger to compile custom version of nginx. > Large documentation must go in a -doc subpackage. Large could be size (~1MB) > or number of files. > Note: Documentation size is 2652160 bytes in 57 files. > See: http://fedoraproject.org/wiki/Packaging/Guidelines#PackageDocumentation This documentation is in -doc subpackage...
First quick comment - 4.0.38 is released ;) - some cleanups : most command can be used directly no need for macro (%{_rm}, ...) - no need for Buildroot and rm -rf %{buildroot} - bundled libeio => probably requires a FPC exception About obsoletes, Not needed for virtual provides, only for package => Obsoletes: rubygem(passenger) < 4.0.37 => Obsoletes: rubygem-passenger%{?_isa} < 4.0.37 Issue with version: Obsoletes: rubygem-passenger < 4.0.37 Provides: rubygem-passenger = 4.0.37 Provides: rubygem-passenger%{?_isa} = 4.0.37 This is ok, but only for now. It will only work if no update will never go in f<=20 Proposal Obsoletes: rubygem-passenger < %{version}-%{release} Provides: rubygem-passenger = %{version}-%{release} Provides: rubygem-passenger%{?_isa} = %{version}-%{release} And use 2 as release, and add a comment in rubygem-passenger in f20 to never use release > 1
Duplicate obsoletes for rubygem-passenger-native-libs (in native-libs only is enough) Duplicate obsoletes for rubygem-passenger-devel (in devel only is enough) Missing obsoletes for rubygem-passenger-doc
Thanks for your check. I will fix the problems you have found. I've asked FPC about libeio vs. libeio :) https://fedorahosted.org/fpc/ticket/403
Isn't the description outdated a bit? I thought that the rename is mainly because passenger is now far more then Ruby thing.
And I would appreciate if you can fix bug 1064822.
Moreover, I dont udnerstand why you completely dropped the rubygem- and install stuff into %{ruby_vendor{lib,arch}dir}. This is discouraged practice.
(In reply to Vít Ondruch from comment #5) > Isn't the description outdated a bit? I thought that the rename is mainly > because passenger is now far more then Ruby thing. Yes, I'm going to fix that. Upstream proposed better description text and I have it already in my local passenger.spec which will be in the new srpm I will upload to this review. (In reply to Vít Ondruch from comment #6) > And I would appreciate if you can fix bug 1064822. Will do that. (In reply to Vít Ondruch from comment #6) > And I would appreciate if you can fix bug 1064822. (In reply to Vít Ondruch from comment #6) > Moreover, I dont udnerstand why you completely dropped the rubygem- and > install stuff into %{ruby_vendor{lib,arch}dir}. This is discouraged practice. You mean why I dropped the "rubygem-" prefix and want the passenger to rename? There are following reasons pointed out by upstream: > It's currently called "rubygem-passenger", but > Passenger is no longer about Ruby only. Nowadays Passenger is a true > polyglot server, with support for Python, Node.js and Meteor as well. > Furthermore, Passenger isn't a gem in the usual sense; it's > distributed over a wide variety of channels such as tarball, deb, > HomeBrew, etc. RubyGems is just another distribution channel for us. Maintainer and other co-maintainers of rubygem-passenger agreed with this change but this decision has not resulted in any action, so I started this process. I treated Passenger as "Non-Gem Packages" category in the Ruby Packaging Guidelines and got the feeling that I should use ruby_vendor* macros. However, if you really think it should be installed into different directories, I can change that, since it's clear you have much more experience with Ruby packages than me.
> (In reply to Vít Ondruch from comment #6) > > Moreover, I dont udnerstand why you completely dropped the rubygem- and > > install stuff into %{ruby_vendor{lib,arch}dir}. This is discouraged practice. > > You mean why I dropped the "rubygem-" prefix and want the passenger to > rename? There are following reasons pointed out by upstream: > No, I don't referring to prefix. I believe that the code which is currently in %{ruby_vendor{lib,arch}dir} should move back into rubygem-passenger subpackage. The resulting subpackage would have the same content as the rubygem-passenger used to have.
BTW this is current scratch build, if somebody is interested: http://koji.fedoraproject.org/koji/taskinfo?taskID=6625587
I've updated the SRPM and the spec file: Spec URL: http://jkaluza.fedorapeople.org/passenger.spec SRPM URL: http://jkaluza.fedorapeople.org/passenger-4.0.38-1.fc21.src.rpm All things pointed out by Remi in Comment 2 and Comment 3 should be fixed (except libeio bundling which is not decided by FPC yet). After discussion on IRC with upstream and Vit, we've decided to keep the "phusion_passenger" module outside of the Ruby load path (because it's for Passenger internal use only). This, together with other changes I've done, should address all the problems Vit found so far.
* Upstream patches - I am wondering, why the Patch2 is not upstreamed? Or is there some upstream ticket for this? * Remove F18 support - What is the purpose of F18 support? Shouldn't it be just dropped? * Is passenger supported on RHEL5? - I am wondering, since there are several conditionals, which could be removed. Moreover, I can't see any build in Koji. And I doubt that passenger supports Ruby 1.8.5. * %{passenger_libdir} into %{_datadir} - Isn't the content of %{passenger_libdir} just noarch stuff? * Is there reason to keep ruby_extension_source in -devel? - Aren't the sources kept in -debuginfo? * Duplicated passenger_native_support.so - This library is included in main package as well as in -native-libs subpackage. This is probably overlook. - I am wondering what is the purpose of -native-libs anyway. Although the description says "It has been separated so that installing a new Ruby interpreter only necessitates rebuilding this package.", I somehow mis how it would be achieved in reality? * Object files in passenger/common - What is the intention here? Why they are kept in devel package?
(In reply to Vít Ondruch from comment #12) > * Upstream patches > - I am wondering, why the Patch2 is not upstreamed? Or is there some > upstream ticket for this? I've checked all patches and it looks they are not needed with newer versions of Passenger. I will remove them. > > * Remove F18 support > - What is the purpose of F18 support? Shouldn't it be just dropped? It was there to stay consistent with upstream spec file, but you are right it's not needed for the official Fedora and won't be probably needed for upstream soon too. > * Is passenger supported on RHEL5? > - I am wondering, since there are several conditionals, which could be > removed. Moreover, I can't see any build in Koji. And I doubt that > passenger > supports Ruby 1.8.5. It's not. I will remove these conditionals. > * %{passenger_libdir} into %{_datadir} > - Isn't the content of %{passenger_libdir} just noarch stuff? There are some internally used binaries, so it is not noarch. I used %{_datadir} originally, but I had to change that to libdir because of that. > * Is there reason to keep ruby_extension_source in -devel? > - Aren't the sources kept in -debuginfo? Passenger is able to compile various nginx versions for Passenger standalone mode. For this functionality, it needs it's source code. I'm not sure about this particular source file. I will ask upstream if it's really needed. > * Duplicated passenger_native_support.so > - This library is included in main package as well as in -native-libs > subpackage. This is probably overlook. > - I am wondering what is the purpose of -native-libs anyway. Although the > description says "It has been separated so that installing a new Ruby > interpreter only necessitates rebuilding this package.", I somehow mis > how > it would be achieved in reality? Good point. I think it will make sense to not provide this as separate subpackage. I will consult with upstream. > * Object files in passenger/common > - What is the intention here? Why they are kept in devel package? As I stated above, they are there to allow passenger to compile nginx (if admin asks Passenger to do it).
(In reply to Jan Kaluža from comment #13) > > * %{passenger_libdir} into %{_datadir} > > - Isn't the content of %{passenger_libdir} just noarch stuff? > > There are some internally used binaries, so it is not noarch. I used > %{_datadir} originally, but I had to change that to libdir because of that. Could you be more specific please? What does it mean "internally used binaries"? Are you speaking about passenger_native_support.so? I can imagine it is convenient to have the .so file side by side with the .rb files, since you save some issues with managing of load path, but I'd not call it the best way. > > * Is there reason to keep ruby_extension_source in -devel? > > - Aren't the sources kept in -debuginfo? > > Passenger is able to compile various nginx versions for Passenger standalone > mode. For this functionality, it needs it's source code. I'm not sure about > this particular source file. I will ask upstream if it's really needed. > > > * Object files in passenger/common > > - What is the intention here? Why they are kept in devel package? > > As I stated above, they are there to allow passenger to compile nginx (if > admin asks Passenger to do it). - Is the package really prepared to do so and properly? Where it places the output binaries? Are they placed into proper places, such as /usr/local or /opt? - Shouldn't be this functionality split into separate package? - Shouldn't we precompile the nginx?
(In reply to Vít Ondruch from comment #14) > (In reply to Jan Kaluža from comment #13) > > > * %{passenger_libdir} into %{_datadir} > > > - Isn't the content of %{passenger_libdir} just noarch stuff? > > > > There are some internally used binaries, so it is not noarch. I used > > %{_datadir} originally, but I had to change that to libdir because of that. > > Could you be more specific please? What does it mean "internally used > binaries"? Are you speaking about passenger_native_support.so? I can imagine > it is convenient to have the .so file side by side with the .rb files, since > you save some issues with managing of load path, but I'd not call it the > best way. I can patch the source code to find out passenger_native_support.so from different directory, but why this is a problem? Where should I put this library then? I can probably move *.rb into %{_datadir} and keep just passenger_native_support.so in %{_libdir}. There are executable binaries in /usr/lib64/passenger/agents which are executed by mod_passenger. I can imagine moving them into /usr/libexec probably. > > > * Is there reason to keep ruby_extension_source in -devel? > > > - Aren't the sources kept in -debuginfo? > > > > Passenger is able to compile various nginx versions for Passenger standalone > > mode. For this functionality, it needs it's source code. I'm not sure about > > this particular source file. I will ask upstream if it's really needed. > > > > > * Object files in passenger/common > > > - What is the intention here? Why they are kept in devel package? > > > > As I stated above, they are there to allow passenger to compile nginx (if > > admin asks Passenger to do it). > > - Is the package really prepared to do so and properly? Where it places the > output binaries? Are they placed into proper places, such as /usr/local or > /opt? > - Shouldn't be this functionality split into separate package? > - Shouldn't we precompile the nginx? It is prepared. You can execute passenger-install-nginx-module which asks for the dependencies, the path where to install nginx (/opt/nginx by default), downloads it and compiles it. We should not precompile nginx, because we would bundle it, which is not allowed. I've contacted nginx maintainers to include passenger module into Fedora's nginx package, but that's not relevant to this review.
(In reply to Jan Kaluža from comment #15) > (In reply to Vít Ondruch from comment #14) > > (In reply to Jan Kaluža from comment #13) > > > > * %{passenger_libdir} into %{_datadir} > > > > - Isn't the content of %{passenger_libdir} just noarch stuff? > > > > > > There are some internally used binaries, so it is not noarch. I used > > > %{_datadir} originally, but I had to change that to libdir because of that. > > > > Could you be more specific please? What does it mean "internally used > > binaries"? Are you speaking about passenger_native_support.so? I can imagine > > it is convenient to have the .so file side by side with the .rb files, since > > you save some issues with managing of load path, but I'd not call it the > > best way. > > I can patch the source code to find out passenger_native_support.so from > different directory, but why this is a problem? Platform independent code belongs to %{_datadir}. In theory, it allows to share the files among platforms and it is less duplication on your drive. So although it is possible and some might have different opinions, I'd prefer the %{_datadir}. This is not blocker, though. > Where should I put this library then? > > I can probably move *.rb into %{_datadir} and keep just > passenger_native_support.so in %{_libdir}. > > There are executable binaries in /usr/lib64/passenger/agents which are > executed by mod_passenger. I can imagine moving them into /usr/libexec > probably. That seems sensible. > > > > * Is there reason to keep ruby_extension_source in -devel? > > > > - Aren't the sources kept in -debuginfo? > > > > > > Passenger is able to compile various nginx versions for Passenger standalone > > > mode. For this functionality, it needs it's source code. I'm not sure about > > > this particular source file. I will ask upstream if it's really needed. > > > > > > > * Object files in passenger/common > > > > - What is the intention here? Why they are kept in devel package? > > > > > > As I stated above, they are there to allow passenger to compile nginx (if > > > admin asks Passenger to do it). > > > > - Is the package really prepared to do so and properly? Where it places the > > output binaries? Are they placed into proper places, such as /usr/local or > > /opt? > > - Shouldn't be this functionality split into separate package? > > - Shouldn't we precompile the nginx? > > It is prepared. You can execute passenger-install-nginx-module which asks > for the dependencies, the path where to install nginx (/opt/nginx by > default), downloads it and compiles it. > > We should not precompile nginx, because we would bundle it, which is not > allowed. I've contacted nginx maintainers to include passenger module into > Fedora's nginx package, but that's not relevant to this review. Ok, thanks. That partly resolved my concerns. However, is there some precedent in shipping object files in RPM? Would you mind to check with FPC?
I've updated the SRPM and the spec file: Spec URL: http://jkaluza.fedorapeople.org/passenger.spec SRPM URL: http://jkaluza.fedorapeople.org/passenger-4.0.38-1.fc21.src.rpm - *.rb moved into %{_datadir}/passenger - passenger-native-libs subpackage merged with passenger package - passenger_native_support.so is in %{_libdir}/passenger - private executables moved from %{_libdir}/passenger/agents to %{_libexecdir}/passenger/ - devel subpackage has been removed. After reading the guidelines and talking with Remi, I think it's not possible to ship raw object files. Debian Passenger package does not ship them too. I will work more on getting passenger support into Fedora nginx package instead. So the only thing missing right now is unbundled libeio, which I will work on new week (I have decided to rename libeio in Fedora, but I want to give FPC some time for possible reactions on this decision).
Another update: Spec URL: http://jkaluza.fedorapeople.org/passenger.spec SRPM URL: http://jkaluza.fedorapeople.org/passenger-4.0.38-1.fc21.src.rpm - Do not bundle libeio. To build Passenger now, you must build and install libeio from Bug 1097089 (Anyone to review this one...?). - Make the %check section informational (do not fail when tests fail). The test-suite is really sensitive to timing and fails from time to time when the machine is slower.
Spec URL: http://jkaluza.fedorapeople.org/p/passenger.spec SRPM URL: http://jkaluza.fedorapeople.org/p/passenger-4.0.37-1.fc21.src.rpm libeio is now in rawhide so Passenger should build correctly without any bundling now. Could you please check the latest spec file and srpm?
Updated to latest passenger version: Spec URL: http://jkaluza.fedorapeople.org/passenger.spec SRPM URL: http://jkaluza.fedorapeople.org/passenger-4.0.50-1.fc20.src.rpm
# Until rubygem-bluecloth is in Fedora, don't use it Patch201: rubygem-passenger-4.0.18-correct_docs.patch When will be bluecloth in Fedora? Does it have review in progress? Is it needed to duplicate isa and non-isa: Provides: rubygem-passenger = %{version}-%{release} Provides: rubygem-passenger%{?_isa} = %{version}-%{release} Is it summary updated as you promised in comments above? The tmpfiles.d section has strange condition. If fedora>15...else. What else means? I guess it's because of systemd, but shouldn't be better to state condition also for EPEL? How far are you with support of passenger by nginx? Is the comment still valid?
(In reply to Marcela Mašláňová from comment #21) > # Until rubygem-bluecloth is in Fedora, don't use it > Patch201: rubygem-passenger-4.0.18-correct_docs.patch > When will be bluecloth in Fedora? Does it have review in progress? Yes, there is review request (Bug 771297), but it seems to be without any update for longer time. Note that this patch only disables regeneration of Packaging.html, but this file is regenerated by upstream before every release and we normally install it from passenger tarball, so there's no documentation lost by applying this patch. > Is it needed to duplicate isa and non-isa: > Provides: rubygem-passenger = %{version}-%{release} > Provides: rubygem-passenger%{?_isa} = %{version}-%{release} I think it is according to guidelines for "Renaming/Replacing Existing Packages". They say: Explicit Provides: need to be aware of whether the package is supplying things that can be used in an arch-independent or arch-specific fashion. For packages that are not noarch, Provides: should be made arch-specific by applying the %{?_isa} macro to the end of the text string in Provides (e.g. Provides: foo%{?_isa} = 2:%{version}-%{release}). > Is it summary updated as you promised in comments above? Yes, it is. Previously it mentioned only Ruby, now it mentions also other supported languages. > The tmpfiles.d section has strange condition. If fedora>15...else. What else > means? I guess it's because of systemd, but shouldn't be better to state > condition also for EPEL? I think that condition can be removed from passenger.spec. I'm going to rename passenger only in rawhide and this is definitely not going to end up in EPEL6, so we can keep this package without these older conditions. I will upload fixed package soon. > How far are you with support of passenger by nginx? Is the comment still > valid? I'm going to contact nginx maintainer once new passenger package will be in rawhide. The plan is to ask for bundling exception and later bundle passenger with nginx.srpm to compile nginx equivalent of "mod_passenger" and ship it together with nginx. However, this is long term plan and out of scope of this renaming review. Passenger srpm from this review does not change anything in the current nginx-passenger relationship.
Spec URL: http://jkaluza.fedorapeople.org/passenger.spec SRPM URL: http://jkaluza.fedorapeople.org/passenger-4.0.50-2.fc20.src.rpm
I was able to rebuild it in rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=7675225 Please, remove the condition in tmpfiles.d. I guess Vit found all possible Ruby problems. I reviewed all comments, specfile and installation. It looks good to me, let's finally APPROVE it.
New Package SCM Request ======================= Package Name: passenger Short Description: Phusion Passenger application server Upstream URL: https://www.phusionpassenger.com/ Owners: jkaluza kanarip tdawson wakko666 Branches: InitialCC:
Git done (by process-git-requests).
Thanks everyone for the work done here! http://koji.fedoraproject.org/koji/taskinfo?taskID=7747512
Jan - any reason not to push this F21?
(In reply to Orion Poplawski from comment #28) > Jan - any reason not to push this F21? I would ask the other way: "Any reason to push this into F21"? F21 is stable and from my POV, this package is quite different to rubygem-passenger which it obsoletes and it might break stuff.
Actually, at the moment the only difference between the F20 rubygem-passenger pacakge and the F22 passenger package is the name. But F21 has been left behind at the moment as Brett was under the impression that it had been replaced by "passenger" in F21 as well. I'm fine with either solution (updating rubygem-passenger in F21 or pushing passenger to F21), just wanted to coordinate.