Restructured a bit, updated SRPM:
http://people.redhat.com/jpokorny/pkgs/clufter/review/clufter-0.3.3_git.9240dfc-1.fc21.src.rpm

Some other packaging tweaks, added man page for CLI frontend:
http://people.redhat.com/jpokorny/pkgs/clufter/review/clufter-0.3.4-1.fc21.src.rpm

Sorry, it seems I confused description with a summary, so correct one:

> While primarily aimed at (CMAN,rgmanager)->(Corosync/CMAN,Pacemaker)
> cluster stacks configuration conversion (as per RHEL trend), the
> command-filter-format framework (capable of XSLT) offers also other
> uses through its plugin library.

Two more spec changes:
- https://github.com/jnpkrn/clufter/commit/a49bc927f0377f23cce2a4b2209148a8276972b4
- https://github.com/jnpkrn/clufter/commit/b0ca43caa28c8b4272c9900a59ca7dd6261f0ab5

http://people.redhat.com/jpokorny/pkgs/clufter/review/clufter-0.3.5-0.1.a_git.b0ca43c.fc21.src.rpm

Taking.

First of all, would you explain why do you want to introduce many macros?
I have to expand these macros in my mind when reading your spec file...
I have to say currently your spec file is very difficult to read.

Would you expand macros more so that we can read your spec file easily?

Would it help if I commented on those macros in the specfile directly
so one has a better picture of what is going on?

The point is to omit tedious stuff and touch the specfile as little as
possible later on:
- be prepared for out-of-release builds, e.g., to have a correct build
  tag as per [1] automatically according to the tarball name specification
  (note: at the same time, I represent the upstream, so I can make some
         firm assumptions)
- do not need to specify specific (and redundant) day names in the
  changelog (especially annoying)
- ability to make specific builds for local/custom purposes

[1] http://fedoraproject.org/wiki/Packaging%3aNamingGuidelines#Pre-Release_packages

No, again the point is that it is very difficult for other people including me to read your spec file. Even if you comment on your macros, readability does not get better.

Other people may touch your spec file after you import your spec file into Fedora git, e.g. on mass rebuild. In that case we have to expand macros you introduced. Especially:

- many %clufter_foo definition
- unusual %changelog
- needless %foo_description

makes this spec file very hard to read. You think you want to remove redundancy and while I want to understand it the current difficulty to read must be fixed in the first place.

Please follow what other people writes spec files. More explicitly:
- Write %changelog in an usual way
- Remove use %clufter_foo and expand them
- Remove unneeded %foo_description . Anyway you need not write duplicate
  description in subpackages. Write main description only on "main" package.
- Remove clufter_bashcomp clufter_script conditional. These are not needed.

OK, point taken.  I will see what I can do about that ... in the corner
case I may use the current arrangement as a meta-specfile (but then,
I have to figure out how not to expand system-defined macros).

At least fix %changelog. For other things, it is recommend to modify, but if you want I try reading them.

I ended up with something like in [comment 8]: keeping the convoluted
spec as a source, but having a way [2] to simplify it on demand, mainly
for downstream purposes in order not to scare anybody needlessly :)
I hope this makes sense, at least for me it does.

New SRPM with such an unwinded spec file in it:
http://people.redhat.com/jpokorny/pkgs/clufter/review/clufter-0.3.5-1.fc21.src.rpm

[2] https://github.com/jnpkrn/clufter/blob/master/misc/distill-spec

Oh, I've now noticed I should not compress man page on my own.
Will fix that.

For 0.3.5-1

* Builds
  - Your srpm does not build
    http://koji.fedoraproject.org/koji/taskinfo?taskID=8696816
    - Parameterized macro (%autosetup) cannot be wrapped with bracket.
    - also %verify cannot be wrapped.

* URL format
  - For specifying version, better to use %version so that you don't have
    to modify URL format when version is upgraded (this type of macro is
    better to use)
  - Also you can use %{version} in other place (using %version is common)

* %{?_isa} specific depdendency on noarch packages
  - is not allowed because this makes that noarch packages non-arch-independent
    (%{?_isa} can be expanded into one specific architecture, and on other arch
    such "noarch" rpms cannot be installed)

