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 "buildtool" to generate C source for cffi-based modules. #76

Closed
wants to merge 1 commit into from

Conversation

inklesspen
Copy link
Contributor

This is useful when using alternative PEP 517 build systems, such as meson-python. It can be used as a CLI script or from an if __name__ == "__main__" block.

Examples of usage:

  • CLI tool: cffi-buildtool _fontconfig.c.py _fontconfig.c
  • Called from build script:
if __name__ == "__main__":
    from cffi.buildtool import generate_c_source
    print(generate_c_source(ffibuilder))

This should be useful in a variety of build scenarios, not just supporting meson-python.

Fixes #47.

However, I think I should probably add some tests, and I do not understand the structure of the CFFI tests. Please advise.

Also I want to write some docs explaining how to use it, but I will do that after you give the approval for this basic approach.

This is useful when using alternative PEP 517 build systems, such as meson-python. It can be used as a CLI script or from an `if __name__ == "__main__"` block.
@arigo
Copy link
Contributor

arigo commented May 16, 2024

I think I understand your approach, but IMHO it would be better to fix existing APIs rather than add a completely different one. If I'm understanding this correctly, the original issue is that calling ffi.emit_c_code() will import setuptools right now, which is not necessary. Would fixing that issue be enough, and if not, why not?

@inklesspen
Copy link
Contributor Author

Well, as noted in #47 the recompile method is pretty complicated and is trying to serve three use cases (generate c source, generate c source and compile it, generate c source and distutils metadata) with a binary if statement.

I thought since so much time had passed without a refactor I would just submit the tooling to use instead.

But yes, fixing that issue would solve it. I just don't use the other functionality of that method enough to understand how it should be refactored.

@arigo
Copy link
Contributor

arigo commented May 16, 2024

I'm not against some code duplication at this point. But I'm asking if your tool could be present behind the existing API of emit_c_code(), which could be changed to no longer call recompile() but your logic instead, if it supports the same use cases.

@inklesspen
Copy link
Contributor Author

Ah, hm, perhaps. I'll look into it over the next few days.

@arigo
Copy link
Contributor

arigo commented May 16, 2024

Alternatively, here's a minimal change to add yet another flag to recompile() to have it not call ffiplatform: bb52dcb (committed to the branch emit-c-code-without-distutils). Can you check if that is enough for you?

@inklesspen
Copy link
Contributor Author

Alternatively, here's a minimal change to add yet another flag to recompile() to have it not call ffiplatform: bb52dcb (committed to the branch emit-c-code-without-distutils). Can you check if that is enough for you?

Yeah, I think this is good enough, actually.

@arigo
Copy link
Contributor

arigo commented May 28, 2024

@nitzmahone Can this checkin still go into the release you're preparing? If so, just merge the branch emit-c-code-without-distutils. Thanks for the work!

@webknjaz
Copy link

webknjaz commented May 28, 2024

@arigo I'd imagine that could be judged by the CI status in a PR that one would make out of such a branch, right? It feels rather weird to merge-in untested code blindly…

UPD: Ah, I noticed there's #81 already, thanks to Matt!

@arigo
Copy link
Contributor

arigo commented May 28, 2024 via email

@nitzmahone
Copy link
Member

Yeah, I already created #81 for this. I'll probably add another commit with a test or something- since the commit originated inside this repo, GHA is "helpfully" caching the list of checks, so just re-running the tests against the same commit isn't picking up the updated workflow context and fixes. 😞

@webknjaz
Copy link

@arigo the reason is that a workflow of direct pushes without checks in obvious reviewable places is completely foreign to GitHub. Making actual PRs ensures transparency. Just the recent case of hijacking the xz-utils project serves as a fairly good example of why transparency and provenance are important. So this mechanism helps with at least some concern around that, by making it easier to audit what's actually been happening and keeping the context around reviewing changes coupled with changes themselves. By the way, creating branches in upstream repos contributes to polluting contributor forks with stale uninteresting branches in different stages of their lives over time. So I'm seeing the increase of use of a different practice — keeping topic branches away from upstream, in forks. This also reduces the noise related to the notifications GitHub tends to send related to different things happening in projects.

@arigo
Copy link
Contributor

arigo commented May 28, 2024 via email

@webknjaz
Copy link

@arigo thanks for understanding! I forgot to mention a bonus side effect — you can get CI build statuses in your fork before making a PR if needed (when it's not yet ready for review). This way, you won't spend extra CI minutes of the org because they are separate from your account. Having many CI runs in the org may end up with other people's tests being queued for a substantial amount of time.

@nitzmahone
Copy link
Member

Heh, far be it from me to tell the esteemed creator of the project that he's "holding the tool incorrectly" 😉 - I'm thrilled to have @arigo's continued participation in keeping CFFI going in any way that he wants! With so many different ways of interacting with git, I probably should've written down the assumed development workflow somewhere when we moved the project to GH, so that's on me.

The xz-utils thing was like watching one of my personal anxiety dreams unfolding in real life- suddenly the inconvenience of "tin-foil hat" practices like commit signing, hard-pinned deps, and personal vetting of contributors begin to seem quite reasonable and worthwhile.

Thanks also @webknjaz for taking a pass over some of the CI stuff- been tricky to balance "drowning in day job" with sanding down a bunch of the rough edges from papering over GHA limitations. I'll try to make some time to look that over in the next couple days and do a 1.17.0rc2 that includes #81.

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.

Document how to use meson to build a cffi cextension
4 participants