Oh boy, that is a big PR

I hasn't been able to review all of it yet, but my notes are starting to get quite long, so I am going to share my first impressions.

User POV

Running rpkg srpm in non-git directory fails with

git_pack: Error during source creation.
git_changelog: Start by setting origin URL.
fatal: Not a git repository (or any of the parent directories): .git
Wrote: /tmp/rpkg/foo.spec
error: line 2: Empty tag: Name:
can't parse specfile

It doesn't matter to me whether it should or shouldn't work without git initialization, I would just expect a more user-friendly error.

Similarly, running rpkg srpm in a git directory, which however doesn't have the origin remote (i.e. local project which hasn't been pushed yet), fails with

Wrote: /tmp/rpkg/foo.spec
error: line 2: Empty tag: Name:
can't parse specfile

It is because of the {{{ git_name }}} macro, but there should be a better message. As a user, I don't want to check the implementation of git_name macro and see that it takes the name from origin repo URL.

However, even though I don't use the {{{ git_name }}} and {{{ git_version lead=1 }}} macros, I still get

Wrote: /tmp/rpkg/bar.spec
error: line 3: Empty tag: Version:
can't parse specfile

Running rpkg sources seems to do nothing, but I would expect it to generate the tar that is packed into SRPM when running rpkg srpm. What is the content of it anyway? It looks like everything in the git directory, but I think that it rather should be only committed files because the final SRPM is labeled with the commit hash.

Lastly, Wrote: /tmp/rpkg/foo.spec. I don't like the idea of using always the same directory. It should be used mkdtemp() for that. Those generated tmp directories can be under /tmp/rpkg for sure, ...

Code review

What is the purpose of rpkg-client.spec.rpkg, rpkg-client.spec.rpkg.cwd and rpkg-client.spec.rpkg.dir? Are those supposed to be just examples of different kinds of macros? In this case they should be much shorter. I like that those are whole spec files that can be copy-pasted and tried, but (Build)Requires, install section and changelog should be greatly reduced.

I would prefer the same formatting of the spec files as in the documentation, which is

Name:          repo
Version:       1.0.git.1.01c067a5
Release:       1%{?dist}
Summary:       RPM packaging utitility
License:       GPLv2+
URL:           https://pagure.io/rpkg-client.git

See for example https://rpm-packaging-guide.github.io/#hello-world .

Macros in bash are one thing, but why do we need also preproc.sh and pack_sources.sh in bash? To me, it doesn't really look like a true part of rpkg but rather a hack. Also mixing languages is a bad thing. I understand the reason for using bash for implementing macros, but that should end there.

Philosophical topics

Those are just topics that I have in my head. I am sure that everyone will have a different opinion on this topics, so we can't say what is right or wrong here, but I just want to share what I think.

First, macros in bash. Some may say that it is a good idea because bash is kind of a universal language, everyone kind of knows it and users can simply implement their own macros in the project. Well, IMHO the premise is good, but to me, the bash is ugly as hell and looking on some macros, a write-only language. And I am not talking only about this type of constructions sed -e 's|/|-|g' -e 's|-$||' which I must say, are my favorites. I don't know if it is just me, but I feel like anyone can really re-open such project after few months and understand it. Personally, I would give an enormously huge +1 to using python for macro implementation and jinja2 for templating since the rest of the project is in python.

And it looked like there will be more of the topics, but at this point I really have just one.

Please don't take this feedback too offensively, I don't mean it this way at all. I just needed tu dump my head.

I would rather force rpmlint upstream to do this a right way:
https://github.com/rpm-software-management/rpmlint/issues/125

Noone knows who is going to parse the .rpmlint* files, and the only one
which one can think of actually doesn't (rpmlint itself).

Why the && f.startswith('.') check? Can the file be like ./something?

missing doublequotes

Well, saying once more publicly -- I don't think such code by-definition belongs
to rpkg (tool to work with dist-git, it could well mean that rpkg will never
happen to be distribution-default dist-git client and distros will have to stick
with their own fedpkg, rhpkg, centpkg forks). But this is not first time we
broke this rule, so it's getting less and less priority; maybe it's leading to another
fork say dist-git-client which will do nothing else than clone && rpmbuild -bs.

