search

scikit-build / cython-cmake

CMake helpers for building Cython modules
Apache License 2.0
4 stars 0 forks source link

Cleanup ideas #18

Open henryiii opened 1 week ago

henryiii commented 1 week ago

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 commented 1 week ago

Seee https://github.com/cython/cython/issues/5486#issuecomment-2187180678 - 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.