Bug 1008063 (ag)

Forgot rpmlint output:

% rpmlint the_silver_searcher.spec ../SRPMS/the_silver_searcher-0.16-2.fc19.src.rpm ../RPMS/x86_64/the_silver_searcher-0.16-2.fc19.x86_64.rpm
the_silver_searcher.src: W: spelling-error Summary(en_US) ack -> ac, ck, sack
the_silver_searcher.src: W: spelling-error %description -l en_US ack -> ac, ck, sack
the_silver_searcher.src: W: spelling-error %description -l en_US gitignore -> git ignore, git-ignore, ignore
the_silver_searcher.src: W: spelling-error %description -l en_US hgignore -> ignore
the_silver_searcher.src: W: spelling-error %description -l en_US repo -> rope, rep, reps
the_silver_searcher.src: W: spelling-error %description -l en_US agignore -> ignore
the_silver_searcher.src: W: spelling-error %description -l en_US extern -> ex tern, ex-tern, external
the_silver_searcher.src: W: spelling-error %description -l en_US regex -> regexp, reg ex, reg-ex
the_silver_searcher.src: W: spelling-error %description -l en_US strstr -> strut
the_silver_searcher.src: W: spelling-error %description -l en_US mmap -> map, m map, mamma
the_silver_searcher.src: W: spelling-error %description -l en_US pcre -> pare, acre, pore
the_silver_searcher.src: W: spelling-error %description -l en_US jillion -> gillion, million, pillion
the_silver_searcher.src: W: spelling-error %description -l en_US fnmatch -> match
the_silver_searcher.x86_64: W: spelling-error Summary(en_US) ack -> ac, ck, sack
the_silver_searcher.x86_64: W: spelling-error %description -l en_US ack -> ac, ck, sack
the_silver_searcher.x86_64: W: spelling-error %description -l en_US gitignore -> git ignore, git-ignore, ignore
the_silver_searcher.x86_64: W: spelling-error %description -l en_US hgignore -> ignore
the_silver_searcher.x86_64: W: spelling-error %description -l en_US repo -> rope, rep, reps
the_silver_searcher.x86_64: W: spelling-error %description -l en_US agignore -> ignore
the_silver_searcher.x86_64: W: spelling-error %description -l en_US extern -> ex tern, ex-tern, external
the_silver_searcher.x86_64: W: spelling-error %description -l en_US regex -> regexp, reg ex, reg-ex
the_silver_searcher.x86_64: W: spelling-error %description -l en_US strstr -> strut
the_silver_searcher.x86_64: W: spelling-error %description -l en_US mmap -> map, m map, mamma
the_silver_searcher.x86_64: W: spelling-error %description -l en_US pcre -> pare, acre, pore
the_silver_searcher.x86_64: W: spelling-error %description -l en_US jillion -> gillion, million, pillion
the_silver_searcher.x86_64: W: spelling-error %description -l en_US fnmatch -> match
the_silver_searcher.x86_64: W: conffile-without-noreplace-flag /etc/bash_completion.d/ag.bashcomp.sh
2 packages and 1 specfiles checked; 0 errors, 27 warnings.


All but one are “spelling errors” that aren't really spelling errors, and the missing noreplace flag is intentional as I don't think the bash completion file should be changed (I can be convinced otherwise, though).

Hello Henrik,

This isn't an official review. I've just taken a look and have a couple of comments.

* You should specify man pages in the %files section as glob patterns such as "%{_mandir}/man1/ag.1*", to avoid errors when/if we change the man page compression sometime in the future

