To be honest, I don't see how does this fix #9
Also, I am not really sure what is the difference between the proposed --get-rpm vs the combination of --dry-run and --verbose.

Hmm, maybe I can answer both of my questions. You are probably planning to dump the list of packages using --get-rpm and then pass it to the createrepo --exclude parameter after --recycle-pkglist, right?

I'd suggest using name like --dump-files-to-remove. And even better, it would be nice
if we could wrap that as library call, instead of playing with stdout from prunerepo.

Something like:

def run_prunerepo(directory, delete_files=False, ...):
    """
    [document here what it does].
    Returns a list of (s)RPM path names that should be removed.
    """

I'd suggest using name like --dump-files-to-remove. And even better, it would be nice
if we could wrap that as library call, instead of playing with stdout from prunerepo.

Something like:
```
def run_prunerepo(directory, delete_files=False, ...):
"""
[document here what it does].

Returns a list of (s)RPM path names that should be removed.
"""
```

even so, I have to change the output to stdout, otherwise, the log information will be printed as well as what command is currently running and we want to have only a list of rpm packages on stdout

Hmm, maybe I can answer both of my questions. You are probably planning to dump the list of packages using --get-rpm and then pass it to the createrepo --exclude parameter after --recycle-pkglist, right?

yes

even so, I have to change the output to stdout, otherwise, the log information will be printed as well as what command is currently running

Consider a library call, not an optional output from /bin/prunerepo command ... then it
doesn't matter what you print to stdout/stderr? Can you elaborate?

even so, I have to change the output to stdout, otherwise, the log information will be printed as well as what command is currently running

Consider a library call, not an optional output from /bin/prunerepo command ... then it
doesn't matter what you print to stdout/stderr? Can you elaborate?

I'm sorry, you're right.

I moved the code from prunerepo to rpm_list.py, the file can be renamed. I still have to fix tests that are not currently running due to my changes.

We can remove this line :-)

[copr-build]

I still have to fix tests

Yeah, thank you. I enabled CI so it should be obvious once it is fixed.

I fixed the tests I broke so Copr build succeeded.
PTAL

The name of the file rpm_list.py doesn't really correspond to what's inside.

The name of the file rpm_list.py doesn't really correspond to what's inside.

What about helpers.py?

What about helpers.py?

Sounds good to me

Updated

Ad helpers.py - speaking of Python API, it would be very convenient to do just
from prunerepo import get_rpms_to_prune. Ie. I'd normally move the code to
prunerepo/__init__.py.

It's a pity that the new method get_rpm_list isn't used by /bin/prunerepo itself -
it basically leaves it untested.

Ad helpers.py - speaking of Python API, it would be very convenient to do just
from prunerepo import get_rpms_to_prune. Ie. I'd normally move the code to
prunerepo/init.py.

I agree with the premise, but I wouldn't move the code there but rather just import it in the __init__.py file. See how python/copr/v3/__init__.py looks like.

export PYTHONPATH=pwd ??

PTAL, I'll use code from https://github.com/praiskup/dnf-hacks/blob/main/find-srpm-to-rpm-pairs.py in the next PR.

Overall, the PR looks really good. Just a few thoughts.

• For each function, it differs in which order dry_run and verbose parameters are defined and whether they are optional.
• We have verbose, silent, and quiet. I guess at least one of them is redundant (the newly added here is silent) and can be replaced with one of the already existing names.
• I think we can drop the __all__ variable. The code should IMHO work even without it and it should be useful only for from prunerepo import * which nobody should really do. Or if we want to keep it, I would suggest limiting it to only a subset of functions that should be an API of some sorts. In this case probably just get_rpm_list. I know the variable is defined in the code I linked, so sorry about that.
• As I understand it, the @praiskup's note "It's a pity that the new method get_rpm_list isn't used by /bin/prunerepo itself" is resolved and we use the get_rpm_list within the prunerepo itself? That's great news.

This is really haaard to reviev.... And it is nothing new to say that prunerepo is really delicate part of our code - doing something wrong here may cause a lot of damage :-).

Lemme suggest this... Instead of moving all the code from A to B, and then to C (helpers.py) let's move everything to helpers in the first commit. This should protect the original git blame ownership. Then, in another commit, move only the ArgumentParser jobs to main.py, and do the rest of the changes in e.g. third commit.

@praiskup @frostyx PTAL, I got rid of the silent variable and rearranged the commits.

Overall, the PR looks really good. Just a few thoughts.

Thank you for fixing those

I think that @praiskup's last comment was meant differently though.

Can't we just import the get_rpm_list here to not expose anything else?

This is now much better, thank you!

