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 --ignore CLI option to ignore certain symbol patterns #111

Open
giampaolo opened this issue Sep 30, 2024 · 7 comments
Open

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

giampaolo opened this issue Sep 30, 2024 · 7 comments
Labels
discussion Needs further discussion

Comments

@giampaolo
Copy link

giampaolo commented Sep 30, 2024

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               
@giampaolo giampaolo changed the title Add --ignore CLI option to ignore symbol patterns Add --ignore CLI option to ignore certain symbol patterns Sep 30, 2024
@woodruffw
Copy link
Member

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
Copy link
Author

giampaolo commented Sep 30, 2024

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
Copy link
Member

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!

@woodruffw woodruffw added the discussion Needs further discussion label Sep 30, 2024
@giampaolo
Copy link
Author

giampaolo commented Sep 30, 2024

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
Copy link
Member

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
Copy link
Member

woodruffw commented Sep 30, 2024

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
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Needs further discussion
Projects
None yet
Development

No branches or pull requests

2 participants