Bug 815624 - Review Request: xedit - Simple text editor for X
Review Request: xedit - Simple text editor for X
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Simone Caronni
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-24 01:44 EDT by Paulo Andrade
Modified: 2014-08-01 09:03 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-07-05 10:08:53 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
negativo17: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Paulo Andrade 2012-04-24 01:44:25 EDT
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.
Comment 1 Paulo Andrade 2012-04-24 01:55:50 EDT
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.
Comment 2 Terje Røsten 2012-04-26 16:57:56 EDT
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.
Comment 3 Paulo Andrade 2012-04-26 18:22:26 EDT
(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
Comment 4 Terje Røsten 2012-04-27 01:49:40 EDT
Nice.

Please post direct, complete urls to updates spec and srpm on each time. 
Makes it easy to continue review.
Comment 5 Paulo Andrade 2012-04-27 07:46:25 EDT
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
Comment 6 Terje Røsten 2012-04-27 16:57:52 EDT
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?
Comment 7 Paulo Andrade 2012-04-27 20:14:23 EDT
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.
Comment 8 Thomas Spura 2012-04-30 16:00:02 EDT
pcpa just pointed out this review request to me.

FYI: I'm about to sponsor him in bug #817306.
Comment 9 Simone Caronni 2012-05-21 03:05:40 EDT
I will review this package
Comment 10 Simone Caronni 2012-05-21 03:23:18 EDT
==== 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.
Comment 11 Simone Caronni 2012-05-21 03:25:38 EDT
[!]: 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!
Comment 12 Paulo Andrade 2012-05-22 20:41:03 EDT
(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
Comment 13 Simone Caronni 2012-05-23 09:24:40 EDT
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.
Comment 14 Thomas Spura 2012-05-23 13:33:24 EDT
(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!
Comment 15 Paulo Andrade 2012-05-23 14:39:09 EDT
(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.
Comment 16 Paulo Andrade 2012-05-26 01:04:13 EDT
(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
Comment 17 Paulo Andrade 2012-07-02 15:14:32 EDT
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
Comment 18 Thomas Spura 2012-07-02 18:48:03 EDT
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?
Comment 19 Simone Caronni 2012-07-03 03:11:00 EDT
Done. Don't know what's happened.

Regards,
--Simone
Comment 20 Paulo Andrade 2012-07-03 08:30:34 EDT
New Package SCM Request
=======================
Package Name: xedit
Short Description: Simple text editor for X
Owners: pcpa
Branches: 
InitialCC:
Comment 21 Simone Caronni 2012-07-03 08:32:57 EDT
You forgot the branches in the SCM request.

--Simone
Comment 22 Gwyn Ciesla 2012-07-03 08:33:14 EDT
Git done (by process-git-requests).
Comment 23 Paulo Andrade 2012-07-03 09:20:32 EDT
(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.
Comment 24 Paulo Andrade 2012-07-05 10:08:53 EDT
Xedit is now available in rawhide.
Comment 25 Paulo Andrade 2014-08-01 08:53:35 EDT
Package Change Request
======================
Package Name: xedit
New Branches: epel7
Owners: pcpa
Comment 26 Gwyn Ciesla 2014-08-01 09:03:26 EDT
Git done (by process-git-requests).

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