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 error messages for url helper calls #3041

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ffesti
Copy link
Contributor

@ffesti ffesti commented Apr 16, 2024

Rpm allows URLs as cli parameters. The files are then automatically
downloaded with %_urlhelper which defaults to curl(1). For far failures
have been ignored right away and error messages are generated later when
the file was not found on disk.

Issue a meaningful error message at least when the help program is
missing. This allows not to ship curl with rpm while still giving the
user a chance to find out what is going on.

Related: rhbz#2216754

Rpm allows URLs as cli parameters. The files are then automatically
downloaded with %_urlhelper which defaults to curl(1). For far failures
have been ignored right away and error messages are generated later when
the file was not found on disk.

Issue a meaningful error message at least when the help program is
missing. This allows not to ship curl with rpm while still giving the
user a chance to find out what is going on.

Related: rhbz#2216754
Give error message if the urlhelper failed (exited with a non zero return
code). This is not quite ideal as the error message from the helper
(typically curl) itself is still not shown but give the user at leat a
hint what is going on.
@pmatilai
Copy link
Member

There should be a test for the case where the urlhelper is missing - it's what we can easily test, and also happens to be the case we're also most interested in for the bug.

$ ./tools/rpm --define "_urlhelper /not/there" -qp https://ftp.funet.fi/pub/mirrors/fedora.redhat.com/pub/fedora/linux/updates/39/Everything/x86_64/Packages/3/389-ds-base-snmp-2.4.5-1.fc39.x86_64.rpm
error: Could not find url helper: "/not/there"
error: open of https://ftp.funet.fi/pub/mirrors/fedora.redhat.com/pub/fedora/linux/updates/39/Everything/x86_64/Packages/3/389-ds-base-snmp-2.4.5-1.fc39.x86_64.rpm failed: No such file or directory

Like I said (elsewhere), I've poked at this thing before. And I remember now, the reason I abandoned it was because I didn't like the "improved" behavior that great either - the latter message is pretty misleading even if the reason is in the line above.

@ffesti
Copy link
Contributor Author

ffesti commented Apr 24, 2024

Yes, this isn't a great solution. But it at least gives the user a fighting chance of figuring out what's happening here. I agree we would want a better solution but there currently just isn't a way to hand over the error code.

Adding a test case is easy if we decide we actually want to go this way.

@pmatilai
Copy link
Member

Hmm, and of course we have entirely different way of reporting the error on install, eg:

[root@localhost _build]# ./tools/rpm --define "_urlhelper /bad" -U --nodeps --root /srv/test https://ftp.funet.fi/pub/mirrors/fedora.redhat.com/pub/fedora/linux/development/rawhide/Everything/x86_64/os/Packages/0/0xFFFF-0.10-7.fc40.x86_64.rpm
error: Could not find url helper: "/bad": No such file or directory
error: skipping https://ftp.funet.fi/pub/mirrors/fedora.redhat.com/pub/fedora/linux/development/rawhide/Everything/x86_64/os/Packages/0/0xFFFF-0.10-7.fc40.x86_64.rpm - transfer failed

Which isn't so bad. But probably --import and all have their own more cryptic variants like the query above. Anyway, if we accept that we call rpmlog() from that location, it is an improvement for the user. Which is what matters in the end I guess.

Besides tests there are some improvements you can make here:
The exec() can fail for other reasons besides not found, so I'd use a more generic error message there. Maybe just "failed to execute url helper". The exact error code is available in the child if we move the log there (ignoring the wrath of rpmlog after fork), or we could extend our use of the bash scheme started in faaa030. From bash(1): "If a command is not found, the child process created to execute it returns a status of 127. If a command is found but is not executable, the return status is 126."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

None yet

2 participants