Bug 985129 - Review Request: text2nato - text converter to nato phonetic alphabet
Review Request: text2nato - text converter to nato phonetic alphabet
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2013-07-16 17:12 EDT by Veaceslav Mindru
Modified: 2014-07-21 09:33 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-07-21 09:33:19 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Veaceslav Mindru 2013-07-16 17:12:37 EDT
Spec URL: https://sourceforge.net/p/text2nato/code/ci/master/tree/ 
SRPM URL: https://sourceforge.net/projects/text2nato/files/Fedora%2018%20rpm/
Description: When you need to communicate  surname/name/ complex naming or word via phone a common practice is used, to  spell the word using NATO Phonetic dictionary. http://en.wikipedia.org/wiki/NATO_phonetic_alphabet This tool will take on input any text and convert it into Nato Phonetic Alphabet. 

[root@localhost text2nato-code]# text2nato
NAME
        text2nato - text converter, converts text into nato phonetic alphabet
SYNOPSIS
        text2nato text...
DESCRIPTIONT
        This utility can be used, when you will have to spell an word/name/surname other the phone. It convert's text  into nato phonetic alphabet, so you can just read it from screen without searching your memorry for each letter
AUTHOR
        Veaceslav Mindru mindruv@gmail.com
[root@localhost text2nato-code]# text2nato convert this text
c       Charlie
o       Oscar
n       November
v       Victor
e       Echo
r       Romeo
t       Tango
t       Tango
h       Hotel
i       India
s       Sierra
t       Tango
e       Echo
x       X-ray
t       Tango
[root@localhost text2nato-code]#

 
Fedora Account System Username: mindruv
Comment 1 Christopher Meng 2013-07-16 21:25:56 EDT
Is it worth packaging?
Comment 2 Veaceslav Mindru 2013-07-17 01:36:15 EDT
Hello,  


regardless it's 10 bytes of code or 10MB. It's an usefull tool at least i find it usefull. I wrote it after i saw my collegues with Nato Phonetic printed on a paper 
and tracking the Alphabet with a finger each time they had to spell something.


Let's make world a better place to leave
Comment 3 Lubos Kocman 2013-07-17 02:45:36 EDT
Hello Christopher,

I personally see some value in case that you're talking with outsourced support center.

Lubos
Comment 4 Veaceslav Mindru 2013-07-17 02:57:59 EDT
Found following problems on SPEC file , will updateit. 

[root@localhost SPECS]# rpmlint  text2nato.spec
text2nato.spec:13: W: hardcoded-path-in-buildroot-tag /usr/bin/
text2nato.spec:34: W: macro-in-comment %{_bindir}
text2nato.spec: W: no-%prep-section
text2nato.spec: W: no-%build-section
text2nato.spec:4: W: mixed-use-of-spaces-and-tabs (spaces: line 4, tab: line 1)
0 packages and 1 specfiles checked; 0 errors, 5 warnings.
[root@localhost SPECS]#
Comment 5 Veaceslav Mindru 2013-07-17 04:46:52 EDT
Hello 


@Chris,

i updated spec file after running rpmlint on it. If you have a moment can you please recheck it. 


https://sourceforge.net/p/text2nato/code/ci/master/tree/text2nato.spec 
https://sourceforge.net/p/text2nato/code/ci/master/tree/  

@Lubos 

added the feature requested. 

[root@localhost text2nato-code]# ./text2nato Convert  this text 
C	Charlie
o	Oscar
n	November
v	Victor
e	Echo
r	Romeo
t	Tango

t	Tango
h	Hotel
i	India
s	Sierra

t	Tango
e	Echo
x	X-ray
t	Tango
[root@localhost text2nato-code]# 





VM
Comment 6 Michael Schwendt 2013-07-17 05:37:09 EDT
> Name: text2nato
> Summary: text2nato is an utility to convert text into nato phonetic 

Package installer tools typically display the Name and the Summary, so repeating the name at the beginning of the summary is a waste of space.
https://fedoraproject.org/wiki/Examples_of_good_package_summaries

Summary: Convert text into nato phonetic alphabet


> BuildRequires: perl

Not true yet. You only install a prebuilt file.


> BuildRoot: %{_tmppath}/  

https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag


> License: GPLv2+	

https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Clarification

and

https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text


> %files
> /usr/bin/text2nato

> mkdir -p %{buildroot}%{_bindir}/
> cp -p text2nato %{buildroot}%{_bindir}/

If you use macros, use them consistently. Choose between /usr/bin and %{_bindir} but not mix both forms.

https://fedoraproject.org/wiki/Packaging:Guidelines#Macros


> %install
> rm -rf %{buildroot} 

https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag


> %changelog

Do practise maintaining this %changelog section. Even during Fedora Package Review, there ought to be at least a first changelog entry.

https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs
Comment 7 Veaceslav Mindru 2013-07-26 10:50:21 EDT
Name: text2nato		
Version: 0.4
Release: 1%{?dist}
Summary:  Convert text into nato phonetic alphabet 	

Group: Applications/Text
License: GPLv2+	

%description
Convert text into nato phonetic alphabet

%files
/usr/bin/text2nato 

%install
rm -rf %{buildroot}
mkdir -p %{buildroot}%{_bindir}/
cp -p text2nato  %{buildroot}%{_bindir}/


%changelog

* Fri Jul 23 2013 Veaceslav Mindru <mindruv@gmail.com> 
- 0.4
- remove -i -s switches and left only on option 
- reduced the code and refactored 


#############################################################

still to be fixed 

"If you use macros, use them consistently. Choose between /usr/bin and %{_bindir} but not mix both forms."
Comment 8 Veaceslav Mindru 2013-07-26 11:57:24 EDT
Hopefully last version. 


Name: text2nato		
Version: 0.5
Release: 1%{?dist}
Summary:  Convert text into nato phonetic alphabet 	

Group: Applications/Text
License: GPLv2+	

%description
Convert text into nato phonetic alphabet

%files
%{_bindir}/text2nato 

%install
mkdir -p %{buildroot}%{_bindir}
cp -p text2nato  %{buildroot}%{_bindir}
cp -p text2nato  %{_bindir}/


%changelog

* Fri Jul 23 2013 Veaceslav Mindru <mindruv@gmail.com> 
- 0.5
- remove -i -s switches and left only one option 
- reduced the code and refactored 
- added space between words
Comment 9 Michael Schwendt 2013-07-27 03:40:46 EDT
There are several issues. For example, see comment 4. I hope I've found all of them, but I could not find a working package:


* It would be helpful, if with each new release of your package, you provided a tested pair of src.rpm and spec file at a direct download location, so tools like fedora-review/wget/curl could access them directly:
  https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Upload_Your_Package


* Run rpmlint (or rpmlint -I for more helpful output) on the src.rpm and all built rpms. Feel free to ignore obvious false positives in the report, but fix anything else. Preferably add a comment here about whether/when you think what rpmlint reports is correct or incorrect.


* The src.rpm package is incomplete and doesn't build at all yet. The reason is simple (but easy to miss when only skimming over the spec), it contains only the spec file, not the program script:

  cp: cannot stat 'text2nato': No such file or directory

In the %install section, the spec file assumes that this file exists in the current directory, the "build directory", but there is nothing that copies it to that location. There is no %prep section in the spec file where you would set up the builddir and copy the file into it.

Since you don't need the builddir, you don't need a %prep section. However, there is no "Source" tag in the spec file either, which would point at the download location for the text2nato script. Therefore, that file is not available and not included in your src.rpm yet. If, for example, you pointed the Source0 tag at the file's download URL (test it via "spectool -g text2nato.spec"), you could access the file via %{SOURCE0} in the %install section.

  https://fedoraproject.org/wiki/Packaging:SourceURL
  https://fedoraproject.org/wiki/Packaging:Guidelines#Tags
  https://fedoraproject.org/wiki/Packaging:ReviewGuidelines

I can't tell how you've built the src.rpm so far, but there's a fundamental error in how you do it.


* Bogus date in %changelog warning when building the rpm: "Fri Jul 23 2013" needs to be fixed to either Jul 26 or Tue.


* cp -p text2nato  %{_bindir}/

This is a line you've added recently. Why? The previous line that copies the file into %{buildroot}/%{_bindir} is fine. Files in %buildroot get included in the package via the %files section. This new line makes the build fail (when not building as "root"), and the package must install into the %buildroot directory, not into the system's file system.


