pypa / abi3audit

Scans Python packages for abi3 violations and inconsistencies
https://pypi.org/project/abi3audit
MIT License
100 stars 9 forks source link

Add --ignore CLI option to ignore certain symbol patterns #111

Open giampaolo opened 1 week ago

giampaolo commented 1 week ago

Hello! Since a recent upgrade of abi3audit, my psutil CI builds started failing because long time ago I defined some custom Py* functions which I use pretty much all over the C extension code (>200 occurrences). My take from this is that abi3audit does not recognize them as known cPython API symbols due to the fact that I used a Py* prefix. Would it be possible to add an --ignore CLI parameter in order to ignore such occurrences? E.g. in my case python3 -m abi3audit --ignore=PyInit__psutil_posix --ignore=PyErr_SetFromOSErrnoWithSyscall ... would serve this purpose.

Thanks in advance

$ python3 -m abi3audit --verbose  dist/*-abi3-*.whl
[10:26:14] πŸ‘Ž psutil-6.0.1-cp310-abi3-linux_x86_64.whl: _psutil_linux.abi3.so has non-ABI3 symbols                                              
           ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓                                                                                                  
           ┃ Symbol                          ┃                                                                                                  
           ┑━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┩                                                                                                  
           β”‚ PyInit__psutil_posix            β”‚                                                                                                  
           β”‚ PyErr_SetFromOSErrnoWithSyscall β”‚                                                                                                  
           β””β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”˜                                                                                                  
           πŸ‘Ž psutil-6.0.1-cp310-abi3-linux_x86_64.whl: _psutil_posix.abi3.so has non-ABI3 symbols                                              
           ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓                                                                                                  
           ┃ Symbol                          ┃                                                                                                  
           ┑━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┩                                                                                                  
           β”‚ PyErr_SetFromOSErrnoWithSyscall β”‚                                                                                                  
           β””β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”˜                                                                                                  
           πŸ’ psutil-6.0.1-cp310-abi3-linux_x86_64.whl: 2 extensions scanned; 0 ABI version mismatches and 3 ABI violations found               
woodruffw commented 1 week ago

Hey @giampaolo, thanks for the feature request!

I've got mixed feelings on this: on one hand these are false positives from a stable ABI perspective, but on the other they're strongly discouraged by CPython's API:

Note: User code should never define names that begin with Py or _Py. This confuses the reader, and jeopardizes the portability of the user code to future Python versions, which may define additional names beginning with one of these prefixes.

(link: https://docs.python.org/3/c-api/intro.html#include-files)

TL;DR: I think I could be convinced here, but only if it'd be a significant refactoring challenge for psutil to align with the CPython API guidelines here, since I'm worried that others will use it to suppress "works on my machine" type ABI errors πŸ™‚

giampaolo commented 1 week ago

TL;DR: I think I could be convinced here, but only if it'd be a significant refactoring challenge for psutil to align with the CPython API guidelines here

Thank you, I appreciate it, but the refactoring is not hard per-se (it's just a mass rename). And I definitively wouldn't ask you to put the burden on your tool just because doing the refactoring on my tool is hard (it's not). =) Adding --ignore should be justified by a realistic use case, but honestly I can't think of any other than mine. The doc extract you quoted makes sense, so I guess I will just go ahead and rename these functions, even though I'll have to come up with a good name, which I don't have at the moment. :)

woodruffw commented 1 week ago

Sounds good! I'll leave this open in case others have other use cases where an --ignore or similar flag makes sense. Thanks for your diligence here!

giampaolo commented 1 week ago

Sorry, I still could use your help. I renamed PyErr_SetFromOSErrnoWithSyscall which fixes 1 warning, but I still get this one re. to PyInit__psutil_posix:

           ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓                                                                                                  
           ┃ Symbol                          ┃                                                                                                  
           ┑━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┩                                                                                                  
           β”‚ PyInit__psutil_posix            β”‚                                                                                                                 
           β””β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”˜      

Note: psutil on Linux has 2 extension modules, _psutil_linux and _psutil_posix:

I'm not sure why abi3audit complains about _psutil_posix but not _psutil_linux. Any idea? If it can help, full diff is here.

woodruffw commented 1 week ago

Hmm, random guess, but is PyInit__psutil_posix present in extensions outside of _psutil_posix.so?

abi3audit allows PyInit_ symbols as long as the match the extension object's name (e.g. _foo.so becomes PyInit__foo.so), but rejects any other PyInit_ symbols. So PyInit__psutil_posix would cause errors if it's present in anything other than _psutil_posix.so.

woodruffw commented 1 week ago

Ah yeah, based on your CI you have PyInit__psutil_posix inside of _psutil_linux.so:

           psutil-6.0.1-cp36-abi3-manylinux_2_17_aarch64.manylinux2014_aarch64.w
           hl: _psutil_linux.abi3.so has non-ABI3 symbols                       
           ┏━━━━━━━━━━━━━━━━━━━━━━┓                                             
           ┃ Symbol               ┃                                             
           ┑━━━━━━━━━━━━━━━━━━━━━━┩                                             
           β”‚ PyInit__psutil_posix β”‚                                             
           β””β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”˜  

This is a funny edge case πŸ™‚ -- I'll have to see what the C ABI reference says about this! It's possible abi3audit is being too conservative about how it checks PyInit, or this may be a slight violation of the extension requirements.

woodruffw commented 1 week ago

From https://docs.python.org/3/extending/building.html#c.PyInit_modulename:

It is possible to export multiple modules from a single shared library by defining multiple initialization functions. However, importing them requires using symbolic links or a custom importer, because by default only the function corresponding to the filename is found. See the β€œMultiple modules in one library” section in PEP 489 for details.

My interpretation of this is that it's OK to have multiple PyInit_ symbols in the same module, since CPython won't touch them unless a symlink exists that matches one. I'm not sure whether your extensions are symlinked are not, but either way it seems like abi3audit's current check is too conservative there. I'll have a fix ready in a moment!