Bug 460000 - (rxtx) Review Request: rxtx - Parallel communication for the Java Development Toolkit
Review Request: rxtx - Parallel communication for the Java Development Toolkit
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-08-25 11:27 EDT by Levente Farkas
Modified: 2008-10-24 19:50 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-10-23 09:32:00 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
huzaifas: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Levente Farkas 2008-08-25 11:27:46 EDT
Spec URL: http://www.lfarkas.org/linux/packages/redhat/5/SPECS/rxtx.spec
SRPM URL: http://www.lfarkas.org/linux/packages/redhat/5/SRPMS/rxtx-2.1-7r2.1.src.rpm
Description: rxtx is an full implementation of java commapi which aims to support RS232 IEEE 1284, RS485, I2C and RawIO.
Comment 1 Mamoru TASAKA 2008-09-12 12:09:17 EDT
Some initial comments

* Versioning
  https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Post-Release_packages
  - Under your usage of macros, the version should be
    %{rel}.%{uprel}%{?dist} 
    (Fedora specific release number comes first: this is for the safety
     of bumping release number)

* License
  - For this package the license tag must be "LGPLv2+".

* Vendor
  - Remove Vendor tag. This is automatically tagged on Fedora side.

* precompiled binaries
  - The source tarball contains some precompiled binaries (mainly
    .jar Java archive), which must all be removed at %prep.
    https://fedoraproject.org/wiki/Packaging/Java#Pre-built_JAR_files_.2F_Other_bundled_software

* %PATCH1
--------------------------------------------------------------
sed -e 's|@JNIPATH@|%{jni}|' %{PATCH1} | patch -p1
--------------------------------------------------------------
  - Use "patch -s" (maybe you want to use "patch -s -b --suffix .p1")

* configure
--------------------------------------------------------------
CFLAGS="$RPM_OPT_FLAGS" LDFLAGS=-s ./configure --prefix=/usr
--------------------------------------------------------------
  - Just use %configure (please check what %configure does by
    $ rpm --eval %configure )
  - Auto, automated stripping is forbidden. Stripping binaries
    must be done by /usr/lib/rpm/find-debuginfo.sh

* parallel make
  - Support parallel make when possible. When it is impossible,
    add a comment in the spec file:
    https://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make

* libtool .la files
  - should usually be removed.
    https://fedoraproject.org/wiki/Packaging/Guidelines#Exclusion_of_Static_Libraries

* Directory ownership issue
  - The directory %_libdir/%name is not owned by any package.
    Please refer to
    https://fedoraproject.org/wiki/PackagingDrafts/UnownedDirectories#Wildcarding_Files_inside_a_Created_Directory

* Documents
  - Please add the following files to %doc.
---------------------------------------------------------------
AUTHORS
COPYING
ChangeLog
---------------------------------------------------------------

* %changelog
---------------------------------------------------------------
rxtx.i386: W: incoherent-version-in-changelog 2.1 2.1-7r2.1.fc10
---------------------------------------------------------------
  - The number "2.1" is not sufficient in the %changelog.
    You should use "2.1-1.7r2", for example. Please refer to:
    https://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs
Comment 2 Mamoru TASAKA 2008-09-12 12:12:30 EDT
One more note:
* Architecture specific
  - Some codes in this tarball actually use funtions in sys/io.h, so this package
    does not compile on ppc/ppc64. Please use ExcludeArch or ExclusiveArch.
Comment 3 Levente Farkas 2008-09-15 08:12:29 EDT
hi,
i fixed all the above things. i hope it's now better.
Comment 4 Mamoru TASAKA 2008-09-15 10:00:46 EDT
Please provide the URL of your new spec/srpm so that we can find them easily
(also please change the release number every time you modify your spec file)
Comment 6 Mamoru TASAKA 2008-09-16 12:10:07 EDT
Well,
* As this uses autogen.sh, "BuildRequires: libtool automake" is needed:
  http://koji.fedoraproject.org/koji/taskinfo?taskID=827920

* and it seems that in ExcludeArch comma cannot be used (i.e. ExcludeArch: ppc ppc64)

* However even if I add these BuildRequires, this time the build fails.
  Would you verify it?
  http://koji.fedoraproject.org/koji/taskinfo?taskID=828328
