PR#661: Process source URLs with fragment in pre-push hook - rpkg

rpkg

#661 Process source URLs with fragment in pre-push hook

Merged 2 years ago by onosek. Opened 2 years ago by lsedlar.

lsedlar/rpkg fix-pre-push-check-with-fragment into master

Process source URLs with fragment in pre-push hook

Lubomír Sedlář • 2 years ago

6d813d4

pyrpkg/__init__.py

file modified

+3 -1

		`@@ -4464,7 +4464,9 @@`
		`# find out the format of the source file path. From URL use just the file name.`
		`# We want to keep hierarchy of the files if possible`
		`res = urllib.parse.urlparse(file_location)`
		`- if res.scheme and res.netloc:`
		`+ if res.scheme and res.fragment:`
		`+ source_files.append(os.path.basename(res.fragment))`
		`+ elif res.scheme and res.netloc:`
		`source_files.append(os.path.basename(res.path))`
		`else:`
		`source_files.append(file_location)`

lsedlar commented 2 years ago

Some download services do not have the actual filename in the URL. Packagers work around that by adding a fragment to the URL. This is then ignored by any server, but tricks RPM into getting the correct filename.

Example:

Source0: https://crates.io/api/v1/crates/actix/0.13.0/download#/actix-0.13.0.crate

The filename is obviously actix-0.13.0.crate, but rpkg without this patch will come up with download.

lsedlar commented 2 years ago

CC @decathorpe who reported this in https://bodhi.fedoraproject.org/updates/FEDORA-2023-e7db0e991f

Ideally rpkg would use the same method that RPM does. I suspect rpm simply splits the string on rightmost /, but I haven't actually found it in their source code yet.

Relevant part of packaging guidelines: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_troublesome_urls

decathorpe commented 2 years ago

I don't know how RPM itself handles this, but the code in spectool that handles this is here: https://pagure.io/rpmdevtools/blob/main/f/rpmdev-spectool#_348-357

onosek commented on line 7 of pyrpkg/__init__.py 2 years ago

needs "elif" here. We don't want both names from the URL in the list.

onosek commented 2 years ago

Thanks @lsedlar. I tested the change (with the "elif" fix) on some repos including fdupes and urlwatch which contain URLs with fragment part.

@decathorpe I can (and I will) build a new rpkg package including the patch for the fc37.

@lsedlar, @decathorpe Does it worth (just thinking, not sure) to rebuild (& reset) other updates in the Bodhi? Or would it be OK to patch it afterwards the current updates are sent to stable? There is an argument for skipping the failed check --no-verify that is suggested when the check fails.

decathorpe commented 2 years ago

I'm ok if you push the fix as a follow-up update.

The --no-verify CLI argument isn't really an option for me at least, because I'd need to change all my package maintenance helper scripts for this ...

rebased onto 6d813d4

2 years ago