#1255 Adjust guidelines for %autorelease+%autochangelog
Merged by ngompa. Opened by zbyszek.
zbyszek/packaging-committee rpmautospec  into  master

Download 1255.patch

See https://fedoraproject.org/wiki/Changes/Rpmautospec_by_Default

The general approach is the the previous recommendations are described
with "may" and moved to the end or to a separate page. Use of
rpmautospec is shown briefly, but with enough information for normal
packaging workflows. Links are provided to the rpmautospec docs in
case packagers need more information.

Use example.com for the domain

rebased onto 4019447dad3e1fd6f288137dfb3fd70d293dd93c

rebased onto c999163c821779ca52476b1a04395156fe14e983

Updated with example.com everywhere and some spurious newlines removed.

1{?dist} here is missing the % sign

1 new commit added

  • Adjust "Renaming or replacing" for rpmautospec

rebased onto 82fbee70c968817414bef442589d0b44f81297eb

1{?dist} here is missing the % sign

Fixed. I made the same error in other places, they are fixed now too.

Ok, I read through the whole change set AND the resulting Versioning.adoc.
Thank you for your effort.
This looks good, but I also have multiple suggestions:

** Release field**
I would move this section right under section Epoch: tag.
So the simple tags would be handled first, followed by deep dive into Version:.
I would also prefer both Release and Epoch sections be named similarly
(so either switch to Release: tag or to Epoch field.)

Upstream has never chosen a version
Said section needs an update, too, since it talks about bumping Release.
Maybe just add When %autorelease is used, this happens automatically.?

Tables
I think section Complex versioning with a reasonable upstream would read better if column Full package NEVRA was changed to Version: field.
The RA part was a constant there already,
and with Rpmautospec by Default, it makes even less sense to include it.

The same goes for sections Complex versioning with non-sorting upstream post-release versions and Complex versioning with a pre- and post-release snapshots,
if the few rows where release is incremented are removed.
Having the Release part there does not bring much clarity there.
Perhaps it did when tilde and caret were not available?

Mixed mode
What is the purpose of the Mixed Mode section?
It seems to just describe the traditional way.
Either just remove it, or explain it more.

Also, Mixed mode refers to the Complex versioning section, which makes heavy use of tilde and caret.
They are not available in this context —
the higher level section is about what to do if an old EL version does not support them.

The links are also broken there. [[Section header]] is not a valid Asciidoc link.
Try xref:_section_header[Section header]].

You want to change an old branch without rebuilding the others
I think this would read better and communicate the same thing:

== Only an old branch needs a change
Sometimes, an older branch needs a fix, but the newer branches are fine.
For example, both F{PREVIOUSOSVER} and F{MAJOROSVER} are built from the same dist-git commit, and only F{PREVIOUSOSVER} needs a fix.
If only F{PREVIOUSOSVER} was changed, its `Release` would increase
and thus the E-V-R for F{PREVIOUSOSVER} would sort higher than E-V-R in F{MAJOROSVER}.
To avoid this situation, rebuild the package also in the later branches,
possibly with just an empty commit to make `%autorelease` bump the release.
If the package does not use `%autorelease`, it is also possible to adjust `Release` by appending a number after the dist tag, like `+7.%{?dist}.1+`.

Another section with almost identical name, You want to change an old branch without rebuilding the others, could also be removed, because the same content is here.

Suggestion to simply let older release sort higher than the newer ones is contradicted by section Rawhide is allowed to lag temporarily, so at least that suggestion must be removed.

Navigation
manual-changelogs.adoc should be added to nav.adoc.

Semantic linebreaks
They should be used everywhere.

rebased onto 7762d489bb6fd0bb4a9f1142ff58a5e8dbf14dc0

Thank you for the review. A detailed look like this is always very useful.

I updated the text with the suggested changes. Earlier I resisted to change the order (to Epoch-Release-Simple_versioning-Complex_versioning), but indeed it makes the structure much clearer. I edited all the tables to just have a Version column. I don't think it made much sense to have some like this and others with full N-V-R. But I added a short sentence that gives an example of a full N-V-R (without macros), so it's easier for the reader to visualize the effect.

I’d like the example to still have the same upstream version multiple times, probably mentioning that the release number will be bumped between them.

Do we still have the rule that E-V-R, F(N-1) must be smaller than E-V-R, F(N)? I thought not having that strict requirement was one argument to have the simplified release calculation algorithm.

Thanks for picking up the slack, @zbyszek! Can’t be said too often…

Uh oh, my mistake, the version variables are actually called PREVVER, CURRENTVER and NEXTVER.
So wherever I suggested to use {PREVIOUSOSVER} should be {PREVVER},
and similarly {MAJOROSVER} should be {CURRENTVER}.

Do we still have the rule that E-V-R, F(N-1) must be smaller than E-V-R, F(N)? I thought not having that strict requirement was one argument to have the simplified release calculation algorithm.

At least for Rawhide vs. branched releases, section Rawhide is allowed to lag temporarily only allows that in one specific situation.
I asked to change the wording here so that there would be no possibility of a conflict with that section.

Of course it may be that the rule has been removed, but Rawhide is allowed to lag temporarily has not been updated to match.

Just a tiny one here:
The file is called manual-changelogs, the nav entry says Manual Changelog, but the page title is just Manual Changelog.
Add the 's' to page title as well, or the other way round.

And final comments about the Manual changelogs page:

I would add a link back to the main guidelines,
so that nobody will accidentally stumble on this page and miss %autochangelog completely:

This is an alternative for the recommended method with +%autochangelog+ described in xref:index.adoc#changelogs[Changelogs].

The first example (with Added README file (rhbz#1000042)) is out of place.
The paragraph describing it is gone, and the guidance for adding rhbz# tags is available in the main page.
So, I would just delete the example.

Sections for Multiple Changelog Entries per Release use the wrong header size.
They should be === to become subsections as they should be.

That's it.
Apart from the minor items listed above, this looks great.
Thank you for taking the time to do this.

(In case anyone is wondering, I am reviewing this pull request because I am one of the change owners for Rpmautospec by Default.)

I’d like the example to still have the same upstream version multiple times, probably mentioning that the release number will be bumped between them.

It makes the table much more awkward. With %autorelease we don't have a fixed string for the dist tag, so it's harder to show. By splitting out the discussion of Version, the whole thing becomes much simpler: this table shows how to handle version, and the user knows that every rebuild gets the Release tag automatically as appropriate. I had to do a bit of mental adjustment to accept this, but this becomes natural once you accept that Version is the only part that the packager needs to think about in normal use.

Do we still have the rule that E-V-R, F(N-1) must be smaller than E-V-R, F(N)? I thought not having that strict requirement was one argument to have the simplified release calculation algorithm.

AFAIK, this requirement hasn't been rescinded.
I don't think this PR is the place to change this, so I just want to update the the existing text for %autorelease but not introduce a changed requirement. I think the current text (suggested by Otto), does a good job of this.


The file is called manual-changelogs, the nav entry says Manual Changelog, but the page title is just Manual Changelog.

I dropped "s" everywhere. I guess either way works.

Updated with the other suggestions.

rebased onto 6ab6ba0447e9b35fd3e66306af9e02e5561ffac1

Any more comments?

My notes for the changes of the Versioning guidelines:

The Epoch tag and the Release tag sections make me expect a Version tag section in the middle. Not a blocker, just a nudge.


Alternatively, the Release: field may be updated manually and contain parts of the upstream version information. See Traditional versioning with part of the upstream version information in the release field.

This makes me think using manual Release without parts of the upstream version is not allowed, which still should be allowed.


Only an old branch needs a change
...
To avoid this situation, rebuild the package also in the later branches,
possibly with just an empty commit to make %autorelease bump the release.

This seems illogical to me. Previously, we said "is a waste of time and energy for the newer branches which do not need to be touched" -- why has this changed?


If the package does not use %autorelease,
you may rebuild just the older branch,
see <<minorbump>>>.

So I am not allowed to do this with autorelease? That seems like a needless limitation to me.

My notes about the changes for the Changelog guidelines:

They must never simply contain an entire copy of the source CHANGELOG entries.

This has been removed and I'd like it to stay. Particularly packit does this and I hate it.


The changes for the Renaming/Replacing or Removing Existing Packages guidelines seem OK.

The Manual Changelog page seems to preserve all relevant information.

rebased onto a935859f496700e4ea35be806bae14c17f343acf

The Epoch tag and the Release tag sections make me expect a Version tag section in the middle. Not a blocker, just a nudge.

I moved the Version following a request from an earlier review (https://pagure.io/packaging-committee/pull-request/1255#comment-184246). With %autorelease, the descriptions of Epoch and Release are very simple, and then there's a lengthy description of Version. I wasn't convinced at first, but the text is easier to read this way.

This makes me think using manual Release without parts of the upstream version is not allowed, which still should be allowed.

I removed that part of the sentence. The other section describes the other approach comprehensively, so it's better not to repeat anything here.

This seems illogical to me. Previously, we said "is a waste of time and energy for the newer branches which do not need to be touched" -- why has this changed?

Minor bumps are not really useful with %autorelease. The very first commit which adds the minor bump would also cause the release number to be bumped, which defeats the purpose. So the guidelines were simplified not to mention this case at all. If you need minor bumps, you're better off with using "Traditional versioning".

So I am not allowed to do this with autorelease? That seems like a needless limitation to me.

Yeah, it just doesn't work.

They must never simply contain an entire copy of the source CHANGELOG entries.

Now it's back. (You'd need to paste the CHANGELOG into the git commit message. Not an obvious thing to do, but I guess it could be done.)


Updated with those changes.

rebased onto b4d762017a289a6dc497a0ce2b0aba20f8315373

I rebased the PR to fix a conflict with #1258.

After talking this over with @churchyard, I added one more patch to remove the subsection "Only an old branch needs a change". See the commit for a longer explanation. In short, we have a disagreement how to handle this case, so let's leave this not documented for now, and work out some solution later.

Just a note that the last time I was in this document, I took great pains to avoid using the word "release" when discussing anything not related to the Release: tag. I thought it best to avoid any possibility of confusion. It's obvious that something was missing from the original, but maybe we could just say "Use the upstream version verbatim ...".

Besides that one minor comment (which I believe applies to one other line in the document) I also want to point out that we have macros NEXTVER, CURRENTVER and PREVVER which we will update occasionally that can be used in example text to refer to a Fedora version without hardcoding something that makes the document look increasingly out of date as time passes. So instead of using "fc20" in various examples, which makes the document seem nearly a decade old, you could use fc{{CURRENTVER}} and then it gets updated whenever we remember to update the macros.

Both of my comments are minor, and something we could easily do post-merge. +1 from me.

Thank you for the review. Updates:

  • "upstream release version" changed to "upstream project version" in the new text
  • fc{CURRENTVER} is now used the versioning examples. I was aware of the macro, but when I generate the page locally, it isn't substituted correctly, and I avoided using it code snippets. But it is used in this way in DistTag page, so I now use the same formatting as there and hope that it'll render correctly.
  • I dropped a change from << >> to xref: that was requested earlier (https://pagure.io/packaging-committee/pull-request/1255#comment-184246). It seems that this is another case that does not render correctly in the local preview, but somehow works in the online build.

… and rebased.

rebased onto 67b6169de975be0fa5a69dcfe4069889bff2259d

Most probably such problems with attribute substitution and << >> cross references can be fixed by moving to use the latest Fedora Docs tooling from Documentation Template.
The tooling used by Packaging Guidelines is either very old, or custom, or both and suffers from already fixed bugs like these.

It's been 6 weeks since the last substantial change. All feedback has been addressed. Miro and Tibbs were +1, and other non-fpc folks too. Are we waiting for something?

Another +1 from me, if it helps.

I think we're probably good to merge this now.

Pull-Request has been merged by ngompa

From the online page:

In this case the full N-V-R could be e.g. pkg-1.1.0.20210105.SP1_CP1-1.fc35.

So I think it all renders as expected.

:100: :tada:

Metadata