* The binary RPM has a conflict: /usr/bin/ag is already provided by the package python-ase (I've only checked on f19)

* The package name concerns me. I don't think underscores in package name is strictly disallowed, but it's frowned upon and should be avoided.

-trond

Hello Trond,

Thanks for the unofficial review!

I chose the package name "the_silver_searcher" as that seems to be what is used in most other package managers. I see that Ubuntu and Debian uses "silversearcher-ag", though, so I could change it to that.

The binary conflict is a little trickier, I'll have to come up with a different name. I've noticed that "ack" (a similar program) uses "ack-grep" in some locations.

I updated the man page %files in my local spec, but some errors showed up when attempting to rename the package (the Makefile is still installing some things to /usr/share/the_silver_searcher). I'll post links to a new spec and rpm file when I get that working.

Thanks again,
Henrik

While it's not part of the guidelines, please read http://mm3test.fedoraproject.org/hyperkitty/list/devel@mm3test.fedoraproject.org/thread/4PV7CQUQ2POT52EDMRUKZUKR4PEQYEG6/

I'll do the formal review.

Ok, I updated the spec/srpm to use the manpage glob.

I looked into renaming the binary. This would require some patches, both to make the Makefile build to the new binary (although this could be done with `mv`, but also to update the included docs (such as `ag --help`). There was an issue about this reported upstream, but doesn't look like the maintainer intends on changing the name: https://github.com/ggreer/the_silver_searcher/issues/155.

I'm not sure which is more preferred in this case -- patching or adding a conflict to the spec?

As for the package name, this is from http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Separators:

There are a few exceptions to the no underscore '_' rule.
    packages where the upstream name naturally contains an underscore are excluded from this.

In this case I believe this package goes under that exception, seeing as the upstream project is named the_silver_searcher. Another alternative name could be "ag", but that might be confusing considering the duplicate binary.

And description is not good, too.

**********
> Description: The Silver Searcher
> An attempt to make something better than ack (which itself is better than grep).

Well, is it a sentence?

IMHO it should be at least:

The Silver Searcher is an attempt to make something better than ack (which itself is better than grep).

> Why use Ag?

Ah, what is "Ag"?

> * It searches code about 3–5× faster than ack.
> * It ignores file patterns from your .gitignore and .hgignore.
> * If there are files in your source repo you don't want to search, just add
>   their patterns to a .agignore file. *cough* extern *cough*
> * The command name is 33% shorter than ack!

Well, I think these are not very helpfule for users.

> How is it so fast?

You don't need to include this line, actually, every software would like to describe how fast it is but in fact this depends on many aspects. You should mark below lines as features. 

> * Searching for literals (no regex) uses Boyer-Moore-Horspool strstr.
> * Files are mmap()ed instead of read into a buffer.
> * If you're building with PCRE 8.21 or greater, regex searches use the JIT
>   compiler.
> * Ag calls pcre_study() before executing the regex on a jillion files.
> * Instead of calling fnmatch() on every pattern in your ignore files, non
> -regex
>   patterns are loaded into an array and binary searched.
> * Ag uses Pthreads to take advantage of multiple CPU cores and search files in
>   parallel.

Ok there are many problems here :(

For the package name, I'd go for `the_silver_searcher' because it's both the upstream name and the most used name on other distros (AFAIK, even on non Linuxes).

If you still want to rename the package, there is a $(pkgdatadir) variable in Mafefile.am that you can probably override at configure or build time.

For the description, I wouldn't put the implementation details as Christopher suggests, and I'd stick to the upstream description:

```
A code searching tool similar to ack, with a focus on speed.

What's so great about Ag?

* It searches code about 3–5× faster than ack.
* It ignores file patterns from your .gitignore and .hgignore.
* If there are files in your source repo you don't want to search,
  just add their patterns to a .agignore file. *cough* extern *cough*
* The command name is 33% shorter than ack!
```

And the real problem is the /usr/bin/ag conflict, I didn't see this one coming. I'll probably need the help of a more experienced packager.

(In reply to Dridi Boukelmoune from comment #8)

Reporter must make it clear.

> * It searches code about 3–5× faster than ack.

Who knows?

> * The command name is 33% shorter than ack!

Well, sounds incredible, right? I'd suggest using "a" or "g" as binary name so it can be 66% shorter than ack.

For the description I just used the description from the upstream spec file (https://github.com/ggreer/the_silver_searcher/blob/master/the_silver_searcher.spec.in). I agree it's not the best, though, so I updated it taking some of your comments into account.

I'll stick with the name "the_silver_searcher" for the package, as it seems the most used, and it was also the first thing I searched for when trying to find out whether the package already existed.

As for the duplicate binary, I'll see if I can find if something similar has happened before.

(In reply to Christopher Meng from comment #9)
> (In reply to Dridi Boukelmoune from comment #8)
> 
> Reporter must make it clear.
> 
> > * It searches code about 3–5× faster than ack.
> 
> Who knows?

I get your point, we can remove this line for the sake of not bashing ack. The claim comes from the upstream itself, but I haven't found backing benchmarks with a *quick* search.

> > * The command name is 33% shorter than ack!
> 
> Well, sounds incredible, right? I'd suggest using "a" or "g" as binary name
> so it can be 66% shorter than ack.

Come on ! This is obviously a joke, and ag is the name for silver in the periodic table.

Which brings me to the next topic, the conflict:
https://fedoraproject.org/wiki/Packaging:Conflicts#Incompatible_Binary_Files_with_Conflicting_Naming_.28and_stubborn_upstreams.29

Since ag is a play on word (like the hg command for mercurial), I'd rather ask the other project whether it could rename its ag binary. But if it's a command line tool, that could break existing (user) scripts.

> For the package name, I'd go for `the_silver_searcher' because
> it's both the upstream name 

For the executable. Most likely with "space" deliberately replaced by "underscore" to concatenate the three words.


Several upstream files call the project "The Silver Searcher". For example but not limited to these:

  http://geoff.greer.fm/2011/12/27/the-silver-searcher-better-than-ack/

  https://github.com/ggreer/the_silver_searcher/blob/master/NOTICE
  https://github.com/ggreer/the_silver_searcher/blob/master/README.md
  https://github.com/ggreer/the_silver_searcher/blob/master/doc/ag.1
  ...

Other pages don't call it "the_silver_surfer" either:

  https://launchpad.net/~tomaz-muraus/+archive/the-silver-searcher
  http://blog.kowalczyk.info/software/the-silver-searcher-for-windows.html
  http://jaxbot.me/articles/ag_the_silver_searcher_for_windows_6_8_2013


Now, with regard to the underscore character in package names, there is:
https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Separators

If one constructed a package name for this project, it would become "the-silver-searcher".


> and the most used name on other distros (AFAIK, even on non Linuxes).

*That* may be relevant.

Note that my General Naming Guidelines clarification request in the FPC trac ticket #336 is related, too.

"ag" from python-ase seems to be a GUI app (https://wiki.fysik.dtu.dk/ase/ase/gui/basics.html). The name seems to be short for "ase-gui" (see https://wiki.fysik.dtu.dk/ase-files/ase-gui.desktop).

One problem with renaming the binary there, of course, is that this package would still be incompatible with older versions of python-ase, so I guess strictly speaking we still need a "Conflicts" entry in the spec?

(In reply to Henrik Hodne from comment #13)
> "ag" from python-ase seems to be a GUI app
> (https://wiki.fysik.dtu.dk/ase/ase/gui/basics.html). The name seems to be
> short for "ase-gui" (see
> https://wiki.fysik.dtu.dk/ase-files/ase-gui.desktop).

Very good, this would make the change invisible to most users.

> One problem with renaming the binary there, of course, is that this package
> would still be incompatible with older versions of python-ase, so I guess
> strictly speaking we still need a "Conflicts" entry in the spec?

I believe this kind of breaking change would only happen in a new release. It's probably too late for Fedora 20.

You should probably open bug against the python-ase package and make it block this bug. I see two positive outcomes:
- the change is propagated in the upstream project
- the package is only modified for Fedora

"ag" from python-ase exists since 2008 and is used in documentation and examples. I highly recommend you first contact python-ase upstream.

Shouldn't we add the python-ase package maintainer in the mix ? This is why I'm suggesting to solve this matter in a different bug.

Wouldn't the packager be our best "ambassador" with the upstream ?

Well, I don't hope python-ase changing their name as they've used for many years.

I think this package should get renamed. Generally I install a software, then I can find its binary easily. But this one is an exception, it has a horrible long name "the_silver_searcher" and a weird binary name "ag", to me it's unacceptable.

Re: comment 16

There is no general answer to those questions.

Even in cases where the package maintainer is involved upstream, there may be no sort of "influence" on other developers or interest in renaming a program that exists since 2008. Afterall, it's mentioned in documentation, possibly even in scientific books. It works for them, why would they want to "make room" for a tiny search tool from 2011?

And it's not a bug in the python-ase package. It's simply two completely separate and different projects (who possibly don't even know of eachother) using the same file name. "ag" short for "ASE gui" has been a bad idea to begin with. It saves some typing. It's a GUI with command-line options. Very short and/or generic file names bear a high risk of conflicting with something else. "ag" as the executable name for "The Silver Searcher" is much worse, especially since the name has been chosen mostly to serve as a joke, but its author is free to do so and may not like renaming the exe file, because another project has been first to use it. However, if it had a longer more unique name, users of that tool could define a short shell alias "ag".

Not even the ag- prefix is unused: ;-)

  $ repoquery --whatprovides /usr/bin/ag-tool
  libaccounts-glib-0:1.8-2.fc20.x86_64
  libaccounts-glib-0:1.8-2.fc20.i686
  $ repoquery --whatprovides /usr/bin/ag-backup
  libaccounts-glib-0:1.8-2.fc20.x86_64
  libaccounts-glib-0:1.8-2.fc20.i686

Anyway, resolving file naming conflicts without involvement/cooperation from upstream is difficult. One can always inform both upstreams and hope that both or either one are willing to compromise. As a last resort, you could request an exception from the FPC for a Conflicts tag. Certainly better than creating an implicit conflict, which is not permitted.

(In reply to Michael Schwendt from comment #18)
> Re: comment 16
> 
> There is no general answer to those questions.
> 
> Even in cases where the package maintainer is involved upstream, there may
> be no sort of "influence" on other developers or interest in renaming a
> program that exists since 2008. Afterall, it's mentioned in documentation,
> possibly even in scientific books. It works for them, why would they want to
> "make room" for a tiny search tool from 2011?

That's my point, we should file a bug on the python-ase package for this matter.

> And it's not a bug in the python-ase package.

Agreed, I tend to use the word "issue" on other trackers because it's more generic. Maybe I should have said "file a BZ" which sounds more neutral ?

> "ag" as the executable name for "The Silver Searcher" is
> much worse, especially since the name has been chosen mostly to serve as a
> joke, but its author is free to do so and may not like renaming the exe

I don't see how ag for the_silver_searcher is worse than hg for mercurial. Even git (the stupid content tracker) is a joke. I could also mention subversion. There are plenty of OSS projects named like that.

> file, because another project has been first to use it. However, if it had a
> longer more unique name, users of that tool could define a short shell alias
> "ag".

And most users will probably start python-ase's ag with a launcher, and won't notice the difference.

> Not even the ag- prefix is unused: ;-)
> 
>   $ repoquery --whatprovides /usr/bin/ag-tool
>   libaccounts-glib-0:1.8-2.fc20.x86_64
>   libaccounts-glib-0:1.8-2.fc20.i686
>   $ repoquery --whatprovides /usr/bin/ag-backup
>   libaccounts-glib-0:1.8-2.fc20.x86_64
>   libaccounts-glib-0:1.8-2.fc20.i686

It could be named ag-grep for instance, but wouldn't be the same as other distros. On ubuntu, they both own /usr/bin/ag:

http://packages.ubuntu.com/saucy/amd64/silversearcher-ag/filelist
http://packages.ubuntu.com/saucy/all/python-ase/filelist

> Anyway, resolving file naming conflicts without involvement/cooperation from
> upstream is difficult.

This is why I'm asking the submitter to create a bug on the python-ase package. We can both block this bug and expose the issue to the packager with this very bug. Of course I expect the packager to bring the issue to the upstream if (s)he finds it relevant.

> As a last resort, you could
> request an exception from the FPC for a Conflicts tag. Certainly better than
> creating an implicit conflict, which is not permitted.

Yes the guidelines mention a mutual conflict tag in both packages.

(In reply to Henrik Hodne from comment #13)
> One problem with renaming the binary there, of course, is that this package
> would still be incompatible with older versions of python-ase, so I guess
> strictly speaking we still need a "Conflicts" entry in the spec?

The Guidelines are a bit unclear on this point.  My feeling would be that the Guidelines anticipate /usr/bin/ag being renamed and an update issued in every Fedora release where the package has been previously built.  Then there is no conflict between the latest versions of the two packages.

The python-ase package maintainer suggested in bug 1010479 to rename the binary for this package (the_silver_searcher) to Ag. What's Fedora's policy on names that only differ in case? It would break for case-insensitive file systems, but I don't know how common those are.

python-ase has a branch renaming the ag binary to ase-gui upstream, but it is unknown when this would be merged in.

The /usr/bin/ag file in python-ase is now known as /usr/bin/ase-gui thanks to bug 1010479, and the update is available on rawhide.

We can resume this review.

Can you please update to the latest version ? 0.18.1 is available.

Since I want the_silver_searcher to include in the Fedora package ASAP, I update.
I'm sorry if I am breaking a rule.

> Henriki, 
I have no intention of stealing your job. So if you want to continue, please tell me.

FAS username: knak3 
Spec URL: https://gist.github.com/nak3/8466841/raw/d84da196a9bd577e46e4eac52440d6d57c6e4b34/the_silver_searcher.spec
SRPM URL: https://github.com/nak3/tmp/blob/master/the_silver_searcher-srpm/the_silver_searcher-0.18.1-1.fc19.src.rpm?raw=true
koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=6417761

(Changed) 
* update to 0.18.1 
* delete the line"It searches code about 3–5× faster than ack". (comment #9, #11)

Need any other fix?

No matter who you are, you all need sponsors.

Dridi doesn't have permissions to sponsor people, so the review mark should be lifted and be set by sponsors instead.

But who is the sponsor? You all just want to submit the packages only.

Kenjiro's spec needs fixes also.

You are right.
I don't have the sponsor yet. I have to find someone.

Christopher, you are right, I can review the package but not actually approve it.

Kenjiro, I consistently get the following error when I try to review your submission:

argument is not an RPM package
cpio: premature end of archive
WARNING: Cannot unpack 1008063-the_silver_searcher/srpm/the_silver_searcher-0.18.1-1.fc19.src.rpm into 1008063-the_silver_searcher/srpm-unpacked

Thank you Dridi!

> Kenjiro, I consistently get the following error when I try to review your submission:

What command are you using?

In my environment(FC19), following command 

$ rpm2cpio the_silver_searcher-0.18.1-1.fc19.src.rpm | cpio -id

can work.

And can you please tell me your rpm version?  (eg. rpm --version)

Kenjiro

(In reply to Kenjiro Nakayama from comment #28)
> What command are you using?

I'm simply running `fedora-review -b 1008063`

See https://fedorahosted.org/FedoraReview/ for more information. It's a must-have for packagers :)

(In reply to Dridi Boukelmoune from comment #29)

OK, Since "$fedora-review --rpm-spec --name the_silver_searcher-0.18.1-1.fc19.src.rpm" command works well, it is the BZ's url problem. 

Maybe this will work well.

Spec URL: https://gist.github.com/nak3/8466841/raw/d84da196a9bd577e46e4eac52440d6d57c6e4b34/the_silver_searcher.spec
SRPM URL: https://www.dropbox.com/s/98rbxzx9yvq97ry/the_silver_searcher-0.18.1-1.fc19.src.rpm

(Additional comment for #30)

> Maybe this will work well.

Sorry, does not work well too.

Can you please download the SRPM and fedora-review it by following steps?

step1. $ wget https://www.dropbox.com/s/98rbxzx9yvq97ry/the_silver_searcher-0.18.1-1.fc19.src.rpm
step2. $ fedora-review --rpm-spec --name the_silver_searcher-0.18.1-1.fc19.src.rpm

Sorry to put you to the trouble.

(In reply to Henrik Hodne from comment #3)

> The binary conflict is a little trickier, I'll have to come up with a
> different name. I've noticed that "ack" (a similar program) uses "ack-grep"
> in some locations.

Thanks to the fact this package uses the autotools, this issue is pretty simple to work-around:
%configure --program-prefix=<whatever>-
will install the program and its manpages with a prefix of <whatever>-

e.g.
%configure --program-prefix=the_silver_searcher-
will install
/usr/share/man/man1/the_silver_searcher-ag.1.gz
/usr/bin/the_silver_searcher-ag


Another issue with this package (MUSTFIX): Building is silent. It's impossible to check whether the compiler receives the correct CFLAGS from build.logs.
Please append --disable-silent-rules to %configure

Nakayama-san,

I'm sorry, I was meaning to get back to this today. Feel free to take this.

Here's a quick unofficial review (there are a few [?]s for things I'm not sure how to check or best evaluate).

The biggest issue seems to be that the tar.gz at the Source0 URL does not match the package in the srpm. Looking at the diff output (https://gist.github.com/henrikhodne/189794abe4a63490d143) it looks like the package in the SRPM was generated from master, since it includes changes that were committed upstream after 0.18.1 was released.

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated


Issues:
=======
- Sources used to build the package match the upstream source, as provided in
  the spec URL.
  Note: Upstream MD5sum check error, diff is in /home/henrikhodne/fedora-pkg-
  review/the_silver_searcher/diff.txt
  See: http://fedoraproject.org/wiki/Packaging/SourceURL


===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "BSD (2 clause)", "Unknown or generated". 23 files have unknown license.
     Detailed output of licensecheck in /home/henrikhodne/fedora-pkg-
     review/the_silver_searcher/licensecheck.txt
[?]: Package does not own files or directories owned by other packages.
     Note: Dirs in package are owned also by: /usr/share/bash-
     completion(createrepo, bash-completion, rpmlint, yum, gvfs, glib2),
     /usr/share/bash-completion/completions(createrepo, firewalld, bash-
     completion, rpmlint, yum, gvfs, glib2)
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[x]: Each %files section contains %defattr if rpm < 4.4
     Note: %defattr present but not needed
[?]: Macros in Summary, %description expandable at SRPM build time.
     Note: Macros in: the_silver_searcher (description)
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory names).
[x]: Package is named according to the Package Naming Guidelines.
[?]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[-]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[?]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 30720 bytes in 2 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: 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.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[!]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: 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]: Final provides and requires are sane (see attachments).
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[?]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[?]: Packages should try to preserve timestamps of original installed files.
[?]: Spec use %global instead of %define unless justified.
     Note: %define requiring justification: %define bashcompdir %(pkg-config
     --variable=completionsdir bash-completion), %define bashcompdir
     "/etc/bash_completion.d"
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Uses parallel make %{?_smp_mflags} macro.
[x]: SourceX is a working URL.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Package should not use obsolete m4 macros


Rpmlint
-------
Checking: the_silver_searcher-0.18.1-1.fc19.x86_64.rpm
          the_silver_searcher-0.18.1-1.fc19.src.rpm
the_silver_searcher.x86_64: W: spelling-error Summary(en_US) ack -> ac, ck, sack
the_silver_searcher.x86_64: W: spelling-error %description -l en_US searcing -> searing, searching, seafaring
the_silver_searcher.x86_64: W: spelling-error %description -l en_US ack -> ac, ck, sack
the_silver_searcher.x86_64: W: spelling-error %description -l en_US gitignore -> git ignore, git-ignore, ignore
the_silver_searcher.x86_64: W: spelling-error %description -l en_US hgignore -> ignore
the_silver_searcher.x86_64: W: spelling-error %description -l en_US repo -> rope, rep, reps
the_silver_searcher.x86_64: W: spelling-error %description -l en_US agignore -> ignore
the_silver_searcher.x86_64: W: spelling-error %description -l en_US extern -> ex tern, ex-tern, external
the_silver_searcher.src: W: spelling-error Summary(en_US) ack -> ac, ck, sack
the_silver_searcher.src: W: spelling-error %description -l en_US searcing -> searing, searching, seafaring
the_silver_searcher.src: W: spelling-error %description -l en_US ack -> ac, ck, sack
the_silver_searcher.src: W: spelling-error %description -l en_US gitignore -> git ignore, git-ignore, ignore
the_silver_searcher.src: W: spelling-error %description -l en_US hgignore -> ignore
the_silver_searcher.src: W: spelling-error %description -l en_US repo -> rope, rep, reps
the_silver_searcher.src: W: spelling-error %description -l en_US agignore -> ignore
the_silver_searcher.src: W: spelling-error %description -l en_US extern -> ex tern, ex-tern, external
the_silver_searcher.src: W: file-size-mismatch 0.18.1.tar.gz = 47467, https://github.com/ggreer/the_silver_searcher/archive/0.18.1.tar.gz = 47848
2 packages and 0 specfiles checked; 0 errors, 17 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint the_silver_searcher
the_silver_searcher.x86_64: W: spelling-error Summary(en_US) ack -> ac, ck, sack
the_silver_searcher.x86_64: W: spelling-error %description -l en_US searcing -> searing, searching, seafaring
the_silver_searcher.x86_64: W: spelling-error %description -l en_US ack -> ac, ck, sack
the_silver_searcher.x86_64: W: spelling-error %description -l en_US gitignore -> git ignore, git-ignore, ignore
the_silver_searcher.x86_64: W: spelling-error %description -l en_US hgignore -> ignore
the_silver_searcher.x86_64: W: spelling-error %description -l en_US repo -> rope, rep, reps
the_silver_searcher.x86_64: W: spelling-error %description -l en_US agignore -> ignore
the_silver_searcher.x86_64: W: spelling-error %description -l en_US extern -> ex tern, ex-tern, external
1 packages and 0 specfiles checked; 0 errors, 8 warnings.
# echo 'rpmlint-done:'



Requires
--------
the_silver_searcher (rpmlib, GLIBC filtered):
    libc.so.6()(64bit)
    liblzma.so.5()(64bit)
    liblzma.so.5(XZ_5.0)(64bit)
    libpcre.so.1()(64bit)
    libpthread.so.0()(64bit)
    libz.so.1()(64bit)
    rtld(GNU_HASH)



Provides
--------
the_silver_searcher:
    the_silver_searcher
    the_silver_searcher(x86-64)



Source checksums
----------------
https://github.com/ggreer/the_silver_searcher/archive/0.18.1.tar.gz :
  CHECKSUM(SHA256) this package     : be394b215fbfb09955d4b4d3d773dc46a544106e9f833b6abc847fbec0421282
  CHECKSUM(SHA256) upstream package : 1f5cdacf955d5707cdb60f3f46aab3aae7fe96f105f00ab2d6a5a52d0aad5dc5
diff -r also reports differences


Generated by fedora-review 0.5.1 (bb9bf27) last change: 2013-12-13
Command line :/usr/bin/fedora-review --rpm-spec --name the_silver_searcher-0.18.1-1.fc19.src.rpm
Buildroot used: fedora-19-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, EPEL5, BATCH, DISTTAG

Nakayama-san,

I'm sorry, I was meaning to get back to this today. Feel free to take this.

Here's a quick unofficial review (there are a few [?]s for things I'm not sure how to check or best evaluate).

The biggest issue seems to be that the tar.gz at the Source0 URL does not match the package in the srpm. Looking at the diff output (https://gist.github.com/henrikhodne/189794abe4a63490d143) it looks like the package in the SRPM was generated from master, since it includes changes that were committed upstream after 0.18.1 was released.

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated


Issues:
=======
- Sources used to build the package match the upstream source, as provided in
  the spec URL.
  Note: Upstream MD5sum check error, diff is in /home/henrikhodne/fedora-pkg-
  review/the_silver_searcher/diff.txt
  See: http://fedoraproject.org/wiki/Packaging/SourceURL


===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "BSD (2 clause)", "Unknown or generated". 23 files have unknown license.
     Detailed output of licensecheck in /home/henrikhodne/fedora-pkg-
     review/the_silver_searcher/licensecheck.txt
[?]: Package does not own files or directories owned by other packages.
     Note: Dirs in package are owned also by: /usr/share/bash-
     completion(createrepo, bash-completion, rpmlint, yum, gvfs, glib2),
     /usr/share/bash-completion/completions(createrepo, firewalld, bash-
     completion, rpmlint, yum, gvfs, glib2)
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[x]: Each %files section contains %defattr if rpm < 4.4
     Note: %defattr present but not needed
[?]: Macros in Summary, %description expandable at SRPM build time.
     Note: Macros in: the_silver_searcher (description)
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory names).
[x]: Package is named according to the Package Naming Guidelines.
[?]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[-]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[?]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 30720 bytes in 2 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: 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.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[!]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: 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]: Final provides and requires are sane (see attachments).
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[?]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[?]: Packages should try to preserve timestamps of original installed files.
[?]: Spec use %global instead of %define unless justified.
     Note: %define requiring justification: %define bashcompdir %(pkg-config
     --variable=completionsdir bash-completion), %define bashcompdir
     "/etc/bash_completion.d"
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Uses parallel make %{?_smp_mflags} macro.
[x]: SourceX is a working URL.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Package should not use obsolete m4 macros


Rpmlint
-------
Checking: the_silver_searcher-0.18.1-1.fc19.x86_64.rpm
          the_silver_searcher-0.18.1-1.fc19.src.rpm
the_silver_searcher.x86_64: W: spelling-error Summary(en_US) ack -> ac, ck, sack
the_silver_searcher.x86_64: W: spelling-error %description -l en_US searcing -> searing, searching, seafaring
the_silver_searcher.x86_64: W: spelling-error %description -l en_US ack -> ac, ck, sack
the_silver_searcher.x86_64: W: spelling-error %description -l en_US gitignore -> git ignore, git-ignore, ignore
the_silver_searcher.x86_64: W: spelling-error %description -l en_US hgignore -> ignore
the_silver_searcher.x86_64: W: spelling-error %description -l en_US repo -> rope, rep, reps
the_silver_searcher.x86_64: W: spelling-error %description -l en_US agignore -> ignore
the_silver_searcher.x86_64: W: spelling-error %description -l en_US extern -> ex tern, ex-tern, external
the_silver_searcher.src: W: spelling-error Summary(en_US) ack -> ac, ck, sack
the_silver_searcher.src: W: spelling-error %description -l en_US searcing -> searing, searching, seafaring
the_silver_searcher.src: W: spelling-error %description -l en_US ack -> ac, ck, sack
the_silver_searcher.src: W: spelling-error %description -l en_US gitignore -> git ignore, git-ignore, ignore
the_silver_searcher.src: W: spelling-error %description -l en_US hgignore -> ignore
the_silver_searcher.src: W: spelling-error %description -l en_US repo -> rope, rep, reps
the_silver_searcher.src: W: spelling-error %description -l en_US agignore -> ignore
the_silver_searcher.src: W: spelling-error %description -l en_US extern -> ex tern, ex-tern, external
the_silver_searcher.src: W: file-size-mismatch 0.18.1.tar.gz = 47467, https://github.com/ggreer/the_silver_searcher/archive/0.18.1.tar.gz = 47848
2 packages and 0 specfiles checked; 0 errors, 17 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint the_silver_searcher
the_silver_searcher.x86_64: W: spelling-error Summary(en_US) ack -> ac, ck, sack
the_silver_searcher.x86_64: W: spelling-error %description -l en_US searcing -> searing, searching, seafaring
the_silver_searcher.x86_64: W: spelling-error %description -l en_US ack -> ac, ck, sack
the_silver_searcher.x86_64: W: spelling-error %description -l en_US gitignore -> git ignore, git-ignore, ignore
the_silver_searcher.x86_64: W: spelling-error %description -l en_US hgignore -> ignore
the_silver_searcher.x86_64: W: spelling-error %description -l en_US repo -> rope, rep, reps
the_silver_searcher.x86_64: W: spelling-error %description -l en_US agignore -> ignore
the_silver_searcher.x86_64: W: spelling-error %description -l en_US extern -> ex tern, ex-tern, external
1 packages and 0 specfiles checked; 0 errors, 8 warnings.
# echo 'rpmlint-done:'



Requires
--------
the_silver_searcher (rpmlib, GLIBC filtered):
    libc.so.6()(64bit)
    liblzma.so.5()(64bit)
    liblzma.so.5(XZ_5.0)(64bit)
    libpcre.so.1()(64bit)
    libpthread.so.0()(64bit)
    libz.so.1()(64bit)
    rtld(GNU_HASH)



Provides
--------
the_silver_searcher:
    the_silver_searcher
    the_silver_searcher(x86-64)



Source checksums
----------------
https://github.com/ggreer/the_silver_searcher/archive/0.18.1.tar.gz :
  CHECKSUM(SHA256) this package     : be394b215fbfb09955d4b4d3d773dc46a544106e9f833b6abc847fbec0421282
  CHECKSUM(SHA256) upstream package : 1f5cdacf955d5707cdb60f3f46aab3aae7fe96f105f00ab2d6a5a52d0aad5dc5
diff -r also reports differences


Generated by fedora-review 0.5.1 (bb9bf27) last change: 2013-12-13
Command line :/usr/bin/fedora-review --rpm-spec --name the_silver_searcher-0.18.1-1.fc19.src.rpm
Buildroot used: fedora-19-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, EPEL5, BATCH, DISTTAG

(In reply to Ralf Corsepius from comment #32)

> Another issue with this package (MUSTFIX): Building is silent. It's impossible to check whether the compiler receives the correct CFLAGS from build.logs.
> Please append --disable-silent-rules to %configure

Thank you, Ralf. I updated.

(In reply to Henrik Hodne from comment #33 and #34)

Thank you Henrik,

> The biggest issue seems to be that the tar.gz at the Source0 URL does not match the package in the srpm. Looking at the diff output (https://gist.github.com/henrikhodne/189794abe4a63490d143) it looks like the package in the SRPM was generated from master, since it includes changes that were committed upstream after 0.18.1 was released.

Yes, you're right, it's my mistake. I updated it to make snapshot package. The name of the SRPM package is changed too.

Spec URL: https://gist.github.com/nak3/8466841
SRPM URL: https://www.dropbox.com/s/hbyg7qwb2stj6g0/the_silver_searcher-0.18.1-1.20140118git.fc19.src.rpm

Kenjiro

I updated spec file and upload them.

Updated spec:  http://diy-kenjiro.rhcloud.com/rpms/the_silver_searcher.spec
Updated SRPM:  http://diy-kenjiro.rhcloud.com/rpms/the_silver_searcher-0.18.1-1.20140118git.fc19.src.rpm
Scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=6453244

* Changed following points.
---
> Summary:        Super-fast text searching tool

Changed clearly.

> Group:          Applications/Text

Added Group.

> %description
> The Silver Searcher is a code searcing tool similar to ack, with a focus on speed.

Shortened by one sentence. I think this is enough.

> BuildRequires:  bash-completion

Simply added bash-completion to BuildRequires, since we don't need to consider rhel packaging.

> %clean

Removed.

... And add some tiny changes.
---

*** This bug has been marked as a duplicate of bug 1057991 ***