Bug 515136 - Review Request: gettext-commons - Java internationalization (i18n) library
Review Request: gettext-commons - Java internationalization (i18n) library
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mat Booth
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 508351
  Show dependency treegraph
 
Reported: 2009-08-02 10:39 EDT by Andrea Musuruane
Modified: 2009-11-16 02:38 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-10-31 13:15:18 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
mat.booth: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
mock build log (11.50 KB, text/plain)
2009-08-12 16:27 EDT, Cédric OLIVIER
no flags Details

  None (edit)
Description Andrea Musuruane 2009-08-02 10:39:25 EDT
Spec URL: http://www.webalice.it/musuruan/RPMS/reviews/gettext-commons.spec
SRPM URL: http://www.webalice.it/musuruan/RPMS/reviews/gettext-commons-0.9.6-1.fc10.src.rpm

Description:
The Gettext Commons project provides Java classes for internationalization 
(i18n) through GNU gettext.

The lightweight library combines the power of the unix-style gettext tools 
with the widely used Java ResourceBundles. This makes it possible to use the 
original text instead of arbitrary property keys, which is less cumbersome 
and makes programs easier to read.

Rpmlint output is clean.

Please note that this is my first JAVA package.
Comment 1 Andrea Musuruane 2009-08-10 15:02:27 EDT
Spec URL: http://www.webalice.it/musuruan/RPMS/reviews/gettext-commons.spec
SRPM URL:
http://www.webalice.it/musuruan/RPMS/reviews/gettext-commons-0.9.6-2.fc10.src.rpm

Changelog:
* Sun Aug 09 2009 Andrea Musuruane <musuruan@gmail.com> 0.9.6-2
- Created JAR alias
Comment 2 Cédric OLIVIER 2009-08-12 16:27:40 EDT
Created attachment 357233 [details]
mock build log

I can't generate any rpm for gettext-commons with mock (tested on fc10 and fc11)
Comment 4 Mat Booth 2009-10-14 16:21:35 EDT
I will take this one. A full review is forthcoming.

</Cherry-Pick>
Comment 5 Mat Booth 2009-10-15 14:36:29 EDT
OK, I've gone through the packaging guidelines and the reviewing guidelines...



Please fix the following:

I cannot install the javadoc subpackage because of line 35, which reads:
  Requires:       %{name}-%{version}-%{release}
It should instead read:
  Requires:       %{name} = %{version}-%{release}

You mix $THIS_STYLE and %{this_style} variables, which is bad form so the guidelines say. You can easily fix this by replacing all instances of $RPM_BUILD_ROOT with %{buildroot}

This one is your choice, but by convention javadoc is installed into a versioned directory %{buildroot}%{_javadocdir}/%{name}-%{version} and then an unversioned symlink is made %{buildroot}%{_javadocdir}/%{name} to the versioned directory. (Just like you did with the jar file.)



Other points:

Have you sent the javadoc.patch upstream? Seems like one they could accept easily. If you have sent this upstream, please include a link the bug report in a comment.



Once you've addressed these points, I'll give it another look and then probably you're ready to go. Thanks for your submission (and thanks for waiting patiently for someone to review it)  :-)
Comment 6 Andrea Musuruane 2009-10-17 08:53:56 EDT
Spec URL: http://www.webalice.it/musuruan/RPMS/reviews/gettext-commons.spec
SRPM URL:
http://www.webalice.it/musuruan/RPMS/reviews/gettext-commons-0.9.6-4.fc10.src.rpm

Changelog:
- Submitted javadoc patch upstream
- Fixed javadoc package requires
- Created javadoc directory alias
Comment 7 Andrea Musuruane 2009-10-17 09:01:55 EDT
(In reply to comment #5)
> Please fix the following:
> 
> I cannot install the javadoc subpackage because of line 35, which reads:
>   Requires:       %{name}-%{version}-%{release}
> It should instead read:
>   Requires:       %{name} = %{version}-%{release}

Fixed.

> You mix $THIS_STYLE and %{this_style} variables, which is bad form so the
> guidelines say. You can easily fix this by replacing all instances of
> $RPM_BUILD_ROOT with %{buildroot}

AFAIK What I did is perfectly acceptable. I didn't use both $RPM_BUILD_ROOT and %{buildroot} - I only used the first. 

Moreover, all other macros are in the second style because they do not have an equivalent in the first syntax. 

You can find a great number of spec files that do the same in Fedora CVS.

> This one is your choice, but by convention javadoc is installed into a
> versioned directory %{buildroot}%{_javadocdir}/%{name}-%{version} and then an
> unversioned symlink is made %{buildroot}%{_javadocdir}/%{name} to the versioned
> directory. (Just like you did with the jar file.)

Fixed.

> Other points:
> 
> Have you sent the javadoc.patch upstream? Seems like one they could accept
> easily. If you have sent this upstream, please include a link the bug report in
> a comment.

Submitted upstream.

> Once you've addressed these points, I'll give it another look and then probably
> you're ready to go. Thanks for your submission (and thanks for waiting
> patiently for someone to review it)  :-)  

Thanks _a lot_ for the review :)
Comment 8 Mat Booth 2009-10-19 07:13:59 EDT
(In reply to comment #7)
> (In reply to comment #5)
> > You mix $THIS_STYLE and %{this_style} variables, which is bad form so the
> > guidelines say. You can easily fix this by replacing all instances of
> > $RPM_BUILD_ROOT with %{buildroot}
> 
> AFAIK What I did is perfectly acceptable. I didn't use both $RPM_BUILD_ROOT and
> %{buildroot} - I only used the first. 
> 
> Moreover, all other macros are in the second style because they do not have an
> equivalent in the first syntax. 
> 
> You can find a great number of spec files that do the same in Fedora CVS.
> 

Fair enough.

I'm happy to rubber-stamp this now. APPROVED.
Comment 9 Andrea Musuruane 2009-10-19 14:46:04 EDT
New Package CVS Request
=======================
Package Name: gettext-commons
Short Description: Java internationalization (i18n) library
Owners: musuruan
Branches: F-10 F-11
InitialCC:
Comment 10 Kevin Fenzi 2009-10-22 00:07:59 EDT
cvs done with F-12 branch added.
Comment 11 Fedora Update System 2009-10-31 12:58:49 EDT
gettext-commons-0.9.6-4.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/gettext-commons-0.9.6-4.fc11
Comment 12 Fedora Update System 2009-10-31 13:07:27 EDT
gettext-commons-0.9.6-4.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/gettext-commons-0.9.6-4.fc10
Comment 13 Andrea Musuruane 2009-10-31 13:15:18 EDT
Built. Closing.
Comment 14 Fedora Update System 2009-11-16 02:37:08 EST
gettext-commons-0.9.6-4.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 15 Fedora Update System 2009-11-16 02:38:19 EST
gettext-commons-0.9.6-4.fc11 has been pushed to the Fedora 11 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.