Add support for openSUSE style detached changelogs

[dcermak]

This PR adds some hacky support for detached/openSUSE style changes files that are inserted into the %changelog section by the open build service.

The tests are not yet all passing, as there is a discrepancy between str(DetachedChangelog) and respective section in str(spec). I'm not sure if this is caused by my (probably rather odd) approach here or whether this is even required?

TODO:

• Write new tests or update the old ones to cover new functionality.
• Update doc-strings where appropriate.
• Update or write new documentation in packit/packit.dev.

Fixes

Related to packit/packit#2399

Merge before/after

RELEASE NOTES BEGIN

Add support for detached (open)SUSE style changelogs

RELEASE NOTES END

[softwarefactory-project-zuul]

Build failed.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/8d0b9a3aaf1b4b2f89c0782e3e038128

✔️ pre-commit SUCCESS in 1m 50s
❌ specfile-tests-rpm-deps FAILURE in 1m 03s
❌ specfile-tests-pip-deps FAILURE in 59s

[nforro]

I appreciate the effort, but I don't think this should be part of Specfile - it represents a single *.spec file, it shouldn't read or modify any other files. Also, I don't understand why there is a need for OpenSUSE-specific classes. If OpenSUSE changelog entries are parseable by RPM, ChangelogEntry should be able to parse/construct them without issues.

[dcermak]

I appreciate the effort, but I don't think this should be part of Specfile - it represents a single *.spec file, it shouldn't read or modify any other files.

Yes, I understand that, the problem is that in openSUSE land it has been the convention for ever and ever to have the changelog in a secondary file and not be it part of the *.spec file. That in itself wouldn't be a problem, but since we would like to extend packit to also handle openSUSE, we need to be able to append to the changelog. Ideally, the API should be the same for all distros as much as possible, so that we don't have to have multiple code paths.

Also, I don't understand why there is a need for OpenSUSE-specific classes. If OpenSUSE changelog entries are parseable by RPM, ChangelogEntry should be able to parse/construct them without issues.

Yes, the openSUSE changes files follow a different format. A typical changes file entry looks like this:

-------------------------------------------------------------------
Wed Dec 18 10:49:41 UTC 2024 - Dan Čermák <[email protected]>

- Enable tests

In contrast to the typical rpm changelog, it always contains a time in UTC, but lacks the EVR (for practical reasons as the build service auto-rebuilds bumping the release).

The build service or obs-build or rpmbuild add the changes file into the built rpm, but that happens on a binary rpm level. This doesn't really help in the context of packit.

[nforro]

Yes, I understand that, the problem is that in openSUSE land it has been the convention for ever and ever to have the changelog in a secondary file and not be it part of the *.spec file. That in itself wouldn't be a problem, but since we would like to extend packit to also handle openSUSE, we need to be able to append to the changelog. Ideally, the API should be the same for all distros as much as possible, so that we don't have to have multiple code paths.

I would argue it should be part of packit codebase, there is literally a class called ChangelogHelper.

Yes, the openSUSE changes files follow a different format. A typical changes file entry looks like this:

-------------------------------------------------------------------
Wed Dec 18 10:49:41 UTC 2024 - Dan Čermák <[email protected]>

- Enable tests

I see.

In contrast to the typical rpm changelog, it always contains a time in UTC, but lacks the EVR (for practical reasons as the build service auto-rebuilds bumping the release).

That shouldn't be a problem, even some Fedora/RHEL spec files don't have EVR in changelog entries. AFAICT the only difference is the line of dashes instead of an asterisk denoting the start of an entry. IMHO it would make sense to add support for that into ChangelogEntry.assemble() and Changelog.parse() and keep the rest as it is.

[nforro]

To be a bit more specific, Changelog.parse() would be able to parse both formats and corresponding ChangelogEntry.header attributes would start either with * or with -------------------------------------------------------------------\n, and ChangelogEntry.assemble() would gain a new argument to choose the correct "starting sequence".

[dcermak]

Ok, I'll see how far I get then.

How would you recommend to handle the parsing/creation of *.changes files? Because if we read a specfile from OBS using the specfile library, it'll have an empty %changelog. And anything that packit creates, will have the changelog in the spec and thereby get auto-rejected for inclusion in the openSUSE distributions.

[nforro]

How would you recommend to handle the parsing/creation of *.changes files? Because if we read a specfile from OBS using the specfile library, it'll have an empty %changelog. And anything that packit creates, will have the changelog in the spec and thereby get auto-rejected for inclusion in the openSUSE distributions.

Well, packit should do something similar to what Specfile.add_changelog_entry() does, except it won't be operating on *spec but on *.changes (although there won't be much left from the original implementation apart from ChangelogEntry.assemble()).

specfile/specfile/specfile.py

