Bug 1036130 - Review request: plv8 - javascript language extension for postgresql
Review request: plv8 - javascript language extension for postgresql
Status: ASSIGNED
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Pavel Kajaba
Fedora Extras Quality Assurance
:
: 1274417 (view as bug list)
Depends On:
Blocks: FE-NEEDSPONSOR
  Show dependency treegraph
 
Reported: 2013-11-29 09:11 EST by Mikko Tiihonen
Modified: 2016-07-25 12:38 EDT (History)
13 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
Proposed spec file against plv8 1.4.1 version (1.58 KB, text/x-rpm-spec)
2013-11-29 09:11 EST, Mikko Tiihonen
no flags Details
Proposed plv8.spec file against plv8 1.4.1 version (1.54 KB, text/x-rpm-spec)
2013-11-29 09:56 EST, Mikko Tiihonen
no flags Details
plv8 src.rpm (153.87 KB, application/x-rpm)
2013-11-29 09:58 EST, Mikko Tiihonen
no flags Details
Proposed plv8.spec file against plv8 1.4.1 version (1.49 KB, text/x-rpm-spec)
2013-12-16 06:55 EST, Mikko Tiihonen
no flags Details
plv8 src.rpm (153.79 KB, application/x-rpm)
2013-12-16 06:56 EST, Mikko Tiihonen
no flags Details
Proposed plv8.spec file against plv8 1.4.4 version (1.42 KB, text/plain)
2015-11-16 10:07 EST, Mikko Tiihonen
no flags Details

  None (edit)
Description Mikko Tiihonen 2013-11-29 09:11:34 EST
Created attachment 830677 [details]
Proposed spec file against plv8 1.4.1 version

Could you please add a new package for postgresql javascript language extension.

The home page for the extension is http://code.google.com/p/plv8js/

The extension provides plv8 (javascript) procedural language to postgresql. Also coffeescript and livescript languages are supported.

I created the spec by combining the spec file from
http://code.google.com/p/plv8js/issues/detail?id=57
and modifying to to look like the latest F20 postgis postgresql extension.
Comment 1 Pavel Raiskup 2013-11-29 09:31:22 EST
Reassigning to 'Package Review' component.  First of all, please could you
use different name?  The postgresql-plv8 seems like it comes from
postgresql.conf.  Probably something like pgplv8 would be OK (we plan to
rename postgresql-ip4r in future to remove the postgresql-* prefix, and same
with other packages).

Also, this is place you should probably start:
http://fedoraproject.org/wiki/Package_Review_Process
Comment 2 Pavel Raiskup 2013-11-29 09:43:05 EST
Mikko, thank you for trying to maintain new package for Fedora, btw.!

(In reply to Pavel Raiskup from comment #1)
> Reassigning to 'Package Review' component.  First of all, please could you
> use different name?  The postgresql-plv8 seems like it comes from
> postgresql.conf.

Sorry for the typo: s/postgresql.conf/postgresql.spec/.  I would be able to
review the package, though I am not sponsor so you'll need somebody else (if
you are not already sponsored) for sponsoring.  Could you post srpm & spec
file according to Package Review Process?
Comment 3 Mikko Tiihonen 2013-11-29 09:56:13 EST
Created attachment 830691 [details]
Proposed plv8.spec file against plv8 1.4.1 version
Comment 4 Mikko Tiihonen 2013-11-29 09:58:23 EST
Created attachment 830692 [details]
plv8 src.rpm
Comment 5 Pavel Raiskup 2013-12-04 10:08:57 EST
Mikko, you are probably not sponsored that means I can't review your package.
So I am adding this bug to FE-NEEDSPONSOR tracker - then some sponsor can pick
this bugzilla and go through this review.  Please, study the [1] document very
carefully (especially the Contributor role).  You should also submit the
comment here in bugzilla a little bit different way - for template you can
look at [2].

[1] http://fedoraproject.org/wiki/Package_Review_Process
[2] https://bugzilla.redhat.com/enter_bug.cgi?product=Fedora&format=fedora-review
Comment 6 Devrim GÜNDÜZ 2013-12-12 09:47:23 EST
Hi,

I can sponsor this package. Here is a quick review:

