remicollet / remirepo

Issue tracker for
https://rpms.remirepo.net/
278 stars 53 forks source link

Issues with php-pecl-xhprof.spec #4

Closed theseer closed 10 years ago

theseer commented 11 years ago

Hi,

when installing named package from the fedora EPEL repository a customer did run into some issues I'd like to discuss first before making it a fedora/EPEL bug ;)

  1. You create an apache config file, yet do not require apache to be installed as a webserver. To me that is inconsistent. I do agree with not requiring apache to be installed but wouldn't want to have an apache config file in that case either. Maybe this needs to be separate package?
  2. The offical pecl release 0.9.2 is from 2009 while the codebase on github.com did undergo many optimizations and fixes. One being the patch you use to build it for PHP 5.4. Any chance to get the other optimiztations and fixes as well? I'm not convinced yet that there will be an offical updated pecl release anytime soon - even though i'll be talking to Scott at facebook to see that happen.
  3. In your package version 0.9.2-4 you moved the code into /usr/share/xhprof. That change is incompatible to the pear path location, which is part of the default include path for php. While i second the choice of location for the html code, which is by definition not a library and probably shouldn't be in the pear path, the library and runtime include code used when actually profiling should. Sadly though, the relative location is hardcoded in the sample code as well as the display code. Using a different path to include the library code compared to a standard pecl/pear installation makes PHP scripts using xhprof less portable though and should thus eventually be reconsidered.
remicollet commented 11 years ago

Thanks for your report.

  1. the xhprof package, which provides the web interface, and the apache config file requires php (aka mod_php), so also requires httpd.
  2. really, I will prefer a new upstream official version, but I could consider adding some patches from upstream
  3. need to think about this, but even if a put the files in the include path (/usr/share/php), consummer will have to set the library path (will be in a sub-dir)

As this package is now in the official EPEL repository, https://bugzilla.redhat.com/ is the correct place to report issues.

theseer commented 11 years ago

Hi,

thanks for the fast reply. Do you want me to move this to bugzilla.redhat.com?

  1. Okay, you got a point - I skipped over that. I'm still a bit unhappy with the apache centric thinking in many - if not most - web related packages. I'd like to have various webbased tools installed from rpm but want to use nginx or lighttpd as a webserver, I fail to see the reason to tightly couple the tool with a specifc server. Installing a "phpmyadmin-apache" package which would require phpmyadmin, makes more sense to me then having the option to have a "phpmyadmin-nginx" package next - if need be, as well as no default config for either webserver if i use the plain package. But this is obivously not the place to rant on this, so disregard my issue point 1 :)
  2. I'm taking to Scott MacVicar and let you know.
  3. I'll talk about that with Scott as well. Though only the library path is needed for profiling itself - which would be in the standard include path. The webpage displaying the stuff is a different story.
remicollet commented 11 years ago

Do you want me to move this to bugzilla.redhat.com?

Not required for now (mostly discussion). But you will be welcome later.

I'm still a bit unhappy with the apache centric thinking in many - if not most - web related packages.

I totally agree. I was thinking of splitting some packages for some time. We are probably going to work on this for Fedora 19, see https://fedoraproject.org/wiki/Features/WebserverDependency

Main issue, for now, is how to drop a config file somewhere and have a webapp working "out of the box" with nginx or lighttpd ?

I'm taking to Scott MacVicar and let you know.

Yes, please

Though only the library path is needed for profiling itself - which would be in the standard include path.

Yes, but which path under /usr/share/php ? xhprof ? xhprof_lib ? xjprof/lib ? other ?

theseer commented 11 years ago

XHProf Release: I didn't get any update from Sara yet (couldn't reach Scott) - so no news on the xhprof release yet.

Webserver: I don't see your issue? Nginx and lighttpd can include config snippets pretty much the same way as apache. So the same logic could be applied for those servers. Of course the config syntax would be different and i'm not convinced that all apache features can be mapped 1:1 for lighty and nginx. So the main issue is what technical dependencies there might be on apache specific features - or vice versa.

Library path: The default pear path in fedora is /usr/share/pear - which is also set in the default include_path of PHP. And the xhprof_lib would be a subdirectory of that. Why would you need to change it?

remicollet commented 11 years ago

This is not a "pear" package, so file must not go in /usr/share/pear.

theseer commented 11 years ago

While you are technically correct, that's the location the pecl installer places the files when you install xhprof using "pecl install".

remicollet commented 10 years ago

I dont plan to change current layout, as this package is available for a long time, and any change will break people configuration relying on this.

I will think to a better layout for uprofiler if it goes to pecl.

theseer commented 10 years ago

How about making the pear path a symlink? Just a thought...