This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 1039323 - Review Request: Weatherman
Review Request: Weatherman
Status: CLOSED DEFERRED
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-12-08 01:03 EST by pphan
Modified: 2015-07-21 10:56 EDT (History)
5 users (show)

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


Attachments (Terms of Use)

  None (edit)
Description pphan 2013-12-08 01:03:39 EST
Spec URL: https://www.dropbox.com/s/a3pvkh4iv8r7zd5/weatherman.spec
SRPM URL: http://matrix.senecac.on.ca/~pphan/weatherman/weatherman-1.2.2-1.fc18.src.rpm
Description: 
weatherman displays weather information from weatherbug.com on the command line. It is written entirely in bash and will run on most UNIX/Linux systems.

The latest version of weatherman was released on January 12, 2013 with the following changes:

Using metric units with extended forecasts works once again
Better column alignment when metric units are specified
Fixed a rare case where extended forecasts would be displayed incorrectly
No longer display units with empty values
Removed the program name and project URL from the output
Fedora Account System Username: pphan2
Comment 1 Christopher Meng 2013-12-08 02:05:02 EST
(In reply to pphan from comment #0)
> Spec URL: https://www.dropbox.com/s/a3pvkh4iv8r7zd5/weatherman.spec

Can you host your spec at http://matrix.senecac.on.ca/~pphan/weatherman/ also?

We don't like dropbox.

> SRPM URL:
> http://matrix.senecac.on.ca/~pphan/weatherman/weatherman-1.2.2-1.fc18.src.rpm
> Description: 
> weatherman displays weather information from weatherbug.com on the command
> line. It is written entirely in bash and will run on most UNIX/Linux systems.
> 
> The latest version of weatherman was released on January 12, 2013 with the
> following changes:
> 
> Using metric units with extended forecasts works once again
> Better column alignment when metric units are specified
> Fixed a rare case where extended forecasts would be displayed incorrectly
> No longer display units with empty values
> Removed the program name and project URL from the output

We don't need changelog as description.

> Fedora Account System Username: pphan2

We can't find your account so far.
Comment 2 Ralf Corsepius 2013-12-08 02:29:28 EST
(In reply to Christopher Meng from comment #1)
> (In reply to pphan from comment #0)
> We don't like dropbox.

Who is we? Submitters are free to choose whatever hosting service is suiteable to them.
Comment 3 Christopher Meng 2013-12-08 02:31:58 EST
(In reply to Ralf Corsepius from comment #2)
> (In reply to Christopher Meng from comment #1)
> > (In reply to pphan from comment #0)
> > We don't like dropbox.
> 
> Who is we? Submitters are free to choose whatever hosting service is
> suiteable to them.

Yes, but that's troublesome for reviewers, fedora-review -b won't work.
Comment 4 phuocphan12 2013-12-24 01:10:57 EST
Sorry, 
Not sure how I edit my post above so Ill reply to you. 
I upload to my school storage.
Spec URL: http://matrix.senecac.on.ca/~pphan/weatherman/weatherman.spec
SRPM URL: http://matrix.senecac.on.ca/~pphan/weatherman/weatherman-1.2.2-1.fc18.src.rpm

CORRECT Fedora Account System Username: pphan12 

(In reply to Christopher Meng from comment #1)
> (In reply to pphan from comment #0)
> > Spec URL: https://www.dropbox.com/s/a3pvkh4iv8r7zd5/weatherman.spec
> 
> Can you host your spec at http://matrix.senecac.on.ca/~pphan/weatherman/
> also?
> 
> We don't like dropbox.
> 
> > SRPM URL:
> > http://matrix.senecac.on.ca/~pphan/weatherman/weatherman-1.2.2-1.fc18.src.rpm
> > Description: 
> > weatherman displays weather information from weatherbug.com on the command
> > line. It is written entirely in bash and will run on most UNIX/Linux systems.
> > 
> > The latest version of weatherman was released on January 12, 2013 with the
> > following changes:
> > 
> > Using metric units with extended forecasts works once again
> > Better column alignment when metric units are specified
> > Fixed a rare case where extended forecasts would be displayed incorrectly
> > No longer display units with empty values
> > Removed the program name and project URL from the output
> 
> We don't need changelog as description.
> 
> > Fedora Account System Username: pphan2
> 
> We can't find your account so far.
Comment 5 Mukundan Ragavan 2014-01-24 23:26:16 EST
Hi pphan12,

I am also a new packager and here are some quick comments on the package. If something I mention here is wrong, I am sure someone will correct it.

* source0: url is pasted twice.

* License: BSD - The word 'license' need not be mentioned. It will cause rpmlint to throw errors.

* %description - This section should have lines less than 80 characters. You can take a look at the common rpmlint errors wiki page.

* Requires: curl - It is my belief that curl need not be added as requires. As far as I know it is not possible to remove curl from the system.

* %post and % preun scriptlets - the source file does not seem provide any info manual. So, I am not sure these scriptlets are needed.

* changelog - please check the format with other packages in the repository. Small changes are needed.

Hope I have not made any mistakes here .. :) Good luck.
Comment 6 nicolas.vieville 2014-10-08 17:35:30 EDT
Hello,

As a candidate packager for Fedora, and if you still want to produce this package, I could make an unofficial review for your proposed package.

I agree with all the point Mukudan gave you.

As the shell script of this package is made for bash (specific use of bash arrays), it would probably better to explicitly indicate that it is required in the requires section of the spec file, even if bash and curl are "low-level" needed packages for the system.

As the upstream web site points to github you should define global variables at the beginning of your spec file:

%global commit 0fe94cb2fb10ef556e82dcce22260717c579f87a
%global shortcommit %(c=%{commit}; echo ${c:0:7})

and then use them as this:

Source0:        https://github.com/subrosa2/weatherman/archive/%{commit}/weatherman-%{commit}.tar.gz

or like this:

%setup -qn %{name}-%{commit}

in order to pass rpmlint tests.

You should also use the directories predefined macros like this:

%install
install -D -m 0755 weatherman %{buildroot}%{_bindir}/weatherman
install -D -m 0644 weatherman.1.gz  %{buildroot}%{_mandir}/man1/weatherman.1.gz

%files
%{_bindir}/weatherman
%doc CHANGES LICENSE
%{_mandir}/man1/%{name}.1*

Maybe it would be also useful to had an empty %build section to avoid rpmlint warning.

If needed, I could provide you a modified spec file as an example.

Let me know if you want me to make an unofficial review of your package, once you modified your spec file.

Cordially,


-- 
NVieville
Comment 7 nicolas.vieville 2014-10-22 12:38:02 EDT
Hello,

I wonder if you could drop a line if you still interested in this review request and my proposed unofficial review. 

Thanks in advance.

Cordially,


-- 
NVieville
Comment 8 Miroslav Suchý 2015-07-21 10:56:17 EDT
Closing due long inactivity. Feel free to reopen if you want to continue.

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