I'm now not sure if I like the log_info removal. But looking at the help output

  --verbose           print all deleted items to stdout
  --quiet             do not print any info messages, just do your job

I'm a bit disappointed by the "stdout" note. It would be much easier if it went
to stderr ... so we could migrate everything to python logging.

This if verbose condition is new... before this was printed unconditionally.

Since this is becoming an API, can you please carefully document all the arguments? When I see it now from a different angle, I'd probably even consider a bit more obvious method name like, get_rpms_to_remove() or something alike?

Seems like the dry_run is redundant, as method get_* shouldn't ever remove any files...?

Thinking again .. what if we indeed changed the log_info into 'log.info()' and 'log.debug()' right away, and mentioned that we log to stderr instead of stdout?

From what I can tell, the stdout output isn't meant to be parseable. And the --verbose option would just set logging level to logging.DEBUG, and --quiet to logging.ERROR. The default would be logging.INFO.

The remaining glitch is that logging behaves weird in dnf. Perhaps we should
use a custom logger instead (log = logging.getLogger(...)), ...?

In [4]: get_rpms_to_remove("/tmp/repo")                                                                                                                                                                                                     
2021-04-07 12:20:44,641 [ERROR] dnf:794857:MainThread @logutil.py:200 - [Errno 13] Permission denied: '/var/log/rhsm/rhsm.log' - Further logging output will be written to stderr
2021-04-07 12:20:44,695 [WARNING] dnf:794857:MainThread @logutil.py:154 - logging already initialized
2021-04-07 12:20:44,696 [ERROR] dnf:794857:MainThread @identity.py:156 - Reload of consumer identity cert /etc/pki/consumer/cert.pem raised an exception with msg: [Errno 13] Permission denied: '/etc/pki/consumer/key.pem'
2021-04-07 12:20:45,829 [ERROR] dnf:794858:MainThread @logutil.py:200 - [Errno 13] Permission denied: '/var/log/rhsm/rhsm.log' - Further logging output will be written to stderr
2021-04-07 12:20:45,886 [WARNING] dnf:794858:MainThread @logutil.py:154 - logging already initialized
2021-04-07 12:20:45,887 [ERROR] dnf:794858:MainThread @identity.py:156 - Reload of consumer identity cert /etc/pki/consumer/cert.pem raised an exception with msg: [Errno 13] Permission denied: '/etc/pki/consumer/key.pem'
2021-04-07 12:20:47,046 [ERROR] dnf:794860:MainThread @logutil.py:200 - [Errno 13] Permission denied: '/var/log/rhsm/rhsm.log' - Further logging output will be written to stderr
2021-04-07 12:20:47,106 [WARNING] dnf:794860:MainThread @logutil.py:154 - logging already initialized
2021-04-07 12:20:47,107 [ERROR] dnf:794860:MainThread @identity.py:156 - Reload of consumer identity cert /etc/pki/consumer/cert.pem raised an exception with msg: [Errno 13] Permission denied: '/etc/pki/consumer/key.pem'
Out[4]: ['/tmp/repo/dummy-pkg-20210407_1216-1.fc34.x86_64.rpm']

stderr, and manual page probably deserves an update, too

By default the info messages are not printed, I tried just run prunerepo . and no INFO message was printed.

Nit: The "Fixes" tag in the commit message is missing.

Thanks for the update btw, I really can't wait for the PR against copr using this! (no need to wait till this is merged)

s/stdout/stderr/

the change to additionaly isn't correct spelling

s/stdout/stderr/

Please document what the special case (default) 0 means.

Would you mind merging those two ^^ into a one argument, like log_level="info"? Btw., it is weird that this method actually defaults to info but no info message is ever logged ... perhaps we could at least log something like Checking "%s" directory for removal candidates, older than %s days?

I reported this: https://bugzilla.redhat.com/show_bug.cgi?id=1947844

PTAL, logging fixed :)

Remaining thing, the logs are duplicated:

$ prunerepo . 
Checking '/tmp/repo' directory for removal candidates older than 0 days
Checking '/tmp/repo' directory for removal candidates older than 0 days

$ prunerepo . |& grep -v 2021
Checking '/tmp/repo' directory for removal candidates older than 0 days
Checking '/tmp/repo' directory

The second one sounds useless, though I think that "chacking age of the {}" would be an useful debug message here.

This, though, could be an info message.

This was debug message before, and it was correct. Sounds like a good idea to inform user by default what files are removed, but not about all the checked files. The above log.info is IMO OK, it is logged only once per whole method run.

Except for one nit, looks fine. @frostyx, do you want to have a final look?

+1

This PR has been migrated to GitHub as a placeholder issue:
https://github.com/fedora-copr/prunerepo/issues/10