Since executables are being shipped in both python 2 and 3 subpackages, each package needs to have executables with -X and -X.Y version suffixes [0], with python2 providing an unversioned symlink executable.

The executables exists in both subpackages, but only the python2 package has the man page.

The LICENSE.MIT file needs to be included with %license.

0: https://fedoraproject.org/wiki/Packaging:Python#Naming

FYI.

I'm not sure how to fix the issues in a correct manner but made some changes in its upstream:

> Since executables are being shipped in both python 2 and 3 subpackages, each
> package needs to have executables with -X and -X.Y version suffixes [0], with
> python2 providing an unversioned symlink executable.

https://github.com/ssato/python-anyconfig/commit/98d28d056299abf2f83feffbc14ec4c157757214#diff-49f0020084413dbd6fb3815a74500f70

> The LICENSE.MIT file needs to be included with %license.

https://github.com/ssato/python-anyconfig/commit/be544779e697674520def3907cf090a42c0f715f#diff-49f0020084413dbd6fb3815a74500f70

License file change looks good.

The version suffixes are for the corresponding python version.

%{python2_version} and %{python3_version} already have the minor version, for instance 2.7, and 3.6.  So the move would be:

mv %{buildroot}%{_bindir}/anyconfig_cli %{buildroot}%{_bindir}/anyconfig_cli-%{python3_version}

Then symlink from the major version only variant:

ln -s anyconfig_cli-%{python3_version} %{buildroot}%{_bindir}/anyconfig_cli-3

And for the python2 subpackage, don't copy the executable for the unversioned executable, just add a second symlink:

ln -s anyconfig_cli-%{python3_version} %{buildroot}%{_bindir}/anyconfig_cli

And then mirror this for the man pages.  