* %post
  - This %post script belongs to no packages (clufter binary rpm itself is
    not created)
  - Rather creating completion file beforehand and getting rid of %verify
    is preferred (for rpm -V).
    - And as actually this is done, I don't think this %post is needed

* completion file
  - Using %{_sysconfdir}/bash_completion.d/ is obsolete. Move completion file
    to %{_datadir}/bash-completion/completions
    https://lists.fedoraproject.org/pipermail/devel/2013-March/180508.html

  - and have some package own %{_datadir}/bash-completion/ (and directories below)
    https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#The_directory_is_owned_by_a_package_which_is_not_required_for_your_package_to_function

* %doc
  - Files under %_datadir/doc , %_datadir/man are automatically marked as %doc
    and redundant %doc should be removed.

* License
  - Files under doc/ are under GFDL so python-clufter should have "GPLv2+ and GFSL"
    license tag.

* %files
  * python-clufter
    - %exclude %{_bindir}/clufter is redundant.

% %files -n usage with having %{name}- prefix
  - %files -n  clufter-cli is sufficient with "%files cli"
  - Similarly, %package lib-general is better than %package -n clufter-lib-general

%check || :
  - The usage "|| :" here is the old craft and should not be used any longer
    (There was a discussion / decision about this long times ago - I no longer remember
    when)
  - For ${ret}
    rpmbuild invokes bash shell with "set -e", so if ./run-check exits with non-0 status
    rpmbuild will abort there, so ret=$? is not needed.

* Some rpmlint stuff
  - Remove macro-in-comment rpmlint

One additional note
- Placing bash completion files under %_datadir (currently) means that we no longer treat this as %config file.

> Your srpm does not build

Oops, sorry, accidentally I've started COPR build with full-fledged spec
(which passed), the subsequent build was skipped (same NVR/SRPM URL), but
it was at the time I was working on a fixed version already ([comment 11]),
and I hit another problem with COPR build later on [3].

I was hoping to move forward faster (and hence provide updated SRPM sooner)
but still at massaging that.

> URL format with %version

I am using a custom file-naming scheme for pre-releases (well, following
what 'python setup.py sdist' produces, and this is not pre-release
specific; mostly because of clashes with RPM versioning) and the original
meta-specfile is accustomed to that.  I'll consider making it play better
for standard releases, exactly as you suggest.

Do I understand it correctly that another suggestion is to use "%version"
instead of "%{version}"?

> %{?_isa} specific depdendency on noarch packages

Ah, %{?_isa} is to be used only within transitive closure of arch-specific
packages only (provided that the dependency itself is not noarch)!

Just for the clarity: the only arch-specific subpackage is python-clufter
as it builds and uses a private binary (ccs_flatten).

> %post

It was intended to allow for dynamic regeneration of Bash completion file
upon installing/removing plugins (e.g., clufter-lib-pcs that offers a new
command, which should be reflected in the completion file).  Currently
this hasn't been tested, I have to look at that.

Creating beforehand is sort of an optimization: any computation that can
be precomputed (memoized) should be done early, once for all.

> completion file

Thanks, I have no idea how could I find this out except for studying spec
of bash-completion!  It's a shame that even Packaging Guidelines page
shows off /etc/bash_completion.d [4].

Re: second point, good catch, I'll will make the package own that dir
itself (bash-completion is completely optional).

Re: %config, that's good, it is not a configuration file, really
(should not be backed up, etc.)

> %doc

Thanks again for teaching me new stuff.

> License (GFDL)

Well, this is embarrassing :)

> %files

Thanks.

> %files -n usage with having %{name}- prefix

Result of meta-specfile arrangement, will think about that.

> usage of "|| :"

Yes, I learnt this from some authoritative docs, and four characters
overhead is negligible enough to put up with that, IMHO.

> ${ret}

I am aware of "set -e", but this is more robust (no macro will
accidentally bring "set +e", etc.)

> Remove macro-in-comment rpmlint

Ah, this is a result of meta-specfile simplification process
(original contains "%%"), will fix it.


[3] https://copr-fe.cloud.fedoraproject.org/coprs/jpokorny/clufter/build/68529/
[4] https://fedoraproject.org/w/index.php?title=Packaging:Guidelines&rd=Packaging%2FGuidelines#The_directory_is_wholly_contained_in_your_package.2C_or_involves_core_functionality_of_your_package

(In reply to Jan Pokorný from comment #14)
> Do I understand it correctly that another suggestion is to use "%version"
> instead of "%{version}"?

- Well, usually macro is wrapped with bracket (this is preferable to
  avoid some confusion), however there are some exceptions (like
  parameterized macros) where macro cannot be wrapped.

> > %post
> 
> It was intended to allow for dynamic regeneration of Bash completion file
> upon installing/removing plugins (e.g., clufter-lib-pcs that offers a new
> command, which should be reflected in the completion file).  Currently
> this hasn't been tested, I have to look at that.

Well, as I said now bash completion files are to be installed under
%_datadir, which means that this file is not expected to be modified.
If you want to modify, it must create symlink which points to some files
under %_sysconfdir (usually under %_sysconfdir/%name), and modifying file
must be done for files under %_sysconfdir.

> Yes, I learnt this from some authoritative docs, and four characters
> overhead is negligible enough to put up with that, IMHO.

- Well, (while I no longer remember when the decision was made), there
  is already a consensus that "|| :" after %check must be removed.

> Well, as I said now bash completion files are to be installed under
> %_datadir, which means that this file is not expected to be modified.
> If you want to modify, it must create symlink which points to some files
> under %_sysconfdir (usually under %_sysconfdir/%name), and modifying
> file must be done for files under %_sysconfdir.

Great point as my temporary solution was just change the path and this
"/usr/share should be static" hadn't occured to me.
On the other hand it's not entirely true (after fresh yum update):

$ find /usr/share -mtime -1 | grep cache
> /usr/share/mime/mime.cache
> /usr/share/mime/text/cache-manifest.xml
> /usr/share/icons/hicolor/icon-theme.cache
> /usr/share/applications/mimeinfo.cache

So I wonder if it is better to follow what you advised (and expose
new risks, e.g., with dangling symlink for whatever reason) or to
be a bit liberal and just change the path.

> - Well, (while I no longer remember when the decision was made), there
>   is already a consensus that "|| :" after %check must be removed.

Main issue is how hard is to find anything on this topic (search engines
are not really ready for such queries).  My original reasoning was that
I'd like to be compatible with the current major release of RPM where
possible (4), but it seems that there never will be a proper major 5
(see rpm5.org project I don't understand too much) and this is the
respective source/change I followed:

http://rpm.org/gitweb?p=max-rpm.git;a=commitdiff;h=12e31f963b5273494183eec10466f775ffd04c89#patch3

But I checked specs of several high-profile and will remove that tail.

> So I wonder if it is better to follow what you advised (and expose
> new risks, e.g., with dangling symlink for whatever reason) or to
> be a bit liberal and just change the path

http://www.linuxbase.org/betaspecs/fhs/fhs.html#usrshareArchitectureindependentData

agrees about a static nature of the files, so I should rather use
the symlink variant.

Mamoru, sorry for the lag.  I was partly busy with an extension to the
arrangement mentioned in [comment 10] (as a byproduct, there is now
an isolated/clufter-agnostic project distill-spec [5]).

I've tried to addresss your remarks and also made a few additional
changes, e.g., there is now a filesystem in Requires for
/usr/share/man/man1:

https://people.redhat.com/jpokorny/pkgs/clufter/review/clufter-0.3.6-0.3.a_git.e88876e.fc21.src.rpm
https://copr-fe.cloud.fedoraproject.org/coprs/jpokorny/clufter/build/69760/

Please note that the spec doesn't look entirely sleek now, especially
with the repeated occurrences of "0.3.6a-git.e88876e".  This come from
pre-release versioning (its arrangement from the meta-specfile).
Please ignore this for now and once you are otherwise happy, I will
release a proper version and all such occurrences will turn into
"%{version}" along with other simplifications that will the be
possible at that point.

You can then re-review such a final spec and give me a final ACK.


[5] https://github.com/jnpkrn/distill-spec

Well, 

* Files listed twice
------------------------------------------
warning: File listed twice: /etc/clufter/bash-completion
warning: File listed twice: /usr/share/bash-completion/completions/clufter
warning: File listed twice: /usr/share/doc/clufter-0.3.6a-git.e88876e/fdl-1.3.txt
warning: File listed twice: /usr/share/doc/clufter-0.3.6a-git.e88876e/gpl-2.0.txt
------------------------------------------

* Directory ownership
  - /usr/share/bash-completion/ is not owned unless you install bash-completion

Other things:
* Redundant requires
  - "Requires: filesystem" is not needed.

* Redundant %doc
  - Again files / directories under /usr/share/doc is automatically marked as
    %doc and redundant %doc for %{_defaultdocdir}/%{name}-XXX should be
    removed.

* Source using git
  - Please follow
    http://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control
    http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Non-Numeric_Version_in_Release
    - Write how to create the source tarball
    - It is advised to include date in release number when using snapshot.

* Scriptlet error
----------------------------------------------
$ sudo rpm -ivh clufter-lib-general-0.3.6-0.3.a_git.e88876e.fc21.noarch.rpm 
Preparing...                          ################################# [100%]
Updating / installing...
   1:clufter-lib-general-0.3.6-0.3.a_g################################# [100%]
/var/tmp/rpm-tmp.BSuniD: line 5: syntax error near unexpected token `&&'
/var/tmp/rpm-tmp.BSuniD: line 5: `  && /usr/bin/clufter --completion-bash > "${bashcomp}" 2>/dev/null || :'
warning: %post(clufter-lib-general-0.3.6-0.3.a_git.e88876e.fc21.noarch) scriptlet failed, exit status 2
----------------------------------------------

ping?

ping again?

> * Files listed twice
> ------------------------------------------
> warning: File listed twice: /etc/clufter/bash-completion
> warning: File listed twice: /usr/share/bash-completion/completions/clufter
> warning: File listed twice: /usr/share/doc/clufter-0.3.6a-git.e88876e/fdl-1.3.txt
> warning: File listed twice: /usr/share/doc/clufter-0.3.6a-git.e88876e/gpl-2.0.txt
> ------------------------------------------

fixed

> * Directory ownership
>   - /usr/share/bash-completion/ is not owned unless you install bash-completion

fixed

> Other things:
> * Redundant requires
>   - "Requires: filesystem" is not needed.

True, this is already required with whatever depends on glibc, which is
practically everything.

> * Redundant %doc
>   - Again files / directories under /usr/share/doc is automatically marked as
>     %doc and redundant %doc for %{_defaultdocdir}/%{name}-XXX should be
>     removed.

ok, changed (I do not consider it a direct violation, but is sensible)

> * Source using git
>   - Please follow
>     http://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control
>     http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Non-Numeric_Version_in_Release
>     - Write how to create the source tarball

done, see Source1 (misc/run-sdist-per-commit), but see a note that
pre-release package is supposed to be exceptional below (normally
a standard distribution URL [people.redhat.com/jpokorny/pkgs/clufter]
will be used)

>     - It is advised to include date in release number when using snapshot.

I am not going to follow this, hopefully it's OK.
Upstream is not split in any way and commit hashes identify the state
(and specific date, for that matter) of the repo uniquely.
I am not going to push builds with commit-identified upstream versions
(as opposed to blessed/tagged final releases) anyway, at least not
into non-rawhide.

It's also a special case for pre-inclusion into Fedora, which should make
sure that even if I am ever packaging pre-release version, it fits into
the guidelines.  As mentioned in [comment 18], once you are happy with the
packaging of pre-release version, I will tag a release and offer
a "proper version" SRPM for last-time check.


> * Scriptlet error
> ----------------------------------------------
> $ sudo rpm -ivh clufter-lib-general-0.3.6-0.3.a_git.e88876e.fc21.noarch.rpm 
> Preparing...                          ################################# [100%]
> Updating / installing...
>    1:clufter-lib-general-0.3.6-0.3.a_g################################# [100%]
> /var/tmp/rpm-tmp.BSuniD: line 5: syntax error near unexpected token `&&'
> /var/tmp/rpm-tmp.BSuniD: line 5: `  && /usr/bin/clufter --completion-bash > "${bashcomp}" 2>/dev/null || :'
> warning: %post(clufter-lib-general-0.3.6-0.3.a_git.e88876e.fc21.noarch) scriptlet failed, exit status 2
> ----------------------------------------------

fixed, thanks for discovery!


SRPM: https://people.redhat.com/jpokorny/pkgs/clufter/review/clufter-0.3.6-0.1.a_610fb11.fc21.src.rpm
COPR: https://copr-fe.cloud.fedoraproject.org/coprs/jpokorny/clufter/build/79155/

Well, at lease please increase release number
even during review process (i.e. 0.3.XXX -> 0.4.XXX)

* Calling "rpm" inside rpmbuild is forbidden
  - While I don't remember where it is written as document
    (currently with google I just found
    http://www.redhat.com/promo/summit/2008/downloads/pdf/Wednesday_130pm_Tom_Callaway_OSS.pdf
    page 25), actually I remember that calling "rpm" inside
    rpmbuild process is forbidden.

    One of the reason is that rpmdb created inside mock chroot is
    created by rpm "outside" mock chroot - and calling rpm
    inside mock chroot may just see rpmdb corruption.

* bash completion file treatment on scriptlets
  - Well, on second thought, I think that writing to modify
    the "actual" file (i.e. %_sysconfdir/%name/bash-completion)
    rather than ${bashcomp} (this is symlink) is preferable,
    - as it clearly shows that scriptlet is modifying files
      under %_sysconfdir and actually not modifying the files
      under %_datadir.

I think only the above two issues are left.

re increase release number:
You are right I should be careful about that.
As a result, current pre-release (and pre-inclusion) is
clufter-0.3.6-0.4.a_a811d08 (see below).

re "rpm" inside rpmbuild:
My expectations were that mere read-only query is OK, but in order
not to raise the alarms, I've resorted to another approach to the
problem description and possible better solution of which I've stated
at [bug 1196724] (targetting RHEL in the hope of getting more
attention, or is it worth fedora-devel ML discussion?).

re bash completion file treatment on scriptlets
Changed this as well, makes a perfect sense.


SRPM:
http://people.redhat.com/jpokorny/pkgs/clufter/review/clufter-0.3.6-0.4.a_a811d08.fc21.src.rpm
SPEC:
http://people.redhat.com/jpokorny/pkgs/clufter/review/clufter-0.3.6-0.4.a_a811d08.spec
COPR
(sorry, this one is actually called clufter-0.3.6-0.2.a_a811d08,
and copr builder had some intermittent issues in 1/4 buids, no clue why)
https://copr-fe.cloud.fedoraproject.org/coprs/jpokorny/clufter/build/79480/

Approving.

--------------------------------------------------
  This package (clufter) is APPROVED by mtasaka
--------------------------------------------------

New Package SCM Request
=======================
Package Name: clufter
Short Description: Tool/library for transforming/analyzing cluster configuration formats
Upstream URL:  https://github.com/jnpkrn/clufter
Owners: jpokorny
Branches: f20 f21 f22
InitialCC:

Thank you, Mamoru!

Git done (by process-git-requests).

Thank you, Jon.

Just for the sake of transparency, at the occasion of getting introduced
into Fedora, I've bumped the version of clufter to 0.10.0 and also from
this point, I'd like to keep packaging only complete tagged versions,
not pre-releases as mostly done prior to inclusion.

There is a minimal delta to the accepted version:

SPEC:
https://people.redhat.com/jpokorny/pkgs/clufter/review/clufter-0.10.0-1.spec
SRPM:
https://people.redhat.com/jpokorny/pkgs/clufter/review/clufter-0.10.0-1.fc21.src.rpm

clufter-0.10.0-1.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/clufter-0.10.0-1.fc20

clufter-0.10.0-1.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/clufter-0.10.0-1.fc21

clufter-0.10.0-1.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/clufter-0.10.0-1.fc22

clufter-0.10.1-1.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/clufter-0.10.1-1.fc20

clufter-0.10.1-1.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/clufter-0.10.1-1.fc21

clufter-0.10.1-1.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/clufter-0.10.1-1.fc22

clufter-0.10.1-1.fc20 has been pushed to the Fedora 20 stable repository.  If problems still persist, please make note of it in this bug report.

clufter-0.10.1-1.fc22 has been pushed to the Fedora 22 stable repository.  If problems still persist, please make note of it in this bug report.

clufter-0.10.1-1.fc21 has been pushed to the Fedora 21 stable repository.  If problems still persist, please make note of it in this bug report.