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

Use assumed ABI3 version for shared objects without ".abi3." #119

Merged
merged 3 commits into from
Nov 12, 2024

Conversation

vadz
Copy link
Contributor

@vadz vadz commented Nov 11, 2024

Allow auditing arbitrary shared objects, not necessarily containing ".abi3." in their name, if --assume-minimum-abi3 option is specified: suppose that all shared objects use this ABI3 version then.

This makes the tool more convenient to use for non-wheel-packaged extensions.

Also specify the default, 3.2, ABI version in a single place only and use "None" instead of the default value elsewhere.


Sorry, I couldn't prevent myself from making this PR, implementing the suggestion discussed in #117. This may address #116 too, but mostly it makes the tool much more convenient for me to use, as I don't have to rename all the files (or create symlinks for them) before running it on them.

@vadz vadz force-pushed the use-min-assumed-abi3-for-so branch from 5b2a4c5 to 964b8c2 Compare November 11, 2024 15:12
Allow auditing arbitrary shared objects, not necessarily containing
".abi3." in their name, if --assume-minimum-abi3 option is specified:
suppose that all shared objects use this ABI3 version then.

This makes the tool more convenient to use for non-wheel-packaged
extensions.

Also specify the default, 3.2, ABI version in a single place only and
use "None" instead of the default value elsewhere.
@vadz vadz force-pushed the use-min-assumed-abi3-for-so branch from 964b8c2 to 89b893f Compare November 11, 2024 15:59
@vadz
Copy link
Contributor Author

vadz commented Nov 11, 2024

And sorry for the force pushes too, I forgot to run make lint (and I am still not sure how am I supposed to do it properly, to be honest, as I don't have mypy, that it tries to run at the end, locally).

@woodruffw
Copy link
Member

And sorry for the force pushes too, I forgot to run make lint (and I am still not sure how am I supposed to do it properly, to be honest, as I don't have mypy, that it tries to run at the end, locally).

No problem. If you run make lint, it should create a temporary environment for you and run mypy within it -- you don't need to install anything explicitly.

Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
@woodruffw woodruffw merged commit be052e5 into pypa:main Nov 12, 2024
10 checks passed
@vadz
Copy link
Contributor Author

vadz commented Nov 12, 2024

No problem. If you run make lint, it should create a temporary environment for you and run mypy within it -- you don't need to install anything explicitly.

Somehow it doesn't for me:

(env) [.../abi3audit(use-min-assumed-abi3-for-so)] $ make lint
. env/bin/activate && \
	ruff format --check abi3audit/_state.py abi3audit/_cli.py abi3audit/__init__.py abi3audit/__main__.py abi3audit/_object.py abi3audit/_cache.py abi3audit/_audit.py abi3audit/_extract.py test/test_minimum_version.py test/test_audit.py test/__init__.py test/test_extract.py && \
	ruff check abi3audit/_state.py abi3audit/_cli.py abi3audit/__init__.py abi3audit/__main__.py abi3audit/_object.py abi3audit/_cache.py abi3audit/_audit.py abi3audit/_extract.py test/test_minimum_version.py test/test_audit.py test/__init__.py test/test_extract.py && \
	mypy abi3audit
12 files already formatted
/bin/sh: 4: mypy: not found
make: *** [Makefile:52: lint] Error 127

Please let me know if you think it's worth debugging this further.

@vadz vadz deleted the use-min-assumed-abi3-for-so branch November 12, 2024 21:44
@woodruffw
Copy link
Member

Weird -- it's listed as a dev-dep:

lint = ["bandit", "interrogate", "mypy", "ruff", "types-requests"]

Could you try deleting your the current venv and re-running make lint? If that still doesn't work, I can look into it further.

@woodruffw woodruffw mentioned this pull request Nov 12, 2024
@vadz
Copy link
Contributor Author

vadz commented Nov 12, 2024

OK, I see what's going on here: if you start by running make lint it indeed works as expected, but if you start by running make test INSTALL_EXTRA=test, as I had originally done, make lint doesn't work later because mypy is missing. IMHO it's a (small) bug, but it's easy to work around, of course.

And thanks for merging this PR, it makes testing ABI compatibility much simpler for us!

@woodruffw
Copy link
Member

OK, I see what's going on here: if you start by running make lint it indeed works as expected, but if you start by running make test INSTALL_EXTRA=test, as I had originally done, make lint doesn't work later because mypy is missing. IMHO it's a (small) bug, but it's easy to work around, of course.

Aha, that makes sense! Thanks for root-causing; I'll look at making a small fix for that.

And no problem, thank you for sending a PR! I greatly appreciate it.

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

Successfully merging this pull request may close these issues.

2 participants