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

Don't require abi3 in shared file names or at least make it clear that it is required in the error message #117

Closed
vadz opened this issue Nov 7, 2024 · 2 comments · Fixed by #118
Labels
C:cli CLI components

Comments

@vadz
Copy link
Contributor

vadz commented Nov 7, 2024

I'm not sure what is the intention here, but currently the tool doesn't accept shared libraries if they don't have abi3 in their name and doesn't say that this is the reason for not accepting them:

$ abi3audit foo.so         
usage: abi3audit [-h] [--debug] [-v] [-R] [-o OUTPUT] [-s] [-S] [--assume-minimum-abi3 ASSUME_MINIMUM_ABI3] SPEC [SPEC ...]
abi3audit: error: argument SPEC: invalid make_specs value: 'foo.so'
$ mv foo.so foo.abi3.so 
$ abi3audit foo.abi3.so
[14:30:30] 💁 foo.abi3.so: 1 extensions scanned; 1 ABI version mismatches and 0 ABI violations found                                                                                                                            

FWIW this is with abi3audit==0.0.17 under Debian 12 (Bookworm).

Personally I think that the tool shouldn't insist on the .abi3. part: I understand that it's the standard convention for the wheels, but we don't distribute our Python extensions in a wheel and so they don't have it in their names, but if the tool does insist on enforcing this convention, it should at least say so, i.e. output something like

abi3audit: error: argument SPEC: 'foo.so' not recognized as Python extension because it doesn't contain '.abi3.' in its name

As it is, I had to go look at the source code to understand what was going on, which is suboptimal from the user experience point of view.

I should be able to make a PR changing this code to do either of the things above, please let me know if you'd like me to make one.

@woodruffw
Copy link
Member

Personally I think that the tool shouldn't insist on the .abi3. part: I understand that it's the standard convention for the wheels, but we don't distribute our Python extensions in a wheel and so they don't have it in their names, but if the tool does insist on enforcing this convention, it should at least say so, i.e. output something like

The reason we do this is because it's otherwise impossible to infer the user's intent when they pass us a single extension (instead of a package distribution): there's nothing invalid about a non abi3 extension, so we don't want to produce a bunch of abi3 violation notices when the extension wasn't intended to be abi3 in the first place.

(This is non-ideal, since as you note the .abi3. tag is optional, and CPython has no runtime checks for whether an extension is actually abi3.)

See #116 for some related context -- we could relax this to allow any extension by requiring that the user always pass --assume-minimum-abi3 in that case. But I'd like to think about that some more before we go forwards with it 🙁

I should be able to make a PR changing this code to do either of the things above, please let me know if you'd like me to make one.

Thank you! If you could improve the error message in a PR, I would greatly appreciate it.

@woodruffw woodruffw added the C:cli CLI components label Nov 7, 2024
vadz added a commit to vadz/abi3audit that referenced this issue Nov 7, 2024
Call make_specs() explicitly and handle exceptions from it in order to
show the appropriate error message instead of letting argparse call it
implicitly. As mentioned in that module documentation:

> In general, the type keyword is a convenience that should only be used
> for simple conversions that can only raise one of the three supported
> exceptions. Anything with more interesting error-handling or resource
> management should be done downstream after the arguments are parsed.

And this is exactly what this commit does.

Closes pypa#117.
@vadz
Copy link
Contributor Author

vadz commented Nov 7, 2024

I rather like the idea of allowing any file names if --assume-minimum-abi3 is specified, I thought about adding a new option just for this (e.g. --assume-abi-so), but this looked like an overkill, while reusing the existing option seems perfect.

But for now I've just created a PR improving the error message, it's already better than nothing IMHO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:cli CLI components
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants