Spec URL: kenobi.mandriva.com/~pcpa/xedit.spec SRPM URL: kenobi.mandriva.com/~pcpa/xedit-1.1.2-1.src.rpm Description: Xedit provides a simple text editor for X. This is my first fedora package so I need a sponsor. Using a simple package to get used to fedora buildsystem and follow wiki instructions. My goal is to help in the Science and Technology SIG.
For a better use experience with the xedit package, I would also like to have *customization: -color appended to /etc/X11/Xresources from xorg-x11-xinit package.
Quick comments: - add disttag - one req. on each line please - don't use %make_install - defattr is almost correct :-) Please create FAS account and do a koji scratch build.
(In reply to comment #2) > Quick comments: > > - add disttag > - one req. on each line please > - don't use %make_install > - defattr is almost correct :-) Thanks for the comments. I noticed there was a missing buildrequires to desktop-file-utils, and corrected the issues in your comment. Earlier I only knew rpmlint was happy with it :-) > Please create FAS account and do a koji scratch build. I had already created one, but did first scratch build now. Not sure if should add a link to koji task to bugzilla as it should be valid for only a short amount of time, but here it is https://koji.fedoraproject.org/koji/taskinfo?taskID=4027234
Nice. Please post direct, complete urls to updates spec and srpm on each time. Makes it easy to continue review.
Thanks. Updated spec: http://kenobi.mandriva.com/~pcpa/xedit.spec Updated srpm: http://kenobi.mandriva.com/~pcpa/xedit-1.1.2-1.fc16.src.rpm
spec files looks good! You might add a comment about the patches and why autoreconf is needed. Do you have more packages for out review? Have you done any reviews?
Thanks again! I added comments to the spec file for the reason of all patches. I changed to just call "autoreconf". I thought it could have been there for historic reasons from when xprint was enabled by default, but it is still required due to the patch to link with Xmu (I tested, and it is also required in Fedora). Just uploaded the spec and srpm overwriting the ones in comment 4. Right now I do not have any extra packages, and I am still very new to fedora packaging. But I plan to submit more packages for review shortly. Need also to read more the wiki to do the proper procedures when requesting patches in existing packages. I had some activity that can be checked in http://lists.fedoraproject.org/pipermail/scitech/2012-April/thread.html as I started with the test xedit package to get used to fedora procedures, but my goal is to help to package sagemath in Fedora. I have packaged and kept sagemath working in Mandriva for around 3 years, since late sagemath 3.x, up to latest one that is 4.8. Another package that should be simple to do, and at least I cannot see it with yum search is megaglest, http://megaglest.org/ that I believe is a very high quality, well, some would say graphics quality are early 2k, but I believe in par with Warcraft 3 and should provide lots of fun for people that like the game genre.
pcpa just pointed out this review request to me. FYI: I'm about to sponsor him in bug #817306.
I will review this package
==== C/C++ ==== [x]: MUST Package does not contain any libtool archives (.la) [x]: MUST Package does not contain kernel modules. [x]: MUST Package contains no static executables. [x]: MUST Rpath absent or only used for internal libs. [x]: MUST Package is not relocatable. ==== Generic ==== [x]: MUST Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: MUST Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: MUST %build honors applicable compiler flags or justifies otherwise. [x]: MUST All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [x]: MUST Buildroot is not present Note: Unless packager wants to package for EPEL5 this is fine [x]: MUST Package contains no bundled libraries. [x]: MUST Changelog in prescribed format. [x]: MUST Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) Note: Clean would be needed if support for EPEL is required [x]: MUST Sources contain only permissible code or content. [!]: MUST Each %files section contains %defattr if rpm < 4.4 Note: defattr(....) present in %files section. This is OK if packaging for EPEL5. Otherwise not needed [-]: MUST Macros in Summary, %description expandable at SRPM build time. [x]: MUST Package requires other packages for directories it uses. [x]: MUST Package uses nothing in %doc for runtime. [x]: MUST Package is not known to require ExcludeArch. [x]: MUST Permissions on files are set properly. [x]: MUST Package does not contain duplicates in %files. [x]: MUST Spec file lacks Packager, Vendor, PreReq tags. [x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. Note: rm -rf would be needed if support for EPEL5 is required [-]: MUST Large documentation files are in a -doc subpackage, if required. [!]: MUST 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 %doc. [!]: MUST License field in the package spec file matches the actual license. [x]: MUST Package consistently uses macros (instead of hard-coded directory names). [x]: MUST Package is named according to the Package Naming Guidelines. [x]: MUST Package does not generate any conflict. [x]: MUST Package obeys FHS, except libexecdir and /usr/target. [x]: MUST Package must own all directories that it creates. [x]: MUST Package does not own files or directories owned by other packages. [x]: MUST Package installs properly. [x]: MUST Requires correct, justified where necessary. [x]: MUST Rpmlint output is silent. [x]: MUST Sources used to build the package match the upstream source, as provided in the spec URL. /home/slaanesh/Documents/fedora/815624/xedit-1.1.2.tar.bz2 : MD5SUM this package : 67193be728414d45a1922911e6437991 MD5SUM upstream package : 67193be728414d45a1922911e6437991 [x]: MUST Spec file is legible and written in American English. [x]: MUST Spec file name must match the spec package %{name}, in the format %{name}.spec. [-]: MUST Package contains a SysV-style init script if in need of one. [x]: MUST File names are valid UTF-8. [x]: MUST Useful -debuginfo package or justification otherwise. [x]: SHOULD Reviewer should test that the package builds in mock. [x]: SHOULD 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]: SHOULD Dist tag is present. [x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q --requires). [x]: SHOULD Package functions as described. [!]: SHOULD Latest version is packaged. [x]: SHOULD Package does not include license text files separate from upstream. [x]: SHOULD Patches link to upstream bugs/comments/lists or are otherwise justified. [x]: SHOULD SourceX / PatchY prefixed with %{name}. Note: Source1: xedit.desktop (xedit.desktop) Patch0: xedit-1.1.2-fix-str- fmt.patch (xedit-1.1.2-fix-str-fmt.patch) Patch1: xedit-1.1.2-autotools- mode.patch (xedit-1.1.2-autotools-mode.patch) Patch2: xedit-1.1.2-int64-bignum.patch (xedit-1.1.2-int64-bignum.patch) Patch3: xedit-1.1.2-newfile.patch (xedit-1.1.2-newfile.patch) Patch4: xedit-1.1.2-underlink.patch (xedit-1.1.2-underlink.patch) [x]: SHOULD SourceX is a working URL. [-]: SHOULD Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: SHOULD Package should compile and build into binary rpms on all supported architectures. [!]: SHOULD %check is present and all tests pass. [!]: SHOULD Packages should try to preserve timestamps of original installed files. [x]: SHOULD Spec use %global instead of %define.
[!]: MUST Each %files section contains %defattr if rpm < 4.4 Note: defattr(....) present in %files section. This is OK if packaging for EPEL5. Otherwise not needed If you're building also for EPEL-5 please add %buildroot, %clean section and remove file as the beginning of the %install section; otherwise please remove %defattr in the %files section. [!]: MUST 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 %doc. The "COPYING" file is not included in the generated package. [!]: MUST License field in the package spec file matches the actual license. The "COPYING" file contains multiple licenses, are you sure a simple "MIT" license is enough? [!]: SHOULD %check is present and all tests pass. A "make check" seems to be implemented in the makefile. [!]: SHOULD Packages should try to preserve timestamps of original installed files. You can use this in the %install section: make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p" [!]: SHOULD Latest version is packaged. I see version 1.2.0 in the xorg repository. rpmlint output is ok: $ rpmlint *rpm 3 packages and 0 specfiles checked; 0 errors, 0 warnings. Please correct the above and you're good to go!
(In reply to comment #11) > [!]: MUST Each %files section contains %defattr if rpm < 4.4 > Note: defattr(....) present in %files section. This is OK if packaging > for EPEL5. Otherwise not needed > > If you're building also for EPEL-5 please add %buildroot, %clean section and > remove file as the beginning of the %install section; otherwise please > remove %defattr in the %files section. I removed the defattr as it is redundant. > [!]: MUST 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 %doc. > > The "COPYING" file is not included in the generated package. Added it to the package as well as other documentation files. > [!]: MUST License field in the package spec file matches the actual license. > > The "COPYING" file contains multiple licenses, are you sure a simple "MIT" > license is enough? The licenses are MIT or BSD-style without clauses. I also added GPLv2+ because of the int64 patch actually adapts code from libgcc. > [!]: SHOULD %check is present and all tests pass. > > A "make check" seems to be implemented in the makefile. It is a fallback that does nothing, but for the sake of review I added it :-) > [!]: SHOULD Packages should try to preserve timestamps of original installed > files. > > You can use this in the %install section: > make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p" Added it. > [!]: SHOULD Latest version is packaged. > > I see version 1.2.0 in the xorg repository. Sorry for that, I had basically just copied the Mandriva spec and made some minor adjustments to it. > rpmlint output is ok: > $ rpmlint *rpm > 3 packages and 0 specfiles checked; 0 errors, 0 warnings. > > > Please correct the above and you're good to go! Thanks. I will probably need to wait a bit until a problem in libXaw is corrected, otherwise, xedit will crash most times, well any time some code path involving selections is exercised. I made a RFE about it at https://bugzilla.redhat.com/show_bug.cgi?id=824198 and rebuilt Xaw locally, as I use xedit for pretty much any text editing :-) New package Spec URL: http://fedorapeople.org/~pcpa/xedit.spec SRPM URL: http://fedorapeople.org/~pcpa/xedit-1.2.0-1.fc18.src.rpm
Please correct the license, "BSD-like" is not a valide License tag: $ rpmlint xedit-1.2.0-1.fc18.src.rpm xedit.src: W: invalid-license BSD-like 1 packages and 0 specfiles checked; 0 errors, 1 warnings. Valid licenses are here in the second column: http://fedoraproject.org/wiki/Licensing#Good_Licenses I'm sure you will fix this prior to importing the package. For me this package is approved.
(In reply to comment #12) > (In reply to comment #11) > > [!]: MUST License field in the package spec file matches the actual license. > > > > The "COPYING" file contains multiple licenses, are you sure a simple "MIT" > > license is enough? > > The licenses are MIT or BSD-style without clauses. I also added > GPLv2+ because of the int64 patch actually adapts code from libgcc. Why the GPLv2+ here? This is in the patch: +/* based on code based on libgcc (that is GPLv3) + * version here doesn't return the result or MINSLONG if overflow + */ wouldn't that make it to GPLv3?? Please elaborate a bit more on that. realpath.c is BSD-4clause == BSD with advertising, which is GPL INCOMPAT! http://fedoraproject.org/wiki/Licensing#SoftwareLicenses (In reply to comment #13) > Please correct the license, "BSD-like" is not a valide License tag Which would be the right one, Simone? Also missing: "Which file is under which license" comment in the spec file: http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios Don't import this package till the licensing is completely clear!
(In reply to comment #14) > (In reply to comment #12) > > (In reply to comment #11) > > > [!]: MUST License field in the package spec file matches the actual license. > > > > > > The "COPYING" file contains multiple licenses, are you sure a simple "MIT" > > > license is enough? > > > > The licenses are MIT or BSD-style without clauses. I also added > > GPLv2+ because of the int64 patch actually adapts code from libgcc. > > Why the GPLv2+ here? > This is in the patch: > +/* based on code based on libgcc (that is GPLv3) > + * version here doesn't return the result or MINSLONG if overflow > + */ > > wouldn't that make it to GPLv3?? > > Please elaborate a bit more on that. I did that as a quick patch to one feature that is only available when evaluating lisp expressions in the "*scratch*" buffer. But that is very unlikely someone would ever use :-) The license update in the spec only refers to the patch, but I can rework it to use some different approach or update the spec to say GPLv3 (I do not recall if I adapted it from libgcc sources before or after switch to GPLv3) > realpath.c is BSD-4clause == BSD with advertising, which is GPL INCOMPAT! > http://fedoraproject.org/wiki/Licensing#SoftwareLicenses It was added to xedit source back in 1990's, but can be removed before starting the build, or replaced by a newer version, that should be GPL compatible. I did not add it back then, and it was added to build XFree86 on systems without a working realpath, strcasecmp, etc. I believe the 3 clause one, first google result, would do it http://www.openbsd.org/cgi-bin/cvsweb/src/lib/libc/stdlib/realpath.c?rev=1.14;content-type=text%2Fplain > (In reply to comment #13) > > Please correct the license, "BSD-like" is not a valide License tag > > Which would be the right one, Simone? I am used to write BSD-like as license tag for Mandriva packages, but I did overlook the COPYING file with a proper license audit, and it does indeed list BSD 4 clause. > Also missing: > "Which file is under which license" comment in the spec file: > http://fedoraproject.org/wiki/Packaging: > LicensingGuidelines#Multiple_Licensing_Scenarios > > Don't import this package till the licensing is completely clear! Ok. I do not have plans to do any work on xedit (other than packaging), and almost nothing was done in 2002-2012, but I still use it, unfortunately :-) But if everything were ok now I would not import/submit until the Xaw issue in https://bugzilla.redhat.com/show_bug.cgi?id=824198 is addressed.
(In reply to comment #14) > Why the GPLv2+ here? > This is in the patch: > +/* based on code based on libgcc (that is GPLv3) > + * version here doesn't return the result or MINSLONG if overflow > + */ > > wouldn't that make it to GPLv3?? > > Please elaborate a bit more on that. I wrote a new patch that checks overflow with some tests and then a division, instead of adapating libgcc one. > realpath.c is BSD-4clause == BSD with advertising, which is GPL INCOMPAT! > http://fedoraproject.org/wiki/Licensing#SoftwareLicenses > > (In reply to comment #13) > > Please correct the license, "BSD-like" is not a valide License tag > > Which would be the right one, Simone? I updated the package to use BSD 3 clause files, imported from latest OpenBSD repository. > Also missing: > "Which file is under which license" comment in the spec file: > http://fedoraproject.org/wiki/Packaging: > LicensingGuidelines#Multiple_Licensing_Scenarios > > Don't import this package till the licensing is completely clear! Ok. The patches were submitted upstream. The Xaw patch was applied to upstream git head, so, once libXaw is rebuilt, see https://bugzilla.redhat.com/show_bug.cgi?id=824198 I believe it should be ok. New package Spec URL: http://fedorapeople.org/~pcpa/xedit.spec SRPM URL: http://fedorapeople.org/~pcpa/xedit-1.2.0-2.fc18.src.rpm
Please set fedora-review-+ if applicable, as rawhide libXaw now has been updated to the latest version, that does not crash xedit built with gcc 4.7. BTW, a screenshot with xedit in my computer :-) http://fedorapeople.org/~pcpa/rawhide-sage-shell.png
I'm sorry for disturbing above... It was already approved by Simone Caronni, but the mail address is not in CC anymore -> adding again. @Simone, could you set fedora-review+ again, please?
Done. Don't know what's happened. Regards, --Simone
New Package SCM Request ======================= Package Name: xedit Short Description: Simple text editor for X Owners: pcpa Branches: InitialCC:
You forgot the branches in the SCM request. --Simone
Git done (by process-git-requests).
(In reply to comment #21) > You forgot the branches in the SCM request. > > --Simone devel branch is implicit, and I do not plan to maintain it for older, actually, already released distros.
Xedit is now available in rawhide.
Package Change Request ====================== Package Name: xedit New Branches: epel7 Owners: pcpa