import_srpm: allow to pass a url as source file

[glehmann]

This way we don't even have to manually download the srpm.

[stormi]

@glehmann ready for re-review?

[stormi]

@glehmann you pushed after my last question but didn't answer nor ask for a re-review. Thus the PR stalled. What's the current status?

[glehmann]

The changes were meant to be minimal, hence the lack of temp file removal.
I've updated the PR to properly remove the tempfile, at the cost of an indentation of most of the code

[stormi]

I believe that you could still keep the changes minimal as initially intended by avoiding the use of a context manager. You could use atexit.register(temp_dir.cleanup) to ensure it's cleaned up even in case of a raised exception. Unless I missed something obvious.

Not a blocker comment.

[glehmann]

I think it's cleaner that way, so I'd prefer to keep the context manager.
The diff is hard to read in github, but with better diff viewers, it's easy to see that it's mostly indentation changes :)

[ydirson]

To make such diffs more readable, a good way is to split things in different commits. Here that could be "add a nullcontext ctxmanager" as a first step.

[glehmann]

So because github is bad at showing this kind of diff, I'm supposed to think about how the diff will be displayed in github and split my work in multiple commits? That seems a bit excessive to me.
Also note that the reviewer would still have to actually look at the diff which indents the code, to make sure nothing is changed.

[ydirson]

Note I'm not talking about GH (because we all know how bad they are 😉), but about reviewing with standard git tools.

[ydirson]

To make such diffs more readable, a good way is to split things in different commits. Here that could be "add a nullcontext ctxmanager" as a first step.

[ydirson]

That is an unrelated change that does not belong to this commit. Likely a fix for a real bug, since the original code really looks strange:

    except:
        raise
        parser.error("Git repository seems to have local modifications.")

[ydirson]

This too is a separate fix

[glehmann]

There was even one more fix, which probably also need its own commit:

[ydirson]

otherwise ok

[ydirson]

The non-remote case could possibly be simplified: there seems to be no need to use a context manager at all in that case, if all the actions inside the with block are for the other case:

Suggested change

	with TemporaryDirectory() if is_remote_rpm else nullcontext() as temp_dir:
	if is_remote_rpm:
	# get the src.rpm locally, and continue with the actual file
	local_filename = f'{temp_dir}/{os.path.basename(source_rpm)}'
	urlretrieve(source_rpm, local_filename)
	source_rpm = local_filename
	if is_remote_rpm:
	with TemporaryDirectory() as temp_dir:
	# get the src.rpm locally, and continue with the actual file
	local_filename = f'{temp_dir}/{os.path.basename(source_rpm)}'
	urlretrieve(source_rpm, local_filename)
	source_rpm = local_filename

[glehmann]

I don't think it would work: the TemporaryDirectory context manager would delete the directory when exiting the with block, but we still need what this dir contains later in the code

[ydirson]

mb, looks like I read that with too little context :(