Comment 7 Mamoru TASAKA 2008-09-16 12:27:46 EDT
(In reply to comment #6)
> * However even if I add these BuildRequires, this time the build fails.
>   Would you verify it?
>   http://koji.fedoraproject.org/koji/taskinfo?taskID=828328
Not using parallel make seems good:
http://koji.fedoraproject.org/koji/taskinfo?taskID=828362

will check later.
Comment 8 Mamoru TASAKA 2008-09-17 03:14:19 EDT
Okay, for -2:

* BuildRequires
  - this autogen.sh "Requires: automake, libtool"

* ExcludeArch:
  - In ExcludeArch comma does not seem to work (i.e.
    use "ExcludeArch: ppc ppc64)

* parallel make
  - parallel make failed, so please remove %{_smp_mflags}
    and write in the spec file as comments that parallel
    make fails.

* %files entry
  - build.log shows:
----------------------------------------------------
   723  warning: File listed twice: /usr/lib/rxtx/librxtxI2C-2.1-7.so
   724  warning: File listed twice: /usr/lib/rxtx/librxtxI2C.so
   725  warning: File listed twice: /usr/lib/rxtx/librxtxParallel-2.1-7.so
   726  warning: File listed twice: /usr/lib/rxtx/librxtxParallel.so
   727  warning: File listed twice: /usr/lib/rxtx/librxtxRS485-2.1-7.so
   728  warning: File listed twice: /usr/lib/rxtx/librxtxRS485.so
   729  warning: File listed twice: /usr/lib/rxtx/librxtxRaw-2.1-7.so
   730  warning: File listed twice: /usr/lib/rxtx/librxtxRaw.so
   731  warning: File listed twice: /usr/lib/rxtx/librxtxSerial-2.1-7.so
   732  warning: File listed twice: /usr/lib/rxtx/librxtxSerial.so
----------------------------------------------------
    The %files entry
----------------------------------------------------
%files
%{jni}
----------------------------------------------------
    contains the directory %{jni} itself and all files/directories/etc
    under %{jni} directory. (so %{jni}/* is no longer needed).

* rpmlint issue
----------------------------------------------------
rxtx.i386: W: file-not-utf8 /usr/share/doc/rxtx-2.1/ChangeLog
----------------------------------------------------
  - Change the encoding to UTF-8 (I don't know the original
    encoding...)
Comment 9 Levente Farkas 2008-09-25 07:22:23 EDT
i change your requests and the result seems to be correct, you can check it in:
http://koji.fedoraproject.org/koji/taskinfo?taskID=843567
Comment 10 Mamoru TASAKA 2008-09-26 02:27:57 EDT
Two issues.

* Macros in comment
--------------------------------------------------------------------
# parallel make fails with make %{?_smp_mflags}
--------------------------------------------------------------------
  - In comments or %changelog, use %% instead of % to make it ensure that
    macros are not expanded.

* build failure
  - Unfortunately now build fails on dist-f10:
    http://koji.fedoraproject.org/koji/taskinfo?taskID=844844

    With the following change build seems to succeed (on dist-f10 and
    dist-f9-updates-candidate)
---------------------------------------------------------------------
%build
./autogen.sh

export JAVA_HOME=%{java_home}
%configure
# parallel make fails with make %%{?_smp_mflags}
make
---------------------------------------------------------------------
    Note that you can find out how %java_home is determined by $ rpm --showrc

Now this package itself is okay, so

-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on:
http://fedoraproject.org/PackageReviewStatus/NEW.html
(NOTE: please don't choose "Merge Review")


Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
-------------------------------------------------------------

For your case I found two other review requests by you (bug 454667, bug 455426).
About these 2 review requests:
- bug 454667 is currently blocked by legal issue, so I don't want to check this.
- bug 455426 has several issues

So I will postpone the approval of this package until you do a pre-review of 
other person's review request or you update bug 455426 (I will comment on
bug 455426)
Comment 11 Mamoru TASAKA 2008-09-29 14:38:26 EDT
Please fix the two issue above when you import this package into CVS.

- This package itself is now okay (when modified)
- Your another review request (bug 455426) is now good to some extent.

--------------------------------------------------------------
    This package (rxtx) is APPROVED by mtasaka
--------------------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Install the Client Tools (Koji)". Now I am sponsoring you.

If you want to import this package into Fedora 8/9, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.

Removing NEEDSPONSOR.
Comment 12 Mamoru TASAKA 2008-10-08 03:18:51 EDT
ping?
Comment 13 Levente Farkas 2008-10-08 08:37:28 EDT
New Package CVS Request
=======================
Package Name: rxtx
Short Description: Serial and Parallel communication for Java
Owners: lfarkas
Branches: F-9 F-10 EL-5
InitialCC: lfarkas
Comment 14 Huzaifa S. Sidhpurwala 2008-10-08 22:42:12 EDT
cvs done
Comment 15 Mamoru TASAKA 2008-10-16 11:15:37 EDT
ping?
Comment 16 Levente Farkas 2008-10-16 18:26:32 EDT
can't checkout the cvs it seems the directory still not exists..
Comment 17 Mamoru TASAKA 2008-10-17 10:41:49 EDT
I can checkout rxtx module.
Comment 18 Mamoru TASAKA 2008-10-22 09:39:52 EDT
Does it still seem than you cannot check out rxtx module from Fedora CVS?
Would you try it again? (If you can check out, please try to build).
Comment 19 Levente Farkas 2008-10-22 14:55:08 EDT
i already check out and in on monday, unfortunately i can't build it and this week a long national holiday in hungary, but i hope i'll have some free time for this on the next week.
Comment 20 Levente Farkas 2008-10-23 08:45:52 EDT
build it, and in the pending queue and in the epel need-signing queue too.
Comment 21 Mamoru TASAKA 2008-10-23 09:32:00 EDT
Okay, thanks.

Now closing. If you have some questions please feel free to
mail to me.
Comment 22 Fedora Update System 2008-10-24 19:50:47 EDT
rxtx-2.1-0.2.7r2.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

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