You can take a look at the python2 and python3 packages to get the idea (though it doesn't use a '-', but for this package you should).

Also the Group: tag is not used in Fedora.

(In reply to Troy Curtis from comment #4)
> Also the Group: tag is not used in Fedora.

Thanks for you comment. I removed it in the upstream:
https://github.com/ssato/python-anyconfig/commit/d45903b65ebe8e2a653094f3956dd9d32079c441#diff-49f0020084413dbd6fb3815a74500f70

Troy-san, thanks a lot for your comments!

(In reply to Troy Curtis from comment #3)
> The version suffixes are for the corresponding python version.
> 
> %{python2_version} and %{python3_version} already have the minor version,
> for instance 2.7, and 3.6.  So the move would be:
> 
> mv %{buildroot}%{_bindir}/anyconfig_cli
> %{buildroot}%{_bindir}/anyconfig_cli-%{python3_version}

I guess that I made so already by the previous commits I mentioned:

ssato@localhost% rpm -ql python2-anyconfig | grep anyconfig_cli | LC_ALL=en_US.UTF-8 xargs ls -l
lrwxrwxrwx. 1 root root   17 Feb  1 23:04 /usr/bin/anyconfig_cli -> anyconfig_cli-2.7
lrwxrwxrwx. 1 root root   17 Feb  1 23:04 /usr/bin/anyconfig_cli-2 -> anyconfig_cli-2.7
-rwxr-xr-x. 1 root root  400 Feb  1 23:04 /usr/bin/anyconfig_cli-2.7
lrwxrwxrwx. 1 root root   22 Feb  1 23:04 /usr/share/man/man1/anyconfig_cli.1.gz -> anyconfig_cli-2.7.1.gz
lrwxrwxrwx. 1 root root   22 Feb  1 23:04 /usr/share/man/man1/anyconfig_cli-2.1.gz -> anyconfig_cli-2.7.1.gz
-rw-r--r--. 1 root root 1724 May  3  2017 /usr/share/man/man1/anyconfig_cli-2.7.1.gz
ssato@localhost% rpm -ql python3-anyconfig | grep anyconfig_cli | LC_ALL=en_US.UTF-8 xargs ls -l
lrwxrwxrwx. 1 root root   17 Feb  1 23:04 /usr/bin/anyconfig_cli-3 -> anyconfig_cli-3.6
-rwxr-xr-x. 1 root root  400 Feb  1 23:04 /usr/bin/anyconfig_cli-3.6
lrwxrwxrwx. 1 root root   22 Feb  1 23:04 /usr/share/man/man1/anyconfig_cli-3.1.gz -> anyconfig_cli-3.6.1.gz
-rw-r--r--. 1 root root 1724 May  3  2017 /usr/share/man/man1/anyconfig_cli-3.6.1.gz
ssato@localhost%

> And for the python2 subpackage, don't copy the executable for the
> unversioned executable, just add a second symlink:
> 
> ln -s anyconfig_cli-%{python3_version} %{buildroot}%{_bindir}/anyconfig_cli

I've fixed it:
https://github.com/ssato/python-anyconfig/commit/51a7e4fa1376e9daa1b90ea0be861bbf6f4b325d#diff-49f0020084413dbd6fb3815a74500f70

Updated spec: https://wakko666.fedorapeople.org/python-anyconfig/python-anyconfig.spec

Updated srpm: https://wakko666.fedorapeople.org/python-anyconfig/python-anyconfig-0.9.3-1.fc28.src.rpm

The source url, which expands to https://github.com/ssato/python-anyconfig/archive/0.9.3/anyconfig-0.9.3.tar.gz gives a "File not Found" error.

BuildRequires actually belong to the source rpm, not individual subpackages, so I'd suggest moving them up a level.  This will also make clear the 'make' is a duplicate dependency.

For some reason my `fedora-review` invocation is having some trouble, so these two items will have to suffice for the moment.  I didn't notice other issues though, and as I'm not (yet) a packager, I can't take it all the way to approved anyhow.

I've fixed the url and the buildrequires. File URLs haven't changed since comment #7

I'll take the review.

> %global sum Python library to load and dump configuration files in various formats
Using normal Summary: and then %summary subsequently saves one line ;)

> %global debug_package %{nil}
That looks suspicious. Why do you need this?
Package builds fine without it.

> %{__rm}
Eh, using a macro here is entirely pointless. It just makes the commands harder to read (and longer). The guidelines say that macros should be used for some *directories* [https://fedoraproject.org/wiki/Packaging:Guidelines#Macros], but even that makes little sense nowadays.

> https://github.com/ssato/%{name}
I know people love macros, but this makes it impossible to just click on this and open it in a browser… It's a matter of preference, but I don't see the advantage of using a macro here.

> Source0:        %{url}/archive/RELEASE-%{version}.tar.gz
This should be ...RELEASE_{%version}...

> %defattr(-,root,root,-)
Not necessary in Fedora and somewhat recent RHEL.

- Large documentation must go in a -doc subpackage. Large could be size
  (~1MB) or number of files.
  Note: Documentation size is 2662400 bytes in 126 files.
  See:
  http://fedoraproject.org/wiki/Packaging/Guidelines#PackageDocumentation

It'd be nice to split out a python-anyconfig-doc subpackage with the docs.


During build I see the following error:
>     import cbor
> ImportError: No module named cbor
Is some dependency missing?

and later:
> toml.py:docstring of anyconfig.backend.toml.Parser._load_from_stream_fn:6: WARNING: Definition list ends without a blank line; unexpected unindent.
> /builddir/build/BUILD/python-anyconfig-RELEASE_0.9.3/anyconfig/backend/xml.py:docstring of anyconfig.backend.xml._tweak_ns:4: WARNING: Field list ends without a blank line; unexpected unindent.
> /builddir/build/BUILD/python-anyconfig-RELEASE_0.9.3/docs/api/anyconfig.cli.rst:4: WARNING: autodoc: failed to import module u'anyconfig.cli'; the following exception was raised:
> Traceback (most recent call last):
>   File "/usr/lib/python2.7/site-packages/sphinx/ext/autodoc.py", line 658, in import_object
>     __import__(self.modname)
>   File "/builddir/build/BUILD/python-anyconfig-RELEASE_0.9.3/anyconfig/cli.py", line 42, in <module>
>     sys.stdout = codecs.getwriter(_ENCODING)(sys.stdout)
>   File "/usr/lib64/python2.7/codecs.py", line 1009, in getwriter
>     return lookup(encoding).streamwriter
> TypeError: lookup() argument 1 must be string, not None
> done

And now the hard part: what is the difference in behaviour or output between anyconfig-2 and anyconfig-3?

And also (a question for upstream): why is the executable called "anyconfig_cli" and not just "anyconfig"?

(In reply to Zbigniew Jędrzejewski-Szmek from comment #10)
> I'll take the review.
> 

Great! Thanks!

> > %global sum Python library to load and dump configuration files in various formats
> Using normal Summary: and then %summary subsequently saves one line ;)
> 

Not true. The summary is used in 3 places because of the python2 & python3 sub-packaged. The macro saves copy/pasting the same text in 3 places.  :)


> > %global debug_package %{nil}
> That looks suspicious. Why do you need this?
> Package builds fine without it.

Removed.

> 
> > %{__rm}
> Eh, using a macro here is entirely pointless. It just makes the commands
> harder to read (and longer). The guidelines say that macros should be used
> for some *directories*
> [https://fedoraproject.org/wiki/Packaging:Guidelines#Macros], but even that
> makes little sense nowadays.
> 

Fixed.

> > https://github.com/ssato/%{name}
> I know people love macros, but this makes it impossible to just click on
> this and open it in a browser… It's a matter of preference, but I don't see
> the advantage of using a macro here.
> 

Fixed.

> > Source0:        %{url}/archive/RELEASE-%{version}.tar.gz
> This should be ...RELEASE_{%version}...
> 

Fixed.

> > %defattr(-,root,root,-)
> Not necessary in Fedora and somewhat recent RHEL.
> 

Fixed

> - Large documentation must go in a -doc subpackage. Large could be size
>   (~1MB) or number of files.
>   Note: Documentation size is 2662400 bytes in 126 files.
>   See:
>   http://fedoraproject.org/wiki/Packaging/Guidelines#PackageDocumentation
> 
> It'd be nice to split out a python-anyconfig-doc subpackage with the docs.
> 

Fixed.

> 
> During build I see the following error:
> >     import cbor
> > ImportError: No module named cbor
> Is some dependency missing?
> 

There is support for a backend (cbor) that does not currently have a package in Fedora. If it's okay with you, I'd prefer to not block this package on the one missing backend.

This anyconfig package is a dependency of the package I'm ultimately looking to get into Fedora (molecule). However, anyconfig's support for cbor is not a part of my critical path.

I am willing to work on a python-cbor package after anyconfig is in Fedora, if that works for you.


> and later:
> > toml.py:docstring of anyconfig.backend.toml.Parser._load_from_stream_fn:6: WARNING: Definition list ends without a blank line; unexpected unindent.
> > /builddir/build/BUILD/python-anyconfig-RELEASE_0.9.3/anyconfig/backend/xml.py:docstring of anyconfig.backend.xml._tweak_ns:4: WARNING: Field list ends without a blank line; unexpected unindent.
> > /builddir/build/BUILD/python-anyconfig-RELEASE_0.9.3/docs/api/anyconfig.cli.rst:4: WARNING: autodoc: failed to import module u'anyconfig.cli'; the following exception was raised:
> > Traceback (most recent call last):
> >   File "/usr/lib/python2.7/site-packages/sphinx/ext/autodoc.py", line 658, in import_object
> >     __import__(self.modname)
> >   File "/builddir/build/BUILD/python-anyconfig-RELEASE_0.9.3/anyconfig/cli.py", line 42, in <module>
> >     sys.stdout = codecs.getwriter(_ENCODING)(sys.stdout)
> >   File "/usr/lib64/python2.7/codecs.py", line 1009, in getwriter
> >     return lookup(encoding).streamwriter
> > TypeError: lookup() argument 1 must be string, not None
> > done
> 


This looks like a bug in the docs. I'll point it out to upstream.


> And now the hard part: what is the difference in behaviour or output between
> anyconfig-2 and anyconfig-3?

There is no difference, AFAICS.

> 
> And also (a question for upstream): why is the executable called
> "anyconfig_cli" and not just "anyconfig"?

Fixed in the spec until it's fixed upstream.


I've updated the spec and srpm. Same URLs as in comment #7.

(In reply to Brett Lentz from comment #11)
> (In reply to Zbigniew Jędrzejewski-Szmek from comment #10)
> > > %global sum Python library to load and dump configuration files in various formats
> > Using normal Summary: and then %summary subsequently saves one line ;)
> > 
> 
> Not true. The summary is used in 3 places because of the python2 & python3
> sub-packaged. The macro saves copy/pasting the same text in 3 places.  :)

I didn't mean copying the text three times. I meant something like this:
Summary: blah blah blah
...
Summary: %summary
...
Summary: %summary

Macro %summary is automatically defined to the contents of the last Summary line.

> > During build I see the following error:
> > >     import cbor
> > > ImportError: No module named cbor
> > Is some dependency missing?
> > 
> 
> There is support for a backend (cbor) that does not currently have a package
> in Fedora. If it's okay with you, I'd prefer to not block this package on
> the one missing backend.
> 
> This anyconfig package is a dependency of the package I'm ultimately looking
> to get into Fedora (molecule). However, anyconfig's support for cbor is not
> a part of my critical path.
> 
> I am willing to work on a python-cbor package after anyconfig is in Fedora,
> if that works for you.

Sure, whatever works. There is no obligation to package everything possible. If something is an optional dependency, it's entirely reasonable to leave it for later or to somebody else who actually needs it.

> > and later:
> > > toml.py:docstring of anyconfig.backend.toml.Parser._load_from_stream_fn:6: WARNING: Definition list ends without a blank line; unexpected unindent.
> > > /builddir/build/BUILD/python-anyconfig-RELEASE_0.9.3/anyconfig/backend/xml.py:docstring of anyconfig.backend.xml._tweak_ns:4: WARNING: Field list ends without a blank line; unexpected unindent.
> > > /builddir/build/BUILD/python-anyconfig-RELEASE_0.9.3/docs/api/anyconfig.cli.rst:4: WARNING: autodoc: failed to import module u'anyconfig.cli'; the following exception was raised:
> > > Traceback (most recent call last):
> > >   File "/usr/lib/python2.7/site-packages/sphinx/ext/autodoc.py", line 658, in import_object
> > >     __import__(self.modname)
> > >   File "/builddir/build/BUILD/python-anyconfig-RELEASE_0.9.3/anyconfig/cli.py", line 42, in <module>
> > >     sys.stdout = codecs.getwriter(_ENCODING)(sys.stdout)
> > >   File "/usr/lib64/python2.7/codecs.py", line 1009, in getwriter
> > >     return lookup(encoding).streamwriter
> > > TypeError: lookup() argument 1 must be string, not None
> > > done
> > 
> This looks like a bug in the docs. I'll point it out to upstream.
Great.

> > And now the hard part: what is the difference in behaviour or output between
> > anyconfig-2 and anyconfig-3?
> 
> There is no difference, AFAICS.
OK. If there is no difference, then only one version of the executable should be packed. See https://fedoraproject.org/wiki/Packaging:Python#Executables_in_.2Fusr.2Fbin:
> If the executables provide the same functionality independent of whether they are run on top of Python 2 or Python 3, then only the Python 3 version of the executable should be packaged.

You went through the effort to get the symlinks right, but it now seems that not actually necessary ;(

FYI.

I've released the new version 0.9.4 contains rpm related fixes and ...

(In reply to Zbigniew Jędrzejewski-Szmek from comment #12)
> (In reply to Brett Lentz from comment #11)
> > (In reply to Zbigniew Jędrzejewski-Szmek from comment #10)
> > > > %global sum Python library to load and dump configuration files in various formats
> > > Using normal Summary: and then %summary subsequently saves one line ;)
> > > 
> > 
> > Not true. The summary is used in 3 places because of the python2 & python3
> > sub-packaged. The macro saves copy/pasting the same text in 3 places.  :)
> 
> I didn't mean copying the text three times. I meant something like this:
> Summary: blah blah blah
> ...
> Summary: %summary
> ...
> Summary: %summary
> 
> Macro %summary is automatically defined to the contents of the last Summary
> line.

I didn't know this works. Thanks a lot for letting me know. Fixed it in the upstream.
 
> > > During build I see the following error:
> > > >     import cbor
> > > > ImportError: No module named cbor
> > > Is some dependency missing?

To keep dependencies at a minimum, anyconfig can process most ImportError-es at runtime correctly and works well w/o some dependencies like cbor are missing.
And the new version becomes dependent on only some popular libraries by default, so this kind of error disappears as much as possible, I think.

> > > and later:
> > > > toml.py:docstring of anyconfig.backend.toml.Parser._load_from_stream_fn:6: WARNING: Definition list ends without a blank line; unexpected unindent.
> > > > /builddir/build/BUILD/python-anyconfig-RELEASE_0.9.3/anyconfig/backend/xml.py:docstring of anyconfig.backend.xml._tweak_ns:4: WARNING: Field list ends without a blank line; unexpected unindent.
> > > > /builddir/build/BUILD/python-anyconfig-RELEASE_0.9.3/docs/api/anyconfig.cli.rst:4: WARNING: autodoc: failed to import module u'anyconfig.cli'; the following exception was raised:
> > > > Traceback (most recent call last):
> > > >   File "/usr/lib/python2.7/site-packages/sphinx/ext/autodoc.py", line 658, in import_object
> > > >     __import__(self.modname)
> > > >   File "/builddir/build/BUILD/python-anyconfig-RELEASE_0.9.3/anyconfig/cli.py", line 42, in <module>
> > > >     sys.stdout = codecs.getwriter(_ENCODING)(sys.stdout)
> > > >   File "/usr/lib64/python2.7/codecs.py", line 1009, in getwriter
> > > >     return lookup(encoding).streamwriter
> > > > TypeError: lookup() argument 1 must be string, not None
> > > > done
> > > 
> > This looks like a bug in the docs. I'll point it out to upstream.
> Great.

Still I've been looking into this and may take some time to fix them unfortunately.
But I don't think it's critical for packaging.

> > > And now the hard part: what is the difference in behaviour or output between
> > > anyconfig-2 and anyconfig-3?
> > 
> > There is no difference, AFAICS.
> OK. If there is no difference, then only one version of the executable
> should be packed. See
> https://fedoraproject.org/wiki/Packaging:Python#Executables_in_.2Fusr.2Fbin:
> > If the executables provide the same functionality independent of whether they are run on top of Python 2 or Python 3, then only the Python 3 version of the executable should be packaged.
> 
> You went through the effort to get the symlinks right, but it now seems that
> not actually necessary ;(

Fixed in the upstream.


Brett-san,
could you please take a look at the RPM SPEC template in the upstream and try to arrange the new version of RPM SPEC and srpm?
I tried to keep there are least differences between mine (upstream) and yours as much as possible but maybe there are issues remained I'm not aware of or forgot.

Is there a new version of the spec file for review?

(In reply to Zbigniew Jędrzejewski-Szmek from comment #10)
> And also (a question for upstream): why is the executable called
> "anyconfig_cli" and not just "anyconfig"?

"anyconfig" feels too generic for me and it's just a demo program of this library at the time I first made it.
And, to change its name causes other issues (I have to update all its references in the doc at least),
so I want to keep as it is.

OK.

New spec and srpm are now up at the location in comment #7.

Note: I bumped the version to 0.9.4 manually, the spec file still says 0.9.3.

The %changelog in the rpm package is _not_ supposed to be a reflection of the vcs logs of upstream. It should be a short overview of high-level changes relevant to the user of the package. Hence, it'd even be better to say "0.9.4 Update to latest upstream version, minor cleanups", i.e. give no details, rather than overwhelm the users with details they don't care about. E.g. there is no python3.3 in any supported Fedora, and some deprecated or replaced methods are not important for the user. See https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs. This is a minor thing, also a bit of personal preference, and I'm not saying you have to change the style, and especially not for past entries, but please consider it for the future.

+ package name is OK
+ license is acceptable for Fedora (MIT)
+ license is specified correctly
+ builds and install OK
+ Provides/Requires/BR look correct
+ new python packaging macros are used
+ %python_provide macros is used

Package is APPROVED.

Some notes here:

%if 0%{?rhel} == 7
BuildRequires:  python-devel
BuildRequires:  python-setuptools
%else
BuildRequires:  python2-devel
BuildRequires:  python2-setuptools
%endif
...
%if 0%{?rhel} == 7
Requires:       python-setuptools
%else
Requires:       python2-setuptools
%endif

Both python2-devel and python2-setuptools is available (provided) in RHEL 7.



%if 0%{?rhel} == 7
%{python_sitelib}/*
%else
%{python2_sitelib}/*
%endif

%{python2_sitelib} is available in EPEL 7 (if you are targeting plain RHEL, feel free to ignore this).

(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/python-anyconfig. You may commit to the branch "f27" in about 10 minutes.

I see this has been built, but no bodhi update for F27?