Bug 1036130 - Review request: plv8 - javascript language extension for postgresql
Review request: plv8 - javascript language extension for postgresql
Status: ON_QA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Robert-André Mauchin
Fedora Extras Quality Assurance
:
: 1274417 (view as bug list)
Depends On: 1517657
Blocks:
  Show dependency treegraph
 
Reported: 2013-11-29 09:11 EST by Mikko Tiihonen
Modified: 2017-12-19 17:49 EST (History)
14 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:
Cloudforms Team: ---
zebob.m: fedora‑review+


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?
Comment 29 Pavel Raiskup 2017-12-15 09:57:23 EST
(In reply to John Griffiths from comment #28)
> Any progress one this?

I think upstream moved forward a bit.  Pavel Kajaba is not in team anymore,
so let's find some other maintainer.
Comment 30 Devrim GÜNDÜZ 2017-12-15 11:34:10 EST
Hi,

I think the community spec file is in a good shape, and can be adjusted for Fedora pretty easily:

https://git.postgresql.org/gitweb/?p=pgrpms.git;a=blob;f=rpm/redhat/master/plv8/master/plv8.spec;h=5802e15d9966f7c051d4bc976a22d7da351ec225;hb=HEAD

Regards, Devrim
Comment 31 Pavel Raiskup 2017-12-15 17:30:29 EST
I started from bug 1274417 (where pkajaba ended, inspired by pgrpms,
thanks!): https://github.com/praiskup/plv8-pkg/blob/master/plv8.spec

Build is on [1], but I doubt it works (linking issues expected [2]).  I'll
have a closer look soon.  %v8_arches requested on [3].

[1] https://koji.fedoraproject.org/koji/taskinfo?taskID=23714224
[2] https://github.com/plv8/plv8/pull/247#partial-pull-merging
[3] https://bugzilla.redhat.com/1526522
Comment 32 Robert-André Mauchin 2017-12-15 19:20:26 EST
Not a review, but:

 - Not needed:

%defattr(-,root,root)

 - make %{?_smp_mflags} → %make_build

 - make install DESTDIR=%{buildroot} %{?_smp_mflags} → %make_install

 - Not used anymore:

Group:	Applications/Databases
Comment 33 Pavel Raiskup 2017-12-16 05:06:52 EST
Thanks for pre-review, fixes applied.

But linking issues are there, as expected:

postgres=# create extension plv8;
ERROR:  could not load library "/usr/lib64/pgsql/plv8.so": /usr/lib64/pgsql/plv8.so: undefined symbol: _ZN2v88platform21CreateDefaultPlatformEiNS0_15IdleTaskSupportENS0_21InProcessStackDumpingEPNS_17TracingControllerE

We need to wait for v8-devel fix (blocking bug).
Comment 35 Pavel Raiskup 2017-12-18 08:52:43 EST
Just to make it clear -- this should be basically ready for review as
the #1517657 is worked-around.

Spec URL: https://raw.githubusercontent.com/praiskup/plv8-pkg/master/plv8.spec
SRPM URL: https://praiskup.fedorapeople.org/plv8-2.1.0-3.src.rpm
Comment 36 Robert-André Mauchin 2017-12-18 12:20:58 EST
 - Add a comment for each patch explaining what they do

 - Group: is not needed in Fedora. See: https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections

 - Use a more meaningful name for your archive, with the following Source0:

Source0:	https://github.com/%{sname}/%{sname}/archive/v%{version}/%{name}-%{version}.tar.gz


Package Review
==============

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



===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Development (unversioned) .so files in -devel subpackage, if present.
     Note: Unversioned so-files in private %_libdir subdirectory (see
     attachment). Verify they are not in ld path.
[x]: Header files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

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: "PostgreSQL", "Unknown or generated". 62 files have unknown
     license. Detailed output of licensecheck in
     /home/bob/packaging/review/plv8/review-plv8/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: Package does not own files or directories owned by other packages.
     Note: Dirs in package are owned also by:
     /usr/share/pgsql/extension(orafce, postgresql-server)
[x]: %build honors applicable compiler flags or justifies otherwise.
[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.
[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]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 40960 bytes in 3 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: 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 is included in %license.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

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

Generic:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[-]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in
     plv8-debuginfo , plv8-debugsource
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[!]: Patches link to upstream bugs/comments/lists or are otherwise
     justified.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[-]: Spec use %global instead of %define unless justified.
     Note: %define requiring justification: %define _configure :
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Package should compile and build into binary rpms on all supported
     architectures.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on debuginfo package(s).
     Note: No rpmlint messages.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: plv8-2.1.0-3.fc28.x86_64.rpm
          plv8-debuginfo-2.1.0-3.fc28.x86_64.rpm
          plv8-debugsource-2.1.0-3.fc28.x86_64.rpm
          plv8-2.1.0-3.fc28.src.rpm
plv8-debugsource.x86_64: W: no-documentation
plv8.src: W: strange-permission plv8-2.1.0-make-test.patch 600
plv8.src: W: strange-permission plv8-2.1.0-make.patch 600
4 packages and 0 specfiles checked; 0 errors, 3 warnings.
Comment 37 Pavel Raiskup 2017-12-19 02:02:20 EST
(In reply to Robert-André Mauchin from comment #36)
>  - Add a comment for each patch explaining what they do

Done inside the patch, but I added comment on top of "patch" section about
this fact; and I've split the 'patch0' into two patches (with better naming,
and per-issue purpose).

>  - Group: is not needed in Fedora. See:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections

Removed.

>  - Use a more meaningful name for your archive, with the following Source0:
> 
> Source0:
> https://github.com/%{sname}/%{sname}/archive/v%{version}/%{name}-%{version}.
> tar.gz

Done, I didn't know this trick!  Thank you.

Spec URL: https://raw.githubusercontent.com/praiskup/plv8-pkg/master/plv8.spec
SRPM URL: https://praiskup.fedorapeople.org/plv8-2.1.0-4.src.rpm
Comment 38 Robert-André Mauchin 2017-12-19 04:40:17 EST
Please add a comment for the patches *in the SPEC*. The goal is to inform someone reading the SPEC, a proven packager needing to intervene on your package for example, what the patches do without needing to go look at the patches file themselves.
Comment 39 Pavel Raiskup 2017-12-19 05:00:16 EST
Robert, I can only disagree (but I'll update, I'm glad that you go through
the review).

As a provenpackager, I basically shouldn't touch patches and if, I should
really know what am I doing.  Opening the patch itself is really trivial
and natural habit.  IOW, I really dislike duplicating the info given by
(a) patch name, (b) git format-patch output and link pointing to upstream
discussion.
Comment 41 Robert-André Mauchin 2017-12-19 06:21:16 EST
Package approved.
Comment 42 Pavel Raiskup 2017-12-19 06:39:10 EST
$ fedrepo-req -t 1036130 plv8 master
https://pagure.io/releng/fedora-scm-requests/issue/3602
$ fedrepo-req-branch plv8 f27
https://pagure.io/releng/fedora-scm-requests/issue/3603
Comment 43 Gwyn Ciesla 2017-12-19 07:18:53 EST
(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/plv8
Comment 45 Fedora Update System 2017-12-19 07:55:28 EST
plv8-2.1.0-5.fc27 has been submitted as an update to Fedora 27. https://bodhi.fedoraproject.org/updates/FEDORA-2017-18be6c0e07
Comment 46 Fedora Update System 2017-12-19 17:49:07 EST
plv8-2.1.0-5.fc27 has been pushed to the Fedora 27 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-18be6c0e07

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