* Licensing: See comment 6.


* %changelog: The RPM package changelog is for packaging related comments. Typically, you would not list all the changes in the text2nato script, but only anything relevant that has changed in the package. For a tiny package like this, that wouldn't be a lot.
https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs


* Just a Perl script -> package should set "BuildArch: noarch"
Comment 10 Veaceslav Mindru 2013-07-29 08:38:05 EDT
Hello ,


@ Michael Schwendt 
first of all thank you for your patients with me, doing it for the first time get's confusing sometimes. 

Spec filed uploaded  also tested SRPM 

Spec URL: https://sourceforge.net/p/text2nato/code/ci/master/tree/ 
SRPM URL: https://sourceforge.net/projects/text2nato/files/Fedora%2018%20rpm/

i don't see any new errors reported on rpmlint 
[root@localhost text2nato-code]# rpmlint  text2nato.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.



Here is the last version of text2nato.spec %build section added and empty as per rpmlint requirements 

[root@localhost text2nato-code]# cat text2nato.spec 
Name: text2nato		
Version: 0.5
Release: 1%{?dist}
Summary: Convert text into nato phonetic alphabet 
Source0: https://sourceforge.net/projects/%{name}/files/%{name}
BuildArch: noarch

Group: Applications/Text
License: GPLv2+	

%description
Convert text into nato phonetic alphabet

%build
%prep
cp -p %{SOURCE0} %{_builddir} 

%files
%{_bindir}/%{name}

%install
mkdir -p %{buildroot}%{_bindir}
cp -p %{name}  %{buildroot}%{_bindir}


%changelog

* Mon Jul 29 2013 Veaceslav Mindru <mindruv@gmail.com> 
- 0.5
- remove -i -s switches and left only one option 
- reduced the code and refactored 
- added space between words 
- updated spec file as per fedora community requirements 
[root@localhost text2nato-code]#
Comment 11 Veaceslav Mindru 2013-07-29 08:40:48 EDT
One more thing regarding the License filed , comment 6 is not quite clear for me yet, going to review it again.
Comment 12 Veaceslav Mindru 2013-07-29 09:10:10 EDT
Added LICENSE file with Artistic clarified license to RPM and SRPM, updated spec file. 

Veaceslav Mindru
Comment 13 Veaceslav Mindru 2013-08-01 07:29:54 EDT
OK while reviewing other packages, found one more thing to correct. Spec file modified available by link in Description https://bugzilla.redhat.com/show_bug.cgi?id=985129#c0 

-Source0: http://sourceforge.net/projects/%{name}/files/%{name}
-Source1:  http://sourceforge.net/projects/%{name}/files/LICENSE
+Source0: http://sourceforge.net/projects/%{name}/files/%{name}-%{version}.tar.gz



-cp -p %{SOURCE0} %{_builddir} 
-cp -p %{SOURCE1} %{_builddir}
+%setup -q 



Name: text2nato
Version: 0.5
Release: 1%{?dist}
Summary: Convert text into nato phonetic alphabet
Source0: http://sourceforge.net/projects/%{name}/files/%{name}-%{version}.tar.gz
BuildArch: noarch
Group: Applications/Text
License: Artistic clarified
%description
Convert text into nato phonetic alphabet
%build
%prep
%setup -q
%files
%doc LICENSE
%{_bindir}/%{name}
%install
mkdir -p %{buildroot}%{_bindir}
chmod a+x %{name}
cp -p %{name} %{buildroot}%{_bindir}
%changelog
* Mon Jul 29 2013 Veaceslav Mindru <mindruv@gmail.com>
- 0.5
- remove -i -s switches and left only one option
- reduced the code and refactored
- added space between words
- updated spec file as per fedora community requirements
Comment 14 Michael Schwendt 2013-08-16 15:14:12 EDT
It's less easy to review a spec file posted as a bugzilla comment. Where is the src.rpm?

Also, if you could add a comment with valid "Spec URL:" and "SRPM URL:" lines, you could run "fedora-review -b 985129" and let that tool perform several checks.


> %build
> %prep

Even if the %build section is empty, it should be placed after %prep. These sections are processed in the following order, and it makes sense if that's reflected within the spec file: %prep -> %build -> %install -> %check -> %files

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