-
Notifications
You must be signed in to change notification settings - Fork 423
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
allow LIEF to fail when using patchelf #5176
Conversation
avoids unconditional failure when LIEF raises, even when not used
CodSpeed Performance ReportMerging #5176 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @minrk , thank you so much for this PR! A few things are requested (or are open for discussion) prior to merge:
-
Is there any way that this code could be re-worked to use
log.warn
/log.error
(withlog = utils.get_logger(__name__)
) for the error handling? I understand that you most likely wanted to remain consistent with the rest of themk_relative_linux
function, but we can take this opportunity to improve error handling patterns (if you feel inclined to do so!) -
It would be great to see a test or two for this change; addtionally it might help to have more details in the PR description for steps on how to reproduce LIEF being unconditionally run even when
method=patchelf
was specified -
Please add a news file with a brief description of this change
I switched it to log and added news. I'm afraid I don't know how to exercise this failure in a test. |
Hm, the modified function is now using two different methods to report errors and warnings to the user. This implies slightly different formats, and also different streams (logging will print to stderr and print to stdout, as currently configured). I'd rather stick to one of the methods so:
|
Per Jaime's comment, I reverted the logging changes; I also went ahead and filed an issue to change all error-reporting to logging. |
print( | ||
f"ERROR :: get_rpaths_raw({elf!r}) with LIEF failed: {e}, but LIEF was specified" | ||
) | ||
traceback.print_tb(e.__traceback__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently this prints to stderr by default. Just mentioning for awareness. I'm not sure if we should print to stdout for consistency or not.
avoids unconditional failure when LIEF raises, even when not used
Description
related to #5175, this allows LIEF to fail just like patchelf.
Previously, even when specifying
method=patchelf
, LIEF was unconditionally run. This adds error handling matching patchelf, so errors in an unused method don't prevent proceeding with patchelf.