Lines 479 to 551 in c0a98a8

	def add_changelog_entry(
	self,
	entry: Union[str, List[str]],
	author: Optional[str] = None,
	email: Optional[str] = None,
	timestamp: Optional[Union[datetime.date, datetime.datetime]] = None,
	evr: Optional[str] = None,
	) -> None:
	"""
	Adds a new _%changelog_ entry. Does nothing if there is no _%changelog_ section
	or if _%autochangelog_ is being used.
	If not specified, author and e-mail will be automatically determined, if possible.
	Timestamp, if not set, will be set to current time (in local timezone).
	Args:
	entry: Entry text or list of entry lines.
	author: Author of the entry.
	email: E-mail of the author.
	timestamp: Timestamp of the entry.
	Supply `datetime` rather than `date` for extended format.
	evr: Override the EVR part of the changelog entry.
	Macros will be expanded automatically. By default, the function
	determines the appropriate value based on the spec file current
	_%{epoch}_, _%{version}_, and _%{release}_ values.
	"""
	with self.sections() as sections:
	# there could be multiple changelog sections, update all of them
	for section in sections:
	if not section.normalized_id == "changelog":
	continue
	if self.contains_autochangelog(section):
	continue
	if evr is None:
	evr = "%{?epoch:%{epoch}:}%{version}-%{release}"
	with self.changelog(section) as changelog:
	if changelog is None:
	return
	evr = self.expand(evr, extra_macros=[("dist", "")])
	if isinstance(entry, str):
	entry = [entry]
	if timestamp is None:
	# honor the timestamp format, but default to date-only
	if changelog and changelog[-1].extended_timestamp:
	timestamp = datetime.datetime.now().astimezone()
	else:
	timestamp = datetime.datetime.now(
	datetime.timezone.utc
	).date()
	if author is None:
	author = guess_packager()
	if not author:
	raise SpecfileException("Failed to auto-detect author")
	elif email is not None:
	author += f" <{email}>"
	if changelog:
	# try to preserve padding of day of month
	padding = max(
	(e.day_of_month_padding for e in reversed(changelog)),
	key=len,
	)
	else:
	padding = "0"
	changelog.append(
	ChangelogEntry.assemble(
	timestamp,
	author,
	entry,
	evr,
	day_of_month_padding=padding,
	append_newline=bool(changelog),
	)
	)

I mean, we could also add an argument to Specfile.add_changelog_entry() specifying path to a changelog file and then the method would write the entry to that file instead of the spec file itself, but I'm still not convinced such code belongs here.

[nforro]

Fedora packages can use changelog files too BTW (example), but they exist for preserving changelog history and are not meant to be updated.

[dcermak]

@nforro I have reworked the PR, what do you think about this approach? I hope it's as noninvasive as possible

[softwarefactory-project-zuul]

Build succeeded.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/7484d1cfc31248658fde5aa26a7e546f

✔️ pre-commit SUCCESS in 1m 49s
✔️ specfile-tests-rpm-deps SUCCESS in 59s
✔️ specfile-tests-pip-deps SUCCESS in 56s

[nforro]

Thanks! LGTM.
Nice to have would be a property of ChangelogEntry that would return ChangelogStyle.

[softwarefactory-project-zuul]

Build succeeded.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/035143ca19de417cb4093c2460cdb05f

✔️ pre-commit SUCCESS in 1m 50s
✔️ specfile-tests-rpm-deps SUCCESS in 1m 02s
✔️ specfile-tests-pip-deps SUCCESS in 58s

[softwarefactory-project-zuul]

Build succeeded.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/8f10a342282f4bf7bd87431b6ccd238b

✔️ pre-commit SUCCESS in 1m 53s
✔️ specfile-tests-rpm-deps SUCCESS in 1m 02s
✔️ specfile-tests-pip-deps SUCCESS in 59s

[nforro]

Just wondering, is rpm in openSUSE patched to be able to parse the changelogs (i.e. does something like cat *.spec *.changes | rpmspec --parse /dev/stdin work)?

[dcermak]

Just wondering, is rpm in openSUSE patched to be able to parse the changelogs (i.e. does something like cat *.spec *.changes | rpmspec --parse /dev/stdin work)?

Nope 😬 :

# cat container-snap.changes | rpmspec --parse /dev/stdin
error: line 1: Unknown tag: -------------------------------------------------------------------

[softwarefactory-project-zuul]

Build succeeded.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/4283f4bbbd7e457582cb12dfaf02577f

✔️ pre-commit SUCCESS in 1m 51s
✔️ specfile-tests-rpm-deps SUCCESS in 1m 01s
✔️ specfile-tests-pip-deps SUCCESS in 1m 02s

[nforro]

# cat container-snap.changes | rpmspec --parse /dev/stdin
error: line 1: Unknown tag: -------------------------------------------------------------------

I meant when you (con)cat a specfile (without changelog) with the corresponding changes file (and add %changelog inbetween if it's missing entirely). I wouldn't expect a sole .changes file to be parseable.

[dcermak]

# cat container-snap.changes | rpmspec --parse /dev/stdin
error: line 1: Unknown tag: -------------------------------------------------------------------

I meant when you (con)cat a specfile (without changelog) with the corresponding changes file (and add %changelog inbetween if it's missing entirely). I wouldn't expect a sole .changes file to be parseable.

Oh, dang. That's what you meant. So rpmspec appears to parse it (it returns 0), but it throws a warning:

# cat /src/container-snap.spec /src/container-snap.changes | rpmspec --parse /dev/stdin > /dev/null
error: %changelog entries must start with *

Afaik there is some logic in the build service that masages the changes files back into the binary rpms

[nforro]

Afaik there is some logic in the build service that masages the changes files back into the binary rpms

Right, that's what I thought. Thanks for the investigation and for the PR 🙂

[softwarefactory-project-zuul]

Build succeeded (gate pipeline).
https://softwarefactory-project.io/zuul/t/packit-service/buildset/b6e9677d2f4a4380ad70d5785317a0b6

✔️ pre-commit SUCCESS in 1m 50s