The spec file location is 404.

The Spec URL is now not showing a 404, just to be sure I have added them else where 


Spec URL : https://dhanvi.fedorapeople.org/packages/asciidoctor-mallard/asciidoctor-mallard.spec 

SRPM URL : https://dhanvi.fedorapeople.org/packages/asciidoctor-mallard/rubygem-asciidoctor-mallard-0.1.0.dev-1.fc24.src.rpm 

Description: An extension for Asciidoctor that converts AsciiDoc documents to Mallard 1.0

Fedora Account System Username: dhanvi

Hi Tummala,

This looks to be output of gem2rpm without any modification. Unfortunately, this is not acceptable for official review and I can spot several issues on the first look:

1. The .spec file naming.
2. Test suite is not executed.
3. License is not marked by %license macro.
4. License file is not included in the main package.

I'd appreciate if you can try to do package self review prior sending package for official review [1].

Actually, it does not look you are sponsored into package group yet, so I suggest you to follow these instructions [2], where you can find plenty of useful links.



[1] http://fedoraproject.org/wiki/Packaging:ReviewGuidelines
[2] https://fedoraproject.org/wiki/Join_the_package_collection_maintainers

Hi dhanvi, there is a lot of time since last update, do you want to continue with this review? If not I can take care of it.

Please remenber than acording to the Stalle Package Review process(0) if you do not reponse in one week I can close this review a open a new one.

Regards

0: https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews

(In reply to William Moreno from comment #4)
> Hi dhanvi, there is a lot of time since last update, do you want to continue
> with this review? If not I can take care of it.

Hello William, I would like to continue with this review, but I might need your help for the same.

> 
> Please remenber than acording to the Stalle Package Review process(0) if you
> do not reponse in one week I can close this review a open a new one.

I will update the spec file and continue with the feedback process before the end of this week 

> 
> Regards
> 
> 0: https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews

I have updated the spec and built it 

spec: https://dhanvi.fedorapeople.org/packages/asciidoctor-mallard/asciidoctor-mallard.spec

srpm: https://dhanvi.fedorapeople.org/packages/asciidoctor-mallard/asciidoctor-mallard.spec

(In reply to Tummala Dhanvi (c0mrad3) from comment #6)
Thx, but I still can't see any test suite executed. Is there some reason for that? Please note that in case of Ruby packages, the test suite is the only thing which can assure that the package works. There is no compiler which would at least partly cover your back ...

I have added the testing part by adding this line 

ruby -Ilib -e 'Dir.glob "./test/*.rb"' 

Hope it should do it. 


Updated packages here https://dhanvi.fedorapeople.org/packages/asciidoctor-mallard/

* Source of the gem?
  - Where did you get the source gem? It is not available on rubygems.org as far
    as I can tell, neither you documented anywhere how you obtained it.

* Package naming
  - Please rename the .spec file to rubygem-asciidoctor-mallar.spec to be in
    line with the guidelines [1].

* Package version
  - Please use proper version scheme. The package version should be probably
    just "0.1.0" while the release should be "0.1.dev".
  - The versioning is documented in detail here [2].

* Duplicated license
  - You should not include the "%license LICENSE.adoc". Please keep just "%doc
    %{gem_instdir}/LICENSE.adoc" in the main package (and change the %doc to
    %license of course).

* Unneeded requires
  - You probably don't need to include "Requires: rubygem-thread_safe". Runtime
    requires should be autogenerated during build process.
  - Not sure what is the "pintail" good for. Some explaining hint above would
    come handy.

* Doc subpackage
  - It is good habit to keep the documentation in -doc subpackage.

* Test suite
  - Well, the line you used is just part of the story. You can see that there
    is nothing which would indicated, that the test suite was executed. You
    should use following line:

```
ruby -Ilib -e 'Dir.glob "./test/*_test.rb", &method(:require)'
```

  - Unfortunately, there seems to be dependency on asciidoctor-doctest, which
    is not in Fedora yet. Since it has quite lot of dependencies, it is
    probably not worth of packaging ATM, but all this should be documented.
  - Instead of execution of test suite, you should consider to provide at least
    some sanity test, e.g. try to convert some document from AsciiDoc to
    Mallard using the %{gem_instdir}/bin/asciidoctor-mallard

* Bump release between review iteration
  - It is good habit to bump the package release each review iteration.



[1] https://fedoraproject.org/wiki/Packaging:Ruby#Naming_Guidelines
[2] https://fedoraproject.org/wiki/Packaging:Versioning#Pre-Release_packages

(In reply to Vít Ondruch from comment #9)
> * Source of the gem?
>   - Where did you get the source gem? It is not available on rubygems.org as
> far
>     as I can tell, neither you documented anywhere how you obtained it.
The spec file contained the url of the github repo https://github.com/asciidoctor/asciidoctor-mallard

I have built the gem my self using `gem build *.gemspec`
> 
> * Package naming
>   - Please rename the .spec file to rubygem-asciidoctor-mallar.spec to be in
>     line with the guidelines [1].
Updated
> 
> * Package version
>   - Please use proper version scheme. The package version should be probably
>     just "0.1.0" while the release should be "0.1.dev".
>   - The versioning is documented in detail here [2].
I upstream had it versioning like that https://github.com/asciidoctor/asciidoctor-mallard/blob/master/lib/asciidoctor-mallard/version.rb

but I have updated it to 0.1.dev
> 
> * Duplicated license
>   - You should not include the "%license LICENSE.adoc". Please keep just
> "%doc
>     %{gem_instdir}/LICENSE.adoc" in the main package (and change the %doc to
>     %license of course).

Done

> * Unneeded requires
>   - You probably don't need to include "Requires: rubygem-thread_safe".
> Runtime
>     requires should be autogenerated during build process.

Done

>   - Not sure what is the "pintail" good for. Some explaining hint above would
>     come handy.
pintail is actually not required

Pintail is a tool to convert mallard to html (https://github.com/projectmallard/pintail)

This package converts asciidoc to mallard  
> 
> * Doc subpackage
>   - It is good habit to keep the documentation in -doc subpackage.
Done
> 
> * Test suite
>   - Well, the line you used is just part of the story. You can see that there
>     is nothing which would indicated, that the test suite was executed. You
>     should use following line:
> 
> ```
> ruby -Ilib -e 'Dir.glob "./test/*_test.rb", &method(:require)'
> ```
> 
>   - Unfortunately, there seems to be dependency on asciidoctor-doctest, which
>     is not in Fedora yet. Since it has quite lot of dependencies, it is
>     probably not worth of packaging ATM, but all this should be documented.
>   - Instead of execution of test suite, you should consider to provide at
> least
>     some sanity test, e.g. try to convert some document from AsciiDoc to
>     Mallard using the %{gem_instdir}/bin/asciidoctor-mallard
> 
Can you please explain me a bit more about how to get it right ? 

URL: https://dhanvi.fedorapeople.org/packages/asciidoctor-mallard/

(In reply to Tummala Dhanvi (c0mrad3) from comment #10)
> (In reply to Vít Ondruch from comment #9)
> > * Source of the gem?
> >   - Where did you get the source gem? It is not available on rubygems.org as
> > far
> >     as I can tell, neither you documented anywhere how you obtained it.
> The spec file contained the url of the github repo
> https://github.com/asciidoctor/asciidoctor-mallard
> 
> I have built the gem my self using `gem build *.gemspec`

There are two things here:

1. If you do something special, mainly if the SourceX tag is not link, it is always good idea to describe in comment what you actually did. Some people even prefer to provide script preparing the packages (isn't this documented somewhere in guidelines? Not sure)

2. More importantly, I don't think this is required and that this is right approach here. I would suggest to get the tarball from the GH [1], expand it using %setup macro and that should be everything you have in %prep section. The rest of the package would be the same.

As soon as there is stable version of the gem available on rubygems.org, then you would change the package to use the .gem as a source.



> > * Package version
> >   - Please use proper version scheme. The package version should be probably
> >     just "0.1.0" while the release should be "0.1.dev".
> >   - The versioning is documented in detail here [2].
> I upstream had it versioning like that
> https://github.com/asciidoctor/asciidoctor-mallard/blob/master/lib/
> asciidoctor-mallard/version.rb
> 
> but I have updated it to 0.1.dev

Please read the [2] carefully. You have to ensure update path. E.g. the original VR you used was "0.1.0.dev-1". If the upstream released stable version "0.1.0", your VR would become "0.1.0.dev-1" and this is what RPM thinks about the versions:

```
$ rpmdev-vercmp 0.1.0.dev-1 0.1.0-1
0.1.0.dev-1 > 0.1.0-1
```

That means the newer version would never get installed. Hence I suggested to use just "0.1.0" while the release should be "0.1.dev". In the .spec file, it would look like:

```
Version: 0.1.0
Release: 0.1.dev%{?dist}
```

Which makes the upgrade path correct:

```
$ rpmdev-vercmp 0.1.0-0.1.dev 0.1.0-1
0.1.0-0.1.dev < 0.1.0-1
```

Again, please read the [2] carefully (although I admit it is not easy read, but hopefully this will get simplified in the future).

> > * Test suite
> >   - Well, the line you used is just part of the story. You can see that there
> >     is nothing which would indicated, that the test suite was executed. You
> >     should use following line:
> > 
> > ```
> > ruby -Ilib -e 'Dir.glob "./test/*_test.rb", &method(:require)'
> > ```
> > 
> >   - Unfortunately, there seems to be dependency on asciidoctor-doctest, which
> >     is not in Fedora yet. Since it has quite lot of dependencies, it is
> >     probably not worth of packaging ATM, but all this should be documented.
> >   - Instead of execution of test suite, you should consider to provide at
> > least
> >     some sanity test, e.g. try to convert some document from AsciiDoc to
> >     Mallard using the %{gem_instdir}/bin/asciidoctor-mallard
> > 
> Can you please explain me a bit more about how to get it right ? 
> 
> URL: https://dhanvi.fedorapeople.org/packages/asciidoctor-mallard/

My idea was to do something as simple as shipping some simple AsciiDoc file as Source1 and trying the conversion, e.g. the %check section could look like:

```
%check
pushd .%{gem_instdir}
# There is no rubygem-asciidoctor-doctest in Fedora yet, which is needed
# for test suite.
# ruby -Ilib -e 'Dir.glob "./test/*_test.rb", &method(:require)'

bin/asciidoctor-mallard %{SOURCE1} -o test.output
popd

```

This is the simplest test you can do, since if the conversion fails for whatever reason, the build should break.

On top of that, you can check for the consistency of the output by calculating hash of the output, e.g. you can add the following line there:

```
sha256sum test.output | grep "870474333c6f4e41238f923f1d27c7688c86b6463db39725e076fc5f05fe2520" 
```

Of course you have to use correct hash, based on the content of the %{SOURCE1} file you are going to use. Or you can choose to test any other bit of the file content, if you consider it important.


BTW just running "bin/asciidoctor-mallard" will fail the build ATM, since there is apparently some bug (undefined method `size' for nil:NilClass).


[1] https://fedoraproject.org/wiki/Packaging:SourceURL#Commit_Revision
[2] https://fedoraproject.org/wiki/Packaging:Versioning#Pre-Release_packages

> 
> 
> > > * Package version
> > >   - Please use proper version scheme. The package version should be probably
> > >     just "0.1.0" while the release should be "0.1.dev".
> > >   - The versioning is documented in detail here [2].
> > I upstream had it versioning like that
> > https://github.com/asciidoctor/asciidoctor-mallard/blob/master/lib/
> > asciidoctor-mallard/version.rb
> > 
> > but I have updated it to 0.1.dev
> 
> Please read the [2] carefully. You have to ensure update path. E.g. the
> original VR you used was "0.1.0.dev-1". If the upstream released stable
> version "0.1.0", your VR would become "0.1.0.dev-1" and this is what RPM
> thinks about the versions:
> 
> ```
> $ rpmdev-vercmp 0.1.0.dev-1 0.1.0-1
> 0.1.0.dev-1 > 0.1.0-1
> ```
> 
> That means the newer version would never get installed. Hence I suggested to
> use just "0.1.0" while the release should be "0.1.dev". In the .spec file,
> it would look like:
> 
> ```
> Version: 0.1.0
> Release: 0.1.dev%{?dist}
> ```
> 
> Which makes the upgrade path correct:
> 
> ```
> $ rpmdev-vercmp 0.1.0-0.1.dev 0.1.0-1
> 0.1.0-0.1.dev < 0.1.0-1
> ```
> 
> Again, please read the [2] carefully (although I admit it is not easy read,
> but hopefully this will get simplified in the future).
Got it right this time!
> 
> > > * Test suite
> > >   - Well, the line you used is just part of the story. You can see that there
> > >     is nothing which would indicated, that the test suite was executed. You
> > >     should use following line:
> > > 
> > > ```
> > > ruby -Ilib -e 'Dir.glob "./test/*_test.rb", &method(:require)'
> > > ```
> > > 
> > >   - Unfortunately, there seems to be dependency on asciidoctor-doctest, which
> > >     is not in Fedora yet. Since it has quite lot of dependencies, it is
> > >     probably not worth of packaging ATM, but all this should be documented.
> > >   - Instead of execution of test suite, you should consider to provide at
> > > least
> > >     some sanity test, e.g. try to convert some document from AsciiDoc to
> > >     Mallard using the %{gem_instdir}/bin/asciidoctor-mallard
> > > 
> > Can you please explain me a bit more about how to get it right ? 
> > 
> > URL: https://dhanvi.fedorapeople.org/packages/asciidoctor-mallard/
> 
> My idea was to do something as simple as shipping some simple AsciiDoc file
> as Source1 and trying the conversion, e.g. the %check section could look
> like:
> 
> ```
> %check
> pushd .%{gem_instdir}
> # There is no rubygem-asciidoctor-doctest in Fedora yet, which is needed
> # for test suite.
> # ruby -Ilib -e 'Dir.glob "./test/*_test.rb", &method(:require)'
> 
> bin/asciidoctor-mallard %{SOURCE1} -o test.output
> popd
> 
> ```
> 
> This is the simplest test you can do, since if the conversion fails for
> whatever reason, the build should break.
> 
> On top of that, you can check for the consistency of the output by
> calculating hash of the output, e.g. you can add the following line there:
> 
> ```
> sha256sum test.output | grep
> "870474333c6f4e41238f923f1d27c7688c86b6463db39725e076fc5f05fe2520" 
> ```
done it using the example given in the README
> 
> Of course you have to use correct hash, based on the content of the
> %{SOURCE1} file you are going to use. Or you can choose to test any other
> bit of the file content, if you consider it important.
> 
> 
> BTW just running "bin/asciidoctor-mallard" will fail the build ATM, since
> there is apparently some bug (undefined method `size' for nil:NilClass).
> 
This might be due to the requirements of rubygem-asciidoctor which I missed, also 

`
ruby ./bin/asciidoctor-mallard
`

works.

URL: https://dhanvi.fedorapeople.org/packages/asciidoctor-mallard/

(In reply to Tummala Dhanvi (c0mrad3) from comment #12)
> > 
> > 
> > > > * Package version
> > > >   - Please use proper version scheme. The package version should be probably
> > > >     just "0.1.0" while the release should be "0.1.dev".
> > > >   - The versioning is documented in detail here [2].
> > > I upstream had it versioning like that
> > > https://github.com/asciidoctor/asciidoctor-mallard/blob/master/lib/
> > > asciidoctor-mallard/version.rb
> > > 
> > > but I have updated it to 0.1.dev
> > 
> > Please read the [2] carefully. You have to ensure update path. E.g. the
> > original VR you used was "0.1.0.dev-1". If the upstream released stable
> > version "0.1.0", your VR would become "0.1.0.dev-1" and this is what RPM
> > thinks about the versions:
> > 
> > ```
> > $ rpmdev-vercmp 0.1.0.dev-1 0.1.0-1
> > 0.1.0.dev-1 > 0.1.0-1
> > ```
> > 
> > That means the newer version would never get installed. Hence I suggested to
> > use just "0.1.0" while the release should be "0.1.dev". In the .spec file,
> > it would look like:
> > 
> > ```
> > Version: 0.1.0
> > Release: 0.1.dev%{?dist}
> > ```
> > 
> > Which makes the upgrade path correct:
> > 
> > ```
> > $ rpmdev-vercmp 0.1.0-0.1.dev 0.1.0-1
> > 0.1.0-0.1.dev < 0.1.0-1
> > ```
> > 
> > Again, please read the [2] carefully (although I admit it is not easy read,
> > but hopefully this will get simplified in the future).
> Got it right this time!


Yep, LGTM!

But you still should work on the Source package I mentioned in previous step. Please study this [1] to get yourself familiar with git snapshots. This is something I come up with:

```
--- rubygem-asciidoctor-mallard.spec.orig	2016-12-05 14:57:16.684010918 +0100
+++ rubygem-asciidoctor-mallard.spec	2016-12-06 10:23:44.589045687 +0100
@@ -1,14 +1,26 @@
 %global gem_name asciidoctor-mallard
 
+%global commit0 cf5a2a8baf8f54f64af4d0f3eac763e1ca47c917
+%global shortcommit0 %(c=%{commit0}; echo ${c:0:7})
+
+%global	prever .dev
+%global	prerpmver %(echo "%{?prever}" | sed -e 's|\\.||g')
+
+# Modify the standard if we are packaging pre-release version.
+%global gem_instdir %{gem_dir}/gems/%{gem_name}-%{version}%{?prever}
+%global gem_cache %{gem_dir}/cache/%{gem_name}-%{version}%{?prever}.gem
+%global gem_spec %{gem_dir}/specifications/%{gem_name}-%{version}%{?prever}.gemspec
+%global gem_docdir %{gem_dir}/doc/%{gem_name}-%{version}%{?prever}
+
 Name: rubygem-%{gem_name}
 Version: 0.1.0
-Release: 0.1.dev%{?dist}
+Release: %{?prever:0.}1%{?prever:.%{prerpmver}}%{?dist}
 Summary: Converts AsciiDoc documents to Project Mallard format
 
 Group: Development/Languages
 License: MIT
 URL: https://github.com/asciidoctor/asciidoctor-mallard
-Source0: %{gem_name}-%{version}.gem 
+Source0: https://github.com/asciidoctor/asciidoctor-mallard/archive/%{commit0}.tar.gz#/%{name}-%{shortcommit0}.tar.gz
 
 BuildArch: noarch
 BuildRequires: ruby
@@ -30,17 +42,20 @@
 Documentation for %{name}
 
 %prep
-gem unpack %{SOURCE0}
-
-%setup -q -D -T -n  %{gem_name}-%{version}
+%setup -n %{gem_name}-%{commit0}
 
-gem spec %{SOURCE0} -l --ruby > %{gem_name}.gemspec
+# We don't have git repository, so just collect everything available,
+# except the .gemspec (which is modified by this command and the gem itself.
+sed -i '/s.files/a    \
+  s.files = Dir["**/*"] - ["asciidoctor-mallard-#{Asciidoctor::Mallard::VERSION}.gem", "asciidoctor-mallard.gemspec"]' \
+  asciidoctor-mallard.gemspec
 
 %build
 # Create the gem as gem install only works on a gem file
 gem build %{gem_name}.gemspec
 
-%gem_install
+# NOTE: The '-n ...' can be dropped when stable version is available.
+%gem_install -n %{gem_name}-%{version}%{?prever}.gem
 
 %install
 mkdir -p %{buildroot}%{gem_dir}
@@ -93,8 +108,10 @@
 
 %files doc
 %{gem_docdir}
+%{gem_instdir}/Gemfile
 %{gem_instdir}/README.adoc
 %{gem_instdir}/Rakefile
+%{gem_instdir}/WORKLOG.adoc
 %{gem_instdir}/test
 
 %changelog
```


> > 
> > > > * Test suite
> > > >   - Well, the line you used is just part of the story. You can see that there
> > > >     is nothing which would indicated, that the test suite was executed. You
> > > >     should use following line:
> > > > 
> > > > ```
> > > > ruby -Ilib -e 'Dir.glob "./test/*_test.rb", &method(:require)'
> > > > ```
> > > > 
> > > >   - Unfortunately, there seems to be dependency on asciidoctor-doctest, which
> > > >     is not in Fedora yet. Since it has quite lot of dependencies, it is
> > > >     probably not worth of packaging ATM, but all this should be documented.
> > > >   - Instead of execution of test suite, you should consider to provide at
> > > > least
> > > >     some sanity test, e.g. try to convert some document from AsciiDoc to
> > > >     Mallard using the %{gem_instdir}/bin/asciidoctor-mallard
> > > > 
> > > Can you please explain me a bit more about how to get it right ? 
> > > 
> > > URL: https://dhanvi.fedorapeople.org/packages/asciidoctor-mallard/
> > 
> > My idea was to do something as simple as shipping some simple AsciiDoc file
> > as Source1 and trying the conversion, e.g. the %check section could look
> > like:
> > 
> > ```
> > %check
> > pushd .%{gem_instdir}
> > # There is no rubygem-asciidoctor-doctest in Fedora yet, which is needed
> > # for test suite.
> > # ruby -Ilib -e 'Dir.glob "./test/*_test.rb", &method(:require)'
> > 
> > bin/asciidoctor-mallard %{SOURCE1} -o test.output
> > popd
> > 
> > ```
> > 
> > This is the simplest test you can do, since if the conversion fails for
> > whatever reason, the build should break.
> > 
> > On top of that, you can check for the consistency of the output by
> > calculating hash of the output, e.g. you can add the following line there:
> > 
> > ```
> > sha256sum test.output | grep
> > "870474333c6f4e41238f923f1d27c7688c86b6463db39725e076fc5f05fe2520" 
> > ```
> done it using the example given in the README
> > 
> > Of course you have to use correct hash, based on the content of the
> > %{SOURCE1} file you are going to use. Or you can choose to test any other
> > bit of the file content, if you consider it important.
> > 
> > 
> > BTW just running "bin/asciidoctor-mallard" will fail the build ATM, since
> > there is apparently some bug (undefined method `size' for nil:NilClass).
> > 
> This might be due to the requirements of rubygem-asciidoctor which I missed,
> also 
> 
> `
> ruby ./bin/asciidoctor-mallard
> `
> 
> works.

Wonderful! Thx.

* Please drop the "Requires: rubygem-asciidoctor". This require is autogenerated, you don't need to specify this dependency explicitly.


[1] https://fedoraproject.org/wiki/Packaging:SourceURL#Git_Hosting_Services

This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time. We're sorry
it is taking so long. If you're still interested in packaging this software
into Fedora repositories, please respond to this comment clearing the
NEEDINFO flag.

You may want to update the specfile and the src.rpm to the latest version
available and to propose a review swap on Fedora devel mailing list to increase
chances to have your package reviewed. If this is your first package and you
need a sponsor, you may want to post some informal reviews. Read more at
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group.

Without any reply, this request will shortly be considered abandoned
and will be closed.
Thank you for your patience.

This is an automatic action taken by review-stats script.

The ticket submitter failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we consider this ticket as DEADREVIEW and proceed to close it.