Fix #699 Fixes for handling file query arguments in rpm v4.14.0+

[schellj]

No description provided.

[robnagler]

It's always better to use a temporary for this so you don't have to do this ($1).

declare n=$(rpm --query --package --nomanifest "$name")
if [[ ! $n ]]; then
     install_err "not found or invalid rpm=$name (see previous error)"    
fi
name=$n

[schellj]

I'll update.

However, not sure if we want the --nomanifest flag here after all. With it no errors seem to get printed at all, whether the file contains an html (not found) response or a corrupted rpm, etc. So there won't be an error for "see previous error". Without --nomanifest we get the weird errors that the previous code was complaining about, e.g.:

[py3;@... rpmtest]# cat > foo.rpm
<html>not found</html>
[py3;@... rpmtest]# rpm --query --package foo.rpm
error: open of <html>not failed: No such file or directory
error: open of found</html> failed: No such file or directory

[robnagler]

This case is a bug in install_download (or whatever is called). It should check the output for this. install_script_eval validates the result before executing. We should validate the rpm downloaded is an rpm instead of just jumping to installing it.

[schellj]

Not sure if I'm following exactly what you're saying.

Maybe this is what you mean regarding the specific "not found" example in my comment -- I could see install_download verifying the response status, but it doesn't seem like it should be the responsibility of that function to validate the contents, unless we specifically add an install_download_rpm function.

We can validate the rpm downloaded here in rsconf.sh, but the code is already doing that.

[robnagler]

In rsconf_install_file there's code like this:

    install_download "$path" > "$tmp"
    # Unlikely we are downloading HTML so this is a sanity check on SimpleHTTPServer
    # returning something that's a directory listing or not found
    if grep -s -q -i '^<title>' "$tmp"; then
        install_err "$path: unexpected HTML file (directory listing?)"
    fi

This probably should be in install_download. To avoid a broad change, rsconf.sh should have a wrapper that does this. If it really is an html file downloaded, it'll end in "html". Perhaps the above test is too loose and should look for <html> or whatever too. There may be scripts that output HTML, but I don't think that's true in rsconf.

For now just leave it as you have it.

[robnagler]

I had to read why "extant" vs "existing". I believe it's an "overuse" of the word, because it implies something about it having "survived". That's irrelevant to this check, which is really existing. I prefer simpler language in comments except when they are necessary. I'm not sure what "query version" means here. You are querying "$name", not the version so I don't understand that part of the comment. I think the interesting bit is that "--package" is superfluous in 4.14.0+, that is, it always checks for a file on disk.

Go simpler:

# before rpm 4.14 --package was required to validate an rpm file.
# --nomanifest prevents a non-rpm file from being treated as a list of file names.

[schellj]

before rpm 4.14 --package was required to validate an rpm file.

But we're doing this command for two reasons; to validate the rpm file, and to get something that we can use with rpm --query at the end of the function to tell us if the package is installed. So, this comment misses the second part, which isn't obvious (at least to me) and why I said "query version instead" in my comment (though mine didn't talk about the first part).

How about this?

    # --package is required to validate a file before rpm 4.14; after, querying an existing file
    # doesn't tell us if it's installed, so we need to query with the file's package version.
    # --nomanifest keeps a non-rpm from being read as a list of names.

[robnagler]

Now I understand the comment you wrote. :) I think this is better (combining below's code):

declare n=$(rpm --query --package --nomanifest "$name")
if [[ ! $n ]]; then
     install_err "not found or invalid rpm=$name (see previous error)"    
fi
# rpm --query returns version-qualified package, which is a better value to check than the file
name=$n

[robnagler]

This is a lot of code to test something that depends on semantics of actual rpms and the rpm command. This type of testing is not very useful imho. Rather, if you really wanted to stub this out, you could create a github workflow that uses a centos & alma image with an integration test of actually installing or not installing an rpm. I don't think that's very useful either.

[schellj]

Fair enough, but I was trying to validate the changes since I was both modifying the code and there was at least one bug before (i.e., with Centos 7, the existing todo filter in rsconf_yum_install would have never removed an rpm file argument). It seems like there should be a test for this logic, but I can remove it if you don't think it's useful and there isn't a better way.

[robnagler]

These type of stub tests are too specific to a few lines of code and they give a false sense of "working". Yes, they'll test the new code, and they'll be brittle. The number of cases alone was way too long to be useful.

Realistic tests are always better even though they take longer. One option is to create tests that call docker and run inside them with "file:///" urls for the install. These tests would take longer to run, but they are "correct" in that they test the semantics e2e. The tests wouldn't need to run on CI, or these could be special tests that run inside a CI that's got multiple environments configured.

All this is doable, and it takes time.

with Centos 7, the existing todo filter in rsconf_yum_install would have never removed an rpm file argument

Aside: we should probably put those files in install_tmp and then they would get deleted on exit. Not sure why they wouldn't be there now. The removal inline is a good idea to not clutter the machine, and removal at the end is a good backup.

Finally, you could ask copilot to do a review. I wonder if it would have caught this bug.

[robnagler]

In rsconf_install_file there's code like this:

    install_download "$path" > "$tmp"
    # Unlikely we are downloading HTML so this is a sanity check on SimpleHTTPServer
    # returning something that's a directory listing or not found
    if grep -s -q -i '^<title>' "$tmp"; then
        install_err "$path: unexpected HTML file (directory listing?)"
    fi

This probably should be in install_download. To avoid a broad change, rsconf.sh should have a wrapper that does this. If it really is an html file downloaded, it'll end in "html". Perhaps the above test is too loose and should look for <html> or whatever too. There may be scripts that output HTML, but I don't think that's true in rsconf.

For now just leave it as you have it.