To me, the defaults for git_version/git_name are real magic; and dictate "how
users should do git repo maintenance ... but OTOH the shell
commands are supposed to be re-defined by rpkg config file in-repo. Sounds OK.
(Note that this may well bring additional srpm-buildrequires (build requires
needed for building the srpm).

I'm not the target audience for this feature, but it could be interesting to someone,
dunno. Thanks for the research.

Well, saying once more publicly -- I don't think such code by-definition belongs
to rpkg (tool to work with dist-git, it could well mean that rpkg will never
happen to be distribution-default dist-git client and distros will have to stick
with their own fedpkg, rhpkg, centpkg forks). But this is not first time we
broke this rule, so it's getting less and less priority

Yes, I am concerned about this too. The previous changes that broke the rule were quite small and helped to a unification of SCM method in Copr, which was really helpful.

Now I am not sure. It is a big change ... We should really talk about how important for us is the goal for rpkg to become the distribution-default dist-git client.

6 new commits added

• update README.md
• use git rev-parse --short HEAD to get the short commit hash
• no HEAD handling
• propagate errors
• more debugging info about spec substitutions
• whitespace tweaking + is_packed fixed

3 new commits added

• add missing quotes
• when bash function is invoked directly from console, ${FUNCNAME[-1]} is not main
• add check for dirty working tree into git_archive

2 new commits added

• encode also the latest file modification time into version if working tree is dirty
• fix rpmlint config file lookup in lint method

@frostyx: ad. the error messages:

that should fixed now by 16e70e5. Can you retry on https://copr.fedorainfracloud.org/coprs/clime/rpkg-client/build/726008/.

Running rpkg sources seems to do nothing, but I would expect it to generate the tar that is packed into SRPM when running rpkg srpm.

rpkg sources actually just downloads sources from distgit. It does not do anything else. For dynamic generation of a source from Git repo, there is rpkg spec --sources. When rpkg srpm is invoked, sources are downloaded from DistGit if any and if you use {{{ git_pack }}} or {{{ git_archive}}} for some Source: field in the input spec file, a dynamically generated tarball is put into /tmp/rpkg as well.

What is the content of it anyway? It looks like everything in the git directory, but I think that it rather should be only committed files because the final SRPM is labeled with the commit hash.

Good point. I fixed the versioning to also take into account the current working tree state.
Actually, only {{{ git_pack }}} uses the working tree. {{{ git_archive }}} uses Git index (hence individual commits in Git history) to generate the tarball. It depends on user what he/she will pick (by the way, git_archive gives behavior closer to tito).

Lastly, Wrote: /tmp/rpkg/foo.spec. I don't like the idea of using always the same directory. It should be used mkdtemp() for that. Those generated tmp directories can be under /tmp/rpkg for sure, ...

I wouldn't like having the content hidden in some random directory names. The content generated by rpkg should be easy to find so that user can investigate it and use it further (e.g. to send a build srpm to COPR for building). Naming of the file depends on a package name, which should be good enough.

But what I would like to do is to add --rpmtree parameter to rpkg which will actually start generating the output files into the classic rpmbuild tree: BUILD BUILDROOT RPMS SOURCES SPECS SRPMS. I actually want to make this a default behaviour (and only generate the files directly into /tmp/rpkg if user asks for it by --outdir param or a config option). But this will likely come after this PR.

What is the purpose of rpkg-client.spec.rpkg, rpkg-client.spec.rpkg.cwd and rpkg-client.spec.rpkg.dir? Are those supposed to be just examples of different kinds of macros? In this case they should be much shorter. I like that those are whole spec files that can be copy-pasted and tried, but (Build)Requires, install section and changelog should be greatly reduced.

I quickly copied and modified rpkg-client.spec.rpkg. Better example templates will land with unit-tests (that will be part of this PR).

Macros in bash are one thing, but why do we need also preproc.sh and pack_sources.sh in bash? To me, it doesn't really look like a true part of rpkg but rather a hack. Also mixing languages is a bad thing. I understand the reason for using bash for implementing macros, but that should end there.

preproc.sh is the a script responsible for invoking a given macro and also for doing some setup (like e.g. sourcing the user definitions from rpkg.sh in the current working directory or maintaining function call cache - this will be described in docs). It could be done even without this file but the subprocess invocation from python would be then much more complex. That's why I kept it.

pack_source.sh is a helper script that produces results similar to git archive command (but pack_source.sh uses tar and it works on current working tree state as opposed to git_archive). Having this particular functionality separated makes it easier to test it (manually or from a test script). It is a useful script on its own.

I would give an enormously huge +1 to using python for macro implementation and jinja2 for templating since the rest of the project is in python.

bash over python was chosen for the macro implementation for the following reaons:

1) it is something, packagers might be used to already thanks to %() rpm construct (shell invocation)
2) All the macros currently implemented need lots of git calls - it is much more simpler for me to do those in shell than in python by using e.g. GitPython library
3) well, the previous reasons are the main ones but there are more. E.g. that you can do a lot with just git+coreutils installed (which almost everyone has by default) and using bash allows users to really easily provide his/her own macro definition (just by putting a bash function into rpkg.sh). It really cannot be simpler than that. Also using bash allowed for really compact implementation (preproc.py + preproc.sh have together 83 + 15 = 98 lines and that's really quite crazy considering that these two files basically implement the rpkg template language - all the rest are by default provided macros).

jinja2 was not picked because it uses just {{, }} and pontentially {%, %} tags which is too similar to rpm's own macros '%{ ... }`. It is also really a big overkill here and actually it also misses some features that we really need (function call cache that I will explain later).

Please don't take this feedback too offensively, I don't mean it this way at all. I just needed tu dump my head.

No, I am totally glad for it. I wanted to get as much feedback as possible.

@praiskup:

Well, saying once more publicly -- I don't think such code by-definition belongs
to rpkg (tool to work with dist-git, it could well mean that rpkg will never
happen to be distribution-default dist-git client and distros will have to stick
with their own fedpkg, rhpkg, centpkg forks). But this is not first time we
broke this rule, so it's getting less and less priority; maybe it's leading to another
fork say dist-git-client which will do nothing else than clone && rpmbuild -bs.

Well, rpkg srpm will do pretty much exactly what you describe at the end if:

1) user is not using any preprocessor macros
2) preprocess_spec is set to False in rpkg configuration
3) both

Nothing should be colliding with the existing use-cases.

Yes, I am concerned about this too. The previous changes that broke the rule were quite small and helped to a unification of SCM method in Copr, which was really helpful.
Now I am not sure. It is a big change ... We should really talk about how important for us is the goal for rpkg to become the distribution-default dist-git client.

(...actually by @frostyx)

Note that the current state here in this branch (and PR) is much more favourable for rpkg. That's because the spec template feature offered here makes the "auto-packing" explicit (before you couldn't tell from a spec file itself where the Source: is being obtained from, which is a huge drawback - now git_pack or git_archivemacros being invoked directly from spec fix that). This PR also offers to switch the both extra features (spec preprocessing + the deprecated auto-packing) off, which is even unnecessary in the case of spec templates.

Btw. a pretty quick tutorial is now available here: https://docs.pagure.org/rpkg-util/tutorials.html#spec-templates-from-scratch (it will need a revision).

1 new commit added

• use brace expansion to generate radix digits, fix dirty check position in git archive

Can you summarize what this is supposed to do?

Can you summarize what this is supposed to do?

it allows for generation of spec files from git metadata and git content. So you can e.g. have a spec file that has value of the Version: field generated from your git repo history.

A couple of questions here:

1. Can we have a way to make rpkg do post/pre-release properly in the Version field? Something like the tilde versioning proposal I have for Fedora (which other distributions already follow).

2. Can we have an option for the changelog entries to be generated with full time stamps? That's been supported since RPM 4.14 using the date format style. We do this for rust2rpm (with openSUSE currently, but I think we'll change it for everyone), for example.

utitility -> utility

A couple of questions here:

Can we have a way to make rpkg do post/pre-release properly in the Version field? Something like the tilde versioning proposal I have for Fedora (which other distributions already follow).

Yes, you can specify follow argument for git version so it e.g becomes:

{{{ git_version follow=1.0~rc1 }}}

which will generate 0.1.0~rc1 optionally with dynamic suffixes (.git., .wtree.) when we are or non-tag in the current git history or if our working tree is dirty.

If follow parameter is specified, the second version field becomes fixed (normally it is derived from number of tags for a given package name and lead - lead is "major" number but we call it 'lead'). Developer can remove follow argument later and return to automatic versioning of the minor number (lead number is always versioned manually) .

But the problem with ~ here is that e.g. rpkg-util-1.1.4~rc1-1 is not a valid tag name according to git so a user wouldn't be able to create a tag on such version.

Can we have an option for the changelog entries to be generated with full time stamps? That's been supported since RPM 4.14 using the date format style. We do this for rust2rpm (with openSUSE currently, but I think we'll change it for everyone), for example.

Yes, sure. Actually full timestamp is default format for %(taggerdate) in

git tag -l --format="%(taggerdate)"

In then end I put %(taggerdate:format:%a %b %d %Y) there so the changelog looks more human-like. But we can add a parameter for full timestamps, for sure e.g.

{{{ git_changelog full_timestamps=1 }}}

{{{ git_changelog full_timestamps=1 }}}

Potentially a full header could be parametrized. Currently it is:

'* %(taggerdate:format:%a %b %d %Y) %(taggername) %(taggeremail)'

Potentially a full header could be parametrized. Currently it is:
'* %(taggerdate:format:%a %b %d %Y) %(taggername) %(taggeremail)'

That's actually not the full header as $verrel is not there. I've just added header_date_format for git_changelog macro.

I've redone the bumping and versioning logic so that the relevant code is only in lib.sh and nowhere else (before there was some logic in lib.sh and also in rpkglib/__init__.py:add_tag).

git_version now works in the following way when spec template is evaluated:

• if we are not bumping (i.e. we are not calling rpkg tag)
  • if follow is specified by user explicitly, return $lead.$follow.<dynamic suffixes>
  • otherwise, extract follow from the latest tag as the last version component (after the last .) and return $lead.$follow.<dynamic suffixes>

• if we are bumping (i.e. we are calling rpkg tag)
  • if follow is specified by user explicitly, return $lead.$follow
  • otherwise, extract follow from the latest tag as the last version component (after the last .) and return $lead.$follow+1. If $follow is not a number, throw an error (manual setting of follow is needed).

I have also added {{{ git_vcs }}} macro (and the related macros: git_dir_vcs and git_cwd_vcs to get the origin of sources. This might come handy if you hold an rpm and you want to track it back to the original Git repository it comes from.

I think this PR is close to finish. I'll try to add some testing (although not sure how exhaustive it will be at this point).

I want to share some final thoughts.

rpkg sources actually just downloads sources from distgit. It does not do anything else. For dynamic generation of a source from Git repo, there is rpkg spec --sources.

Ok, it is good, that there is at least some way how to achieve this - rpkg spec --sources. So, once it is properly documented, I am fine with it. However, to be honest, I don't like the command very much. I wouldn't expect at all, that command rpkg spec ... will generate tarball to me.

Please consider rpkg sources for it. I don't mind if you choose not (re)implementing it for some reason, just please think about it.

pack_source.sh is a helper script that produces results similar to git archive command (but pack_source.sh uses tar and it works on current working tree state as opposed to git_archive). Having this particular functionality separated makes it easier to test it (manually or from a test script). It is a useful script on its own.

I can agree with all of it, but that actually isn't a valid reason for implementing standalone scripts in language A when the whole project is in language B. You might as well do it in Haskell. This way a different test toolkit needs to be used, it puts additional requirements for contributors to know another language on some level and so on. Even if it is a bash that everyone kind of knows.

1) it is something, packagers might be used to already thanks to %() rpm construct (shell invocation)
2) All the macros currently implemented need lots of git calls - it is much more simpler for me to do those in shell than in python by using e.g. GitPython library
3) well, the previous reasons are the main ones but there are more. E.g. that you can do a lot with just git+coreutils installed (which almost everyone has by default) and using bash allows users to really easily provide his/her own macro definition (just by putting a bash function into rpkg.sh). It really cannot be simpler than that.

Well, I've already said all my reasons why not to do it in bash, so I am not going to repeat myself. I can imagine that some people will agree that the bash is the best option here as well as some people that will disagree with it. In the end, someone has to make the decision and pick the language.

To address your comment ...

it is much more simpler for me to do those in shell than in python by using e.g. GitPython library

In my eyes, python + subprocess would still be a much better option than bash

and using bash allows users to really easily provide his/her own macro definition (just by putting a bash function into rpkg.sh)

This may as well be done with python by putting a function into rpkg.py

that should fixed now by 16e70e5. Can you retry on https://copr.fedorainfracloud.org/coprs/clime/rpkg-client/build/726008/.

Yes, I see there are a lot of changes since I tried it, so I will test them as well.

I want to share some final thoughts.

rpkg sources actually just downloads sources from distgit. It does not do anything else. For dynamic generation of a source from Git repo, there is rpkg spec --sources.

Ok, it is good, that there is at least some way how to achieve this - rpkg spec --sources. So, once it is properly documented, I am fine with it. However, to be honest, I don't like the command very much. I wouldn't expect at all, that command rpkg spec ... will generate tarball to me.
Please consider rpkg sources for it. I don't mind if you choose not (re)implementing it for some reason, just please think about it.

Actually only rpkg spec --sources really makes sense. That's because the tarball is generated as a side product of spec template evaluation (when git_pack or git_archive macros are present).

If you are a true functional purist, then this feature is probably not for you (and you can use more complicated syntax that might be added at some point later - it probably won't be added just for this reason though).

pack_source.sh is a helper script that produces results similar to git archive command (but pack_source.sh uses tar and it works on current working tree state as opposed to git_archive). Having this particular functionality separated makes it easier to test it (manually or from a test script). It is a useful script on its own.

I can agree with all of it, but that actually isn't a valid reason for implementing standalone scripts in language A when the whole project is in language B. You might as well do it in Haskell. This way a different test toolkit needs to be used, it puts additional requirements for contributors to know another language on some level and so on. Even if it is a bash that everyone kind of knows.

Well, some languages are more suitable to do one thing and some other are more suitable to do another thing. For text processing and git manipulation, I just prefer bash much more.

If you look e.g. at (a completely different project) firefox here: https://fedoramagazine.org/oxidizing-fedora-try-rust-applications-today/ (the paragraph about Tokei), how many languages are actually used to implement it.

I evaluated bash as much better language to implement the macros and I was really very happy about that choice during the implementation.

1) it is something, packagers might be used to already thanks to %() rpm construct (shell invocation)
2) All the macros currently implemented need lots of git calls - it is much more simpler for me to do those in shell than in python by using e.g. GitPython library
3) well, the previous reasons are the main ones but there are more. E.g. that you can do a lot with just git+coreutils installed (which almost everyone has by default) and using bash allows users to really easily provide his/her own macro definition (just by putting a bash function into rpkg.sh). It really cannot be simpler than that.

Well, I've already said all my reasons why not to do it in bash, so I am not going to repeat myself. I can imagine that some people will agree that the bash is the best option here as well as some people that will disagree with it. In the end, someone has to make the decision and pick the language.
To address your comment ...

it is much more simpler for me to do those in shell than in python by using e.g. GitPython library
self.repo.git.tag
In my eyes, python + subprocess would still be a much better option than bash

and using bash allows users to really easily provide his/her own macro definition (just by putting a bash function into rpkg.sh)

This may as well be done with python by putting a function into rpkg.py

Yes, that would work. Well, we don't have the equivalent implementation in python at hand to compare but I still believe the current implementation would come out better (=simpler).

Feel free to prove me wrong but as I said I was really happy about bash and started to appreciate that language much more.

that should fixed now by 16e70e5. Can you retry on https://copr.fedorainfracloud.org/coprs/clime/rpkg-client/build/726008/.

Yes, I see there are a lot of changes since I tried it, so I will test them as well.

Please, download the latest built package from https://copr.fedorainfracloud.org/coprs/clime/rpkg-client/. I am trying to keep it up-to-date.

Note that I understand all your arguments about bash being a bit arcane, hard-to-read language :) but I would still prefer it over python in certain use-cases (like this one).

...We could offer direct use of rpkg-util/rpkglib/preproc/scripts/pack_sources.sh as an alternative way to pack the sources for functional purists. Then they could avoid the git_pack and git_archive macros and use just what they need out of the rpkg template language if they wanted to. I don't want to offer that as an additional rpkg subcommand (rpkg pack-sources) because there already are tons of "source"-like subcommands. Rather, just put it into /usr/bin/ (/usr/bin/pack-sources) and mention it in docs and man pages as an alternative way to create the source tarball (it could also be a separate package altogether).

I changed the name of the file with user-defined macros to 'rpkg.macros'. It is more obvious like that what is the purpose of the file if included in a user project.

Sorry for additional changes in this PR. I keep discovering things that can be done better. I would like to reach some final state before this is merged.

Basic stuff should be in place so will try to merge.

This PR has been migrated to GitHub as a placeholder issue:
https://github.com/fedora-copr/rpkg-util/issues/7