* Deps are defined fine
* Builds on my Fedora 19 machine
* Please remove 2nd %changelog parameter
* Please fix summary -- I don't think it belongs to this package

I changed the spec a bit for upstream packaging. Package works, and all extensions are loaded w/o any issues.

Pavel, can you please remind me how I can sponsor?

Regards, Devrim
Comment 7 Mikko Tiihonen 2013-12-16 06:55:54 EST
Created attachment 837220 [details]
Proposed plv8.spec file against plv8 1.4.1 version

Thank you for the review.

Changes since last time:
- fixed the Summary
- removed duplicate %changelog line
- enhanced the description to mention also CoffeeScript and LiveScript
Comment 8 Mikko Tiihonen 2013-12-16 06:56:31 EST
Created attachment 837221 [details]
plv8 src.rpm
Comment 9 Zoltan Boszormenyi 2013-12-18 07:07:19 EST
Informal review:

1. rpmlint gives this:

$ rpmlint -i -v plv8.spec 
plv8.spec:29: W: mixed-use-of-spaces-and-tabs (spaces: line 29, tab: line 4)
The specfile mixes use of spaces and tabs for indentation, which is a cosmetic
annoyance.  Use either spaces or tabs for indentation, not both.

plv8.spec: I: checking-url http://plv8js.googlecode.com/files/plv8-1.4.1.zip (timeout 10 seconds)
plv8.spec: W: invalid-url Source0: http://plv8js.googlecode.com/files/plv8-1.4.1.zip HTTP Error 404: Not Found
The value should be a valid, public HTTP, HTTPS, or FTP URL.

0 packages and 1 specfiles checked; 0 errors, 2 warnings.

I followed the links on Googe Code and the Source0 URL should be https://plv8js.googlecode.com/files/plv8-1.4.1.zip , instead of http://...

Tabs and spaces should be consistently used.

2. PostgreSQL extra PL packages are called postgresql-plsomething, calling it "plv8" doesn't say much.

3. Scratch builds were completed for f19, f20, f21, rawhide, failed for dist-5E-epel and dist-6E-epel.

http://koji.fedoraproject.org/koji/taskinfo?taskID=6308990
http://koji.fedoraproject.org/koji/taskinfo?taskID=6308983
http://koji.fedoraproject.org/koji/taskinfo?taskID=6309024
http://koji.fedoraproject.org/koji/taskinfo?taskID=6309028
http://koji.fedoraproject.org/koji/taskinfo?taskID=6309011
http://koji.fedoraproject.org/koji/taskinfo?taskID=6309018

4. Remove BuildRoot tag.

5. Group tags are not needed anymore, remove them from the spec file.

6. "%defattr(-,root,root)" is also not needed, remove it.

7. Remove %clean section.

8. Remove "rm -rf  %{buildroot}" in %install

9. I tried "make install DESTDIR=..." manually and there is no *.sql file in %{datadir}, i.e. in $DESTDIR/usr/share. So the extra "rm -f  %{buildroot}%{_datadir}/*.sql" line is not needed in the %install section after "make install". Remove it.
Comment 10 Michael Schwendt 2013-12-18 07:28:52 EST
* rpmlint is not only for checking spec files. Run it on the src.rpm *and* all built rpms at once.

* "rawhide" and "f21" are the same currently.

* A single scratch-build for only "rawhide" would have been enough during review. Please be gentle with available resources, such as the Fedora build system. Imagine a thousand packagers submit six scratch-build jobs for each minor package update.

[...]

* Please don't attach packages in bugzilla. Request fedorapeople.org web space if you don't have any other public place where to share the rpms. That's step 2.1.11 here:

  https://fedoraproject.org/wiki/Join_the_package_collection_maintainers

Also, if you followed the process, you would add to lines "Spec URL: ..." and "SRPM URL: ...", which can be evaluated by the fedora-review tool. One would run "fedora-review -b 1036130" and have it perform lots of helpful tests.
Comment 11 Mikko Tiihonen 2013-12-21 16:39:55 EST
Spec URL: https://www.dropbox.com/s/9cezsd32j7ppj5q/plv8.spec
SRPM URL: https://www.dropbox.com/s/8q89vbvggh9dz5o/plv8-1.4.1-1.fc20.src.rpm
Description:

3rd revision of package comment 9 review:
- fixed findings 1,4,5,6,7,8 and 9

- about 3: the 6E-epel failed due to missing pg 9.2 package
    DEBUG util.py:264:  Getting requirements for plv8-1.4.1-1.el6.src
    DEBUG util.py:264:  Error: No Package found for postgresql-devel >= 9.2
  - I found slides that say plv8 supports pg >= 8.4 -> updated spec
    to allow older versions in hope that rhel builds will succeed

- about 2:
  I had initially named the package postgresql-plv8 but Pavel Raiskup said
  it will be confused with official postgresql packages.

  If plv8 is not acceptable either then I propose we decide
  postgresql-<package> to be from official postgresql sources and
  postgresql-ext-<package> be any extension and document the practice in
  https://fedoraproject.org/wiki/Packaging:NamingGuidelines

  Should I keep plv8 or rename it to postgresql-ext-plv8 ?

Other changes:
- fixed plv8.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/plv8/README
- fixed plv8-debuginfo.x86_64: E: debuginfo-without-sources
Comment 12 Zoltan Boszormenyi 2013-12-22 07:36:36 EST
My 2 cents: postgresql-plparrot and postgresql-plruby are also not official, they are not part of the postgresql sources and are Fedora packages.

"plv8" would be also confusing on the first sight, like, does it have anything to do with the "pl" (SWI Prolog) package?
Comment 13 Pavel Raiskup 2013-12-30 06:11:20 EST
(In reply to Zoltan Boszormenyi from comment #12)
> My 2 cents: postgresql-plparrot and postgresql-plruby are also not official,
> they are not part of the postgresql sources and are Fedora packages.

Yes, but we plan to rename these packages as I said before.  It is not a
reason for naming misleadingly another package.
Comment 14 Mikko Tiihonen 2014-03-31 05:22:24 EDT
Is there anything I can do to advance this bug further?

I have addressed all the comments given so far about the spec file.
Comment 15 Christopher Meng 2014-03-31 06:15:42 EDT
Look at other review requests, and then look back to this.

I think you should figure out what still needed.

plv8? postgresql-plv8? Bug title? SPEC name?

LDFLAGS not inserted.

make install DESTDIR=%{buildroot} %{?_smp_mflags}, we only need smp flags during build, I don't think it's needed in %install.

==============

You didn't follow http://fedoraproject.org/wiki/Join_the_package_collection_maintainers, you only want to get this package into Fedora.

Sorry, there are a lot of things you haven't done yet.
Comment 16 Mikko Tiihonen 2014-03-31 08:39:10 EDT
Spec URL: https://www.dropbox.com/s/9cezsd32j7ppj5q/plv8.spec
SRPM URL: https://www.dropbox.com/s/ehbxasatvkcqe3f/plv8-1.4.1-1.fc21.src.rpm
Fedora Account System Username: gmokki

The postgresql-plv8 is not an allowed name, so the package will be kept as plv8 (to match upstream).

Since last report:
1) I got myself a fedora account and did all required steps (ssh key etc)
2) Removed the smp_flags from the make install
3) Did a scratch build http://koji.fedoraproject.org/koji/taskinfo?taskID=6692068
4) verified that the correct(?) fedora ld flags are passed to the final .so link command.

See: http://kojipkgs.fedoraproject.org//work/tasks/2070/6692070/build.log which shows the following parameters: 

g++ -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -m64 -mtune=generic -DLINUX_OOM_SCORE_ADJ=0 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -fpic -shared -o plv8.so plv8.o plv8_type.o plv8_func.o plv8_param.o coffee-script.o livescript.o -L/usr/lib64 -Wl,-z,relro   -Wl,--as-needed  -lv8 

I think that by including the /usr/lib64/pgsql/pgxs/src/makefiles/pgxs.mk from the postgresql-devel package the Makefile gets correct linking parameters.
Comment 17 Pavel Raiskup 2015-11-11 03:04:31 EST
*** Bug 1274417 has been marked as a duplicate of this bug. ***
Comment 18 Pavel Raiskup 2015-11-11 03:25:15 EST
Mikko, I tried:

$ fedora-review -b 1036130 -m fedora-rawhide-x86_64
INFO: Processing bugzilla bug: 1036130
INFO: Getting .spec and .srpm Urls from : 1036130
INFO:   --> SRPM url: https://www.dropbox.com/s/ehbxasatvkcqe3f/plv8-1.4.1-1.fc21.src.rpm
INFO:   --> Spec url: https://www.dropbox.com/s/9cezsd32j7ppj5q/plv8.spec
INFO: Using review directory: /home/praiskup/rh/packages/plv8/review/1036130-plv8
INFO: Downloading .spec and .srpm files
error: line 1: Unknown tag: <!DOCTYPE html><html lang="en" xmlns:fb="http://ogp.me/ns/fb#" xml:lang="en" class="media-desktop" xmlns="http://www.w3.org/1999/xhtml"><head><script nonce="u8IIwhjtSjJay1Rbjr8U">

The links you post here should directly point to the files.  As it has been
suggested before, fedorapeople.org could help.
Comment 19 Mikko Tiihonen 2015-11-16 10:07 EST
Created attachment 1094959 [details]
Proposed plv8.spec file against plv8 1.4.4 version
Comment 20 Pavel Kajaba 2015-11-16 11:12:56 EST
Please don't attach packages in bugzilla. Request fedorapeople.org web space if you don't have any other public place where to share the rpms. That's step 2.1.11 here:

  https://fedoraproject.org/wiki/Join_the_package_collection_maintainers

Would you kindly reupload it somewhere else ? 

Thanks :)
Comment 21 Pavel Raiskup 2015-11-16 14:51:32 EST
I agree, we basically need the format as you've done in comment #16, but the
links need to point directly to files, not to "download page".  That way you
allow automatic 'fedora-review --bug 1036130 -m fedora-rawhide-x86_64' command
to succeed.
Comment 22 Pavel Kajaba 2015-12-17 02:51:42 EST
Hello, do you want to complete this review?

We would like to get this package into Fedora so we would complete it.

Thanks.
Comment 23 John Griffiths 2016-05-06 10:47:22 EDT
What is going on with this package?

xTuple has new versions that will not run against postgre in Fedora 22 since postgre does not have plv8.
Comment 24 John Griffiths 2016-05-06 10:51:58 EDT
There is a package on the Postgre site for plv8, http://yum.postgresql.org/9.4/fedora/fedora-22-x86_64/plv8_94-1.4.4-1.f22.x86_64.rpm

That rpm will not install against the Fedora 22 Postgre. There are some naming mismatches I think.
Comment 25 Pavel Raiskup 2016-05-09 02:08:46 EDT
(In reply to John Griffiths from comment #24)
> There is a package on the Postgre site for plv8,
> http://yum.postgresql.org/9.4/fedora/fedora-22-x86_64/plv8_94-1.4.4-1.f22.
> x86_64.rpm
> That rpm will not install against the Fedora 22 Postgre. There are some
> naming mismatches I think.

Right, RPMs on yum.postgresql.org are self-standing, those are usually built
against PostgreSQL server from yum.postgresql.org, too.

Well, with respect to original reporter - we wanted a wait a bit.  Maybe it is
the right time to take it over.

Pavel, are you OK to re-start the review process (starting probably wit plv8
from PGRPMS spec file, I bet the spec license is compatible as usually - we
should CC Devrim) and submit a new Review bug?
Comment 26 Pavel Kajaba 2016-05-09 05:18:16 EDT
Sure, I will fire new bug today.
Comment 27 Pavel Kajaba 2016-05-10 04:34:02 EDT
I tried to work a bit on plv8 yesterday. However there is problem since latest released version supports just v8 up to version 4.10 and in rawhide there is something like 5.*. 

Current master branch supports latest v8, but it's just in development since they have lot of issues to solve [1].

I have contacted upstream [2]. Is there anyone who could help with JavaScript? I will try to help them, but I have just basic knowledge of JS.

[1] https://github.com/plv8/plv8/issues?q=is%3Aopen+is%3Aissue+milestone%3A%222.0+Release%22
[2] https://github.com/plv8/plv8/issues/178
Comment 28 John Griffiths 2016-07-25 12:38:15 EDT
Any progress one this?

Note You need to log in before you can comment on or make changes to this bug.