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

Patch to support xNVMe #38

Merged
merged 8 commits into from May 25, 2022
Merged

Patch to support xNVMe #38

merged 8 commits into from May 25, 2022

Conversation

Baekalfen
Copy link
Contributor

@Baekalfen Baekalfen commented May 16, 2022

Hi, I used autopxd over at xNVMe, and needed to patch a few things to make it work, and I thought it could be of use to others.

The commits should be self-contained and have each the functionality they describe.

An overview:

  • --regex on command-line to allow arbitrary conversions. Some datatypes or syntax just doesn't get parsed correctly by autopxd. As a quick solution, I can hotfix something easily without making full and proper support in autopxd -- for example an opaque pointer in a function I don't need. (--regex 's/FILE \*/void */g').
  • Flattening members of unnamed unions and structs. Cython just cannot express these, so I made a "hack" where these members become accessible at least in some manner. See example below.
  • Make enums into cpdef to make them accessible from Python as well as Cython for free.
  • Ignore StaticAssert. Simply skip them. They don't make much sense in Cython and currently it makes autopxd output faulty code.
  • Possible to ignore known/defined declarations. Maybe there's a better way to do this, but I needed this to support multiple .h files. This way, I can provide the declarations from other headers, so they are not repeated. There is a better way. The whitelist kwarg.
  • Replace wget in favor of urllib. Not all distros has wget (nor cURL) installed, so I opted for a native Python implementation. This works across all distros I have tried (xNVMe test run).

I would love to get feedback from you, especially on the part I will highlight.

struct test_struct {
    int a;
    struct {
        int b;
        int c;
    }
}
cdef struct test_struct:
    int a
    int b
    int c

Comment on lines 85 to 94
if hasattr(decl, "type") and hasattr(decl.type, "name") and decl.type.name in additional_ignore_declarations:
continue

if hasattr(decl, "decl") and hasattr(decl.decl, "name") and decl.decl.name in additional_ignore_declarations:
continue

if not hasattr(decl, "name") or decl.name not in (IGNORE_DECLARATIONS | additional_ignore_declarations):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to hear, if there's a better way to do this.

@elijahr
Copy link
Owner

elijahr commented May 17, 2022

@Baekalfen this is great! Can you get tests passing?

@Baekalfen
Copy link
Contributor Author

@Baekalfen this is great! Can you get tests passing?

Yes, I'll get it fixed. I realize the thing with anon nested structs (like below) isn't totally correct. I'll investigate further if it needs to be narrowed down or removed.

    int nested__d
    int nested__e

@Baekalfen
Copy link
Contributor Author

Baekalfen commented May 18, 2022

They should be fixed now to get the best of both worlds. The confusing came from trying to print the structs.

This will work for any struct as long as it only has named nested structs inside it.

cdef test_struct A = ...
A.nested.x = 2
print(A)

But the same setup with a struct that contains a nested anon struct will result in a compile error along the lines of:

test_cython.c:4240:18: error: conflicting types for '__pyx_convert__to_py_struct___xnvme_opts_css'
static PyObject* __pyx_convert__to_py_struct___xnvme_opts_css(struct _xnvme_opts_css s) {

But dropping the print of the entire struct, will make it work. I believe this is either a bug in Cython or an accepted limitation from their side.

cdef test_struct A = ...
A.nested.x = 2
print(A.a)
print(A.b)
print(A.c)
print(A.nested.x)

Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
…ered by autopxd

Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
@Baekalfen Baekalfen force-pushed the patch_xnvme branch 3 times, most recently from f6dab57 to 66ce9b8 Compare May 19, 2022 08:45
@Baekalfen
Copy link
Contributor Author

@elijahr The tests and the formatting should all be done now, but there's something with the linting step in GHA.

Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
Signed-off-by: Mads Ynddal <m.ynddal@samsung.com>
@Baekalfen
Copy link
Contributor Author

Baekalfen commented May 20, 2022

I removed the additional ignore thing. Using the whitelist kwarg was much easier once I got it to work. I think the docs are a little off. You have to add '<stdin>' to the whitelist to allow declarations from the .h file you are providing. Otherwise it only takes declarations from the whitelisted includes.

@elijahr
Copy link
Owner

elijahr commented May 25, 2022

This is great! Thanks so much for the contribution!

@elijahr elijahr merged commit 997608c into elijahr:master May 25, 2022
@elijahr
Copy link
Owner

elijahr commented May 25, 2022

@Baekalfen this has been released in https://pypi.org/project/autopxd2/2.1.1/

@Baekalfen
Copy link
Contributor Author

@Baekalfen this has been released in https://pypi.org/project/autopxd2/2.1.1/

Fantastic! Thank you!

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

2 participants