PR#658: Avoid calling repo_name from load_nameverrel - rpkg

rpkg

#658 Avoid calling repo_name from load_nameverrel

Merged a year ago by onosek. Opened a year ago by oturpe.

oturpe/rpkg prevent-repo-name-loop into master

Avoid calling repo_name from load_nameverrel

Otto Liljalaakso • a year ago

5c39dcf

pyrpkg/__init__.py

file modified

+2 -3

		`@@ -739,7 +739,7 @@`
		`self._rel = docker_release`
		`return`
		`else:`
		`- raise rpkgError("Dockerfile for {0} not found.".format(self.repo_name))`
		`+ raise rpkgError("Dockerfile not found.")`

		`# Otherwise, we get 'verrel' information from the '.spec' file.`
		`cmd = ['rpm']`
		`@@ -766,8 +766,7 @@`
		`except Exception as e:`
		`self.log.debug('Errors occoured while running following command to get N-V-R-E:')`
		`self.log.debug(joined_cmd)`
		`- raise rpkgError('Could not query n-v-r of %s: %s'`
		`- % (self.repo_name, e))`
		`+ raise rpkgError('Could not query n-v-r: %s' % e)`
		`if err:`
		`self.log.debug('Errors occoured while running following command to get N-V-R-E:')`
		`self.log.debug(joined_cmd)`

oturpe commented a year ago

Method load_repo_name(9 calls load_nameverrel(), which calls
repo_name() to format its error message. That leads to calling
load_repo_name() again, and so on to endless recursion. Prevent this
situation by simplifying load_nameverrel() error messages.

Fixes #657.

Signed-off-by: Otto Liljalaakso otto.liljalaakso@iki.fi

onosek commented a year ago

That leads to calling load_repo_name() again

I tested the case you are talking about. Let's have for example random repository that doesn't contain Dockerfile:
https://src.fedoraproject.org/container/container-engine
fedpkg verrel behaves well here.

The issue with endless recursion could potentially appear when load_repo_name() doesn't find "push_url".
Did you find any specific repo that you could observe it?

However, I am more open to change these messages. Additional information in messages is not a major loss.

oturpe commented a year ago

Apologies, the pull request description is too terse.
A fresh Fedora contributor actually hit the endless recursion when they were trying to follow the Packaging Tutorial from the Package Maintainer Docs.
Unfortunately, due to other, unrelated issues, threading in the devel discussion broke,
so it is very difficult to find the ensuing discussion from the mailing list archives.
Anyhow, the initial message is here:
fedpkg: Failed to get repository name from Git url or pushurl

The recursion happened because two conditions were fulfilled:

Specfile was corrupted and not valid UTF-8, leading to an exception when load_nameverrel() is called.
fedpkg was called outside of Git repository, leading to an alternate code path inside load_repo_name() that calls load_nameverrel() before anything else has a chance.

So if you corrupt a specfile with a hex editor,
then try to do something to it with fedpkg without a Git repository,
you will reproduce #657.

onosek commented a year ago

Thanks for the extended explanation. I didn't look at #657, my bad. Otherwise, I would have had enough information.
But as I indicated in the post above, I will rather lose some details in the message than risk an endless loop (although it is rare).