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

Cleanup ideas #18

Open
henryiii opened this issue Jun 24, 2024 · 1 comment
Open

Cleanup ideas #18

henryiii opened this issue Jun 24, 2024 · 1 comment

Comments

@henryiii
Copy link
Contributor

henryiii commented Jun 24, 2024

I have some ideas on the redesign, and want to put them down:

First, I think we should not hard-code flags, and instead allow them to be passed through, like TARGET_LANGUAGE and LANGUAGE_LEVEL unless there's a benefit for us (possibly with TARGET_LANGUAGE? Edit: Yes, we set the extension from this). Users should generally set these options in source files anyway, and if not, they can just as easily pass them through CYTHON_ARGS without us adding lots of custom options. We should also not try to set defaults for the user, and leave that up to Cython.

The command supports adding multiple input files and a single output file - not sure what that does, but if it's useful, we should mimic that here. I don't think there's a lot of benefit in supporting multiple outputs (users can just write a loop) - I'd like to mimic cythonize as closely as possible, just trying to bring it into CMake.

I think instead of an output variable, maybe we should support setting the OUTPUT file directly? That way users can choose where to put the output file location easily. (This also would work best if we don't support multiple input files to multiple output files, above)

Once these are addressed, I think the back-compat "add_cython_target" could be designed using "cython_compile_pyx" Ahh, nice, already done. It could even only exist in scikit-build (classic), as it has a really poor name (it doesn't have anything to do with targets). The "official" version upstreamed to Cython could be called "cythonize".

@henryiii
Copy link
Contributor Author

Seee cython/cython#5486 (comment) - maybe we should require the LANGUAGE keyword? Currently it changes based on the LANGUAGES set (C unless only CXX is available), which might be slightly supposing? Unless we can detect the language, might be best to just require this for now. Also, I think LANGUAGE is better than TARGET_LANGUAGE, as it's unambiguous already.

@henryiii henryiii mentioned this issue Jun 24, 2024
1 task
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 a pull request may close this issue.

1 participant