Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for RPMDBI_BASENAMES on file queries #1755

Merged
merged 2 commits into from Sep 13, 2021

Conversation

dmnks
Copy link
Contributor

@dmnks dmnks commented Aug 16, 2021

  • Add the --allfiles switch which augments -qf
  • Add a basic test for -qf while at it
  • Update the list of possible file states in the man page (needed for consistency with the new --allfiles description)

@pmatilai
Copy link
Member

The patch is straightforward enough as such. This being rpm though... does this end up overriding the install-side --allfiles flag behavior? I seem to recall ambiguous flags in popt needing to be handled through a callback instead because of that, but it's been a while.

I'm also pondering about the placement/naming: due to internal implementation details, this ends up being a query source selector, when from a user POV it's just a modifier flag. --allfiles as a name also seems applicable to a number of other query/verify things, such as to override possible --nofoo flags with --list, but also alias to a theoretical --filestate=any selector that would allow filter by arbitrary file state (we don't have such a thing now of course). None of this is a bad thing, just trying to think about potential future implications a bit.

I'll try to get my pondering to a resolution over lunch 😅

@pmatilai
Copy link
Member

This being rpm though... does this end up overriding the install-side --allfiles flag behavior?

Answering myself, yes it does. Without this patch:

[root@localhost rpm]# ./rpm -e --allfiles telnet
lt-rpm: --allfiles may only be specified during package installation
[root@localhost rpm]#

And with it:

[root@localhost rpm]# ./rpm -e --allfiles telnet
[root@localhost rpm]# 

So this needs to be changed to use a popt callback for --allfiles, I think on both install and query side.

@dmnks
Copy link
Contributor Author

dmnks commented Sep 1, 2021

Eh, you're right. It turns out the handling of --allfiles is consumed entirely by the callback that my patch is adding, making the original install-time option ineffective. Thanks for noticing...

This is because, in the rpmInstallPoptTable, the existing option is using POPT_BIT_SET and that, once parsed, is never passed down to any callback.

The fix is simple: just replace the POPT_BIT_SET with a POPT_ARG_NONE and let the installArgCallback() handle it instead, just like the queryArgCallback() handles the new option. That way, both ia->transFlags and qva->qva_flags are updated accordingly when the option is present on the CLI (they're independent anyway).

That said, I agree with your pondering about the naming. As I'm thinking about this more, I'm not quite convinced that adding such a generically-named option that's only applicable in -qf mode is a good idea, after all. For example, the SYNOPSIS in the man page would look like this:

   query-options
       ...
       Files:  [--allfiles]  [-c,--configfiles]  [-d,--docfiles]  [--dump]  [--fileclass] [--filecolor] [--fileprovide][--filerequire] [--filecaps] [--filesbypkg] [-l,--list] [-s,--state]
       [--noartifact] [--noghost] [--noconfig]
       ...

The proximity to and similarity with --configfiles and --docfiles could be misleading here since their semantics is completely different as they alter the -ql listing output, not package selection. One might conclude that --allfiles overrides those, however, that's not the case. Of course, the details would be explained in the option's description, but seeing this discrepancy in SYNOPSIS raises some red flags for me.

@dmnks dmnks force-pushed the bz1940895 branch 2 times, most recently from 7762dfb to a95afc4 Compare September 7, 2021 09:38
@dmnks
Copy link
Contributor Author

dmnks commented Sep 7, 2021

I've pushed a more straightforward version that adds --path as a more general alternative to --file that, in the same way as the previous --allfiles approach, considers all files in the rpmdb, whether or not they're in fact installed on disk. Let me know your thoughts.

@pmatilai
Copy link
Member

pmatilai commented Sep 9, 2021

At least to me, "file" seems to refer to something concrete, eg on-disk file, whereas a "path" is more abstract so it matches the case quite nicely. I'd simplify the documentation though: update --file to say: "Query package(s) owning installed FILE" (which I should've done when changing the behavior), and then --path can be described as "Query package(s) owning PATH, whether on disk or not". Most people have no clue about rpm's stored file states so eg a reference to some mysterious "netshared" state is more likely to just send them away 😅

@dmnks
Copy link
Contributor Author

dmnks commented Sep 9, 2021

At least to me, "file" seems to refer to something concrete, eg on-disk file, whereas a "path" is more abstract so it matches the case quite nicely.

Yep, that's exactly what I was going for 😉 I'm glad it clicked for you too!

I'd simplify the documentation though: update --file to say: "Query package(s) owning installed FILE" (which I should've done when changing the behavior), and then --path can be described as "Query package(s) owning PATH, whether on disk or not". Most people have no clue about rpm's stored file states so eg a reference to some mysterious "netshared" state is more likely to just send them away sweat_smile

Wow, that's much better. I have to admit, I spent quite a while going through various iterations of this documentation, until it felt just about right and expressed the above difference from --file while also being reasonably succinct. Yet, I've included the file state technicalities in there because:

  • replaced files, while physically present, are regarded as non-installed
  • physically removed files (not by RPM) are regarded as installed

However, now I realize both are corner cases that can be left out for the sake of simplicity. They're not an important distinction for the typical use case here, anyway. Moreover, files removed independently of RPM are basically in the undefined state. There's an implicit assumption in the docs that when we talk about queries, we usually mean RPM database queries, as opposed to file system queries.

In general, basic operations should have short names and simple, ideally one-sentence, descriptions. Anything more than that raises red flags, such as the scope of the switch being too wide.

So, I like your suggestion a lot. I'll update the PR accordingly. Thanks!

@pmatilai
Copy link
Member

pmatilai commented Sep 9, 2021

Oh, indeed. There are a few additional subtleties there as you point out, and my "whether on disk or not" is not quite right. I think I originally wrote that "whether installed or not", which is closer to the point, but then I changed to "on disk" because "installed" may refer to package or a file, and .. meh 😅
To remove that ambiguity one could simply say something like "Query package(s) containing PATH, whether the file is installed or not".

The replaced files case is a bit special, but replacing a file causes effectively an ownership change so "owning the file" kinda takes care of that. It wouldn't hurt to explain it though, if you find a nice way of putting it.

@dmnks
Copy link
Contributor Author

dmnks commented Sep 10, 2021

Oh, indeed. There are a few additional subtleties there as you point out, and my "whether on disk or not" is not quite right. I think I originally wrote that "whether installed or not", which is closer to the point, but then I changed to "on disk" because "installed" may refer to package or a file, and .. meh sweat_smile

Actually, "on disk" would be a good enough abstraction, I think, but let's go with "installed" for another reason, which is that it's an established term used throughout the man page already (i.e. we say "installed" whenever we mean "on disk", but never the latter).

To remove that ambiguity one could simply say something like "Query package(s) containing PATH, whether the file is installed or not".

I think we could even leave out "the file" because we're talking about package queries here and it is assumed that those packages are installed... but yeah, let's not make it overly cryptic either. 😄 (Though, I'll take the liberty for the --help output where every word counts.)

The replaced files case is a bit special, but replacing a file causes effectively an ownership change so "owning the file" kinda takes care of that. It wouldn't hurt to explain it though, if you find a nice way of putting it.

Good point. I'm adding a note there which should make this replacement case obvious, without being overly verbose. It's now two sentences instead of one, which hurts my perfectionist soul, but meh, it explains it well. Let me know otherwise, we can always just drop it 😄

I'm also removing the man page update for --state as it's no longer necessary and would only confuse users in a similar way. The three states listed now (normal, not installed and replaced) are the most common anyway.

Pushing the updated branch next.

This has been missing, so add a simple one.
There are legitimate reasons (such as rhbz#1940895 or the included test)
for wanting the former behavior where all file states were considered in
file queries prior to commit 9ad57bd,
so celebrate the tenth anniversary of that commit by adding a CLI switch
(a new package selector --path), as contemplated back then.

Update the man page for --file to reflect it's current behavior and make
--path that more obvious.

Resolves: rhbz#1940895
Copy link
Member

@pmatilai pmatilai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with me 👍

@pmatilai pmatilai merged commit 949dc7c into rpm-software-management:master Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants