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.
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
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?
Created attachment 830691 [details] Proposed plv8.spec file against plv8 1.4.1 version
Created attachment 830692 [details] plv8 src.rpm
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
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
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
Created attachment 837221 [details] plv8 src.rpm
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.
* 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.
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
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?
(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.
Is there anything I can do to advance this bug further? I have addressed all the comments given so far about the spec file.
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.
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.
*** Bug 1274417 has been marked as a duplicate of this bug. ***
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.
Created attachment 1094959 [details] Proposed plv8.spec file against plv8 1.4.4 version
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 :)
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.
Hello, do you want to complete this review? We would like to get this package into Fedora so we would complete it. Thanks.
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.
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.
(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?
Sure, I will fire new bug today.
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
Any progress one this?
(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.
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
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
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
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).
Spec URL: https://raw.githubusercontent.com/praiskup/plv8-pkg/master/plv8.spec SRPM URL: https://praiskup.fedorapeople.org/plv8-2.1.0-2.src.rpm
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
- 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.
(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
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.
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.
Spec URL: https://raw.githubusercontent.com/praiskup/plv8-pkg/master/plv8.spec SRPM URL: https://praiskup.fedorapeople.org/plv8-2.1.0-5.src.rpm
Package approved.
$ 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
(fedrepo-req-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/plv8
https://koji.fedoraproject.org/koji/taskinfo?taskID=23792623 https://koji.fedoraproject.org/koji/taskinfo?taskID=23792614
plv8-2.1.0-5.fc27 has been submitted as an update to Fedora 27. https://bodhi.fedoraproject.org/updates/FEDORA-2017-18be6c0e07
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
plv8-2.1.0-5.fc27 has been pushed to the Fedora 27 stable repository. If problems still persist, please make note of it in this bug report.