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

contrib: bpf_inspect: adjust syntax for python < 3.12 #412

Merged
merged 1 commit into from
Jul 11, 2024

Conversation

brenns10
Copy link
Contributor

@brenns10 brenns10 commented Jul 9, 2024

Hi, this is a totally optional PR, but I noticed that contrib/bpf_inspect.py uses nested f-string quotes that are only available in Python 3.12 or later. It would be nice to be able to run this on older Python versions. If @Asphaltt doesn't mind it's a simple tweak to make these work on the older versions.

@Asphaltt
Copy link
Contributor

May you fix it after merging #408 ? Because it uses nested f-string quotes too.

@brenns10
Copy link
Contributor Author

That makes perfect sense! I'm happy to wait, I just wanted to post the PR since it was a patch I needed to include in my RPM builds for drgn on python 3.6-based Oracle Linux versions.

@osandov
Copy link
Owner

osandov commented Jul 11, 2024

Ok, #408 is merged. Even though we don't want to enforce style guidelines on contrib, I wonder if we should at least check the syntax with vermin.

Python 3.12 introduces the ability to nest the quote character within
f-strings. Previously, this was not allowed. While contrib scripts don't
have any specific requirement for code quality or compatibility, it
would be nice to make this runnable on Python versions prior to 3.12.

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
@osandov osandov merged commit 5dd3f47 into osandov:main Jul 11, 2024
31 of 32 checks passed
@brenns10
Copy link
Contributor Author

Even though we don't want to enforce style guidelines on contrib, I wonder if we should at least check the syntax with vermin.

Maybe, yeah. But I don't necessarily think that the contrib directory needs to have the same Python compatibility guarantees (3.6+) as the rest of drgn -- part of the goal is to lower the barrier to entry. Expecting compatibility with old (EOL...) python versions is exactly one of those barriers that we would probably like to avoid here?

I wasn't trying to require that this file or any other, e.g., be 3.6-compatible, but rather helpfully come around after the fact to make easy changes to broaden compatibility, since it wasn't intrusive.

As a distributor I'm happy to either downstream patch, or flat-out remove files from the contrib directory if they're not compatible with the Python I'm shipping on. It's kind of my job to make sure these things work :)

@brenns10 brenns10 deleted the fix_bpf_contrib branch July 11, 2024 23:32
@osandov
Copy link
Owner

osandov commented Jul 11, 2024

Yeah, I was hoping to avoid any requirements beyond the bare minimum for contrib, so I'm fine keeping it that way and accepting fixes after the fact.

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.

None yet

3 participants