To express my opinion on several discussion points:
1. I'd suggest to refactor main() to split it and create 3 helper function. Once per "mode", list_file, get_files, verify_checksum. This should be a separate PR. The question for me is: refactor first or after?
2. Previous point should also help in addressing discussion point (3). Code reuse is not easy atm.
3. I'm not sure if it's possible to auto-discover sources files for a given spec file. If possible, I'd suggest to do so by default.

I don't have strong opinions for other topics. Just need your feedback.

The reason why the implementation of spectool is kind of weird is that it was a 1:1 port of an old Perl script, and we needed to keep the CLI and behaviour compatible.

However, I'm not even sure if spectool is the correct place to put these checks. The proposed workflow looks a bit weird to me: A packager who first updates a package to a new version would download files with spectool -g, at which point there is no prior checksum to compare to.

So is the workflow you suggest above meant to happen when somebody that is not the initial packager re-downloads the sources from upstream, and compares the checksum of the fresh downloads with those from the lookaside cache? This check seems to me to be completely independent of the usual packager workflow, so I don't think putting it into spectool is the right choice.

If you want to have a tool to re-download source files and compare them against the contents of the lookaside cache, I'd prefer that to be a separate tool instead of making spectool more complicated by adding even more unrelated functionality to it.

Yes, the idea is that someone would run this (likely as part of a batch process) to validate that sources actually match the checksums. We originally thought of implementing this is a standalone tool, but @ikruglov noticed early on that spectool already had most of the logic required, so it seemed logical to add this here instead of duplicating functionality. I suppose another option could be to factor the logic out in a library (rpkg? a new one?) and make both the standalone tool and spectool use it, but it seemed a bit overkill for this.

Right, I assume you refer to the spec parsing / download part that is already implemented in spectool?

One potential problem I see is that previously we had pretty strong push-back on adding new dependencies to rpmdevtools / spectool because they were not yet in RHEL. I think they're even patching out the progressbar dependency ...

Not sure if pyrpkg is in RHEL?

EDIT: Not sure if it's even worth it to add a pretty heavy dependency just for parsing a very simple plain-text format (the sources file).

Ah, good catch. rpkg is not in CentOS Stream, so it's definitely not in RHEL either. It is branched for EPEL though: https://src.fedoraproject.org/rpms/rpkg

Well ... rpmdevtools is in RHEL proper, not in EPEL - so spectool needs to work (without causing too many headaches) in RHEL proper, too.

Hi.

Right, I assume you refer to the spec parsing / download part that is already implemented in spectool?

Yes. spectool already does many things I need.

One potential problem I see is that previously we had pretty strong push-back on adding new dependencies to rpmdevtools

Dependency on rpkg is not a major issue. Parsing sources files is not a complex task.

I think the key question what is expected workflow and whether spectool --verify-checksum fits into it. @dcavalca, I need to rely on your arguments here since I'm pretty new to the field.

@zbyszek should weigh in as well, but IMO the expected workflow here is that a script will loop over every package in dist-git and run spectool --get-files followed by spectool --verify-checksum. We'd record the results somewhere in a useful format, and packagers can run spectool --verify-checksum themselves to validate them (and then hopefully fix their packages).

I agree that we should drop the pyrpkg dependency here.

Some comments on the code:

• "-vc",

A multi-char short option is very strange. Please don't do this.

• if args["get_files"] or args["verify_checksum"]:

Hmm, this is strange. Is args an argparse.Namespace object? Maybe before adding such ugly code, first refactor it to use the usual args.foo notation.

• headers = {}
• for header in args["headers"]:
• k, sep, v = header.partition(':')
• headers[k.strip()] = v.strip()

k, v = header.split(':', maxsplit=1)

• print("RPM Failed to parse 'sources' file.")
• return 1

This prints the error message to stdout (instead of stderr). Sentence is miscapitalized.
sys.exit("rpm failed to parse 'sources' file.")

with open(path, 'rb', buffering=0) as f:

The context manager is completely unnecessary for files opened for reading.
Also why disable buffering?

+ except e:

Bare except should never be used, except in very special circumstances (interactive interpreters, etc). It suppresses exceptions other than Exception, e.g. ^C.

Please don't use .format(). We have f-strings now.

I think that adding this in spectool is reasonable. It let's the existing functionality that people already use to download files be reused, and the amount of new code that is needed is minimal. And since people use spectool to download files, by using the same download mechanism we reduce the chance of a spurious failure.

OTOH, I think that the current way to invoke the test is very cumbersome. (As are all spectool calls in general… sadly.) Please note that some (many?) dist-git spec files cannot be parsed by spectool without --define '_sourcedir .'.

One usability improvement would be to open a temporary directory, download the files into there, and if the verification succeeds, remove the directory. If it fails, print the directory name to let the user diagnose the failure. This way, the check can be done with less preparation.

Ideally, I think we'd want to have this integrated in fepkg / rpkg, so that we can say fedpkg verify-sources, and that'd do the call to spectool --define '_sourcedir .' $(basename $PWD) --verify-checksum -C $tmpdir.

FYI, on my backlog, there is the task of making a better spectool, this time without keeping backwards compatibility with the old Perl script, and making the executable name != spectool (probably making it available in parallel so things can migrate to the new thing at their own pace).

That would allow to drop all the old cruft and make the CLI less weird and more flexible, and also make it usable as a library that could be integrated into fedpkg. Integrating this verification functionality there would be even better, but since the rewrite obviously doesn't exist yet, that's hard to do :)

That sounds nice. If we do a rework, I think this should be part of fedpkg/rpkg. It's much easier to have on tool to interact with the package than 20 different tools, each with an unpredictable name, strange semantics and idiosyncrasies.

Hi @zbyszek. Sorry for long reply. I have been on an extended sick leave. I should be able to reply faster from now on.

Please find my comment bellow:

Some comments on the code:

• "-vc",

Np. I'll change it.

A multi-char short option is very strange. Please don't do this.

• if args["get_files"] or args["verify_checksum"]:

Hmm, this is strange. Is args an argparse.Namespace object? Maybe before adding such ugly code, first refactor it to use the usual args.foo notation.

I used args[] notation because the rest of the code does the same. I'm happy to refactor but I think it make sense to either keep the current style, or refactor them all.

• headers = {}
• for header in args["headers"]:
• k, sep, v = header.partition(':')
• headers[k.strip()] = v.strip()

k, v = header.split(':', maxsplit=1)

This is a piece of code which I just moved around. Happy to refactor though if you want.

• print("RPM Failed to parse 'sources' file.")
• return 1

This prints the error message to stdout (instead of stderr). Sentence is miscapitalized.
sys.exit("rpm failed to parse 'sources' file.")

Yes, I'm aware that printing error messages to stdout is not a best practice. However, there multiple cases in the existing code which does it (ex: 1, 2). So, I can change this. However, I'm concerned that this can lead to unexpected behaviour for users where they expect errors on stdout.

with open(path, 'rb', buffering=0) as f:

The context manager is completely unnecessary for files opened for reading.
Also why disable buffering?

No particular reason. I'll fix it.

+ except e:

Bare except should never be used, except in very special circumstances (interactive interpreters, etc). It suppresses exceptions other than Exception, e.g. ^C.

Will fix it as well.

Please don't use .format(). We have f-strings now.

Once again. The existing code uses .format(). It doesn't use f"" strings.

Overall. I agree with all of your best practices recommendation. However, it seems that this code follows non/few of them. So, I'd suggest keeping existing style to simplify future refactor. Or, I can refactor it all but it seems that there are bigger plans for it.

Yeah, the Python rewrite was imported to replace the Perl spectool over 4 years ago, and the script already wasn't new back when it was added. So obviously it does not follow the latest Python best practices :(

There is also an RPM upstream ticket asking about supporting checksums in the spec file: https://github.com/rpm-software-management/rpm/issues/463