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.

Sign up for GitHub

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

Jump to bottom

feat: rewrite #19

Closed
wants to merge 1 commit into from
Closed

feat: rewrite #19

wants to merge 1 commit into from

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Jun 24, 2024 •
edited
Loading

Close #18.

  • Remove the back-compat method for now; I think it could exist only inside scikit-build (classic)
  • Currently require 3.20 (for cmake_path), which was implicitly required before. Reduced to at least 3.15, probably 3.7, but DEPFILE only works on Ninja until Make was added in 3.20.
  • Try to support spaces in paths correctly
  • Add a test using scikit-build-core
  • Add a Cython::Cython target
  • Try to compute the language automatically if not specified and both C and CXX are enabled.

Along with this, this is the new syntax for the Cython_compile_pyx function:

Cython_compile_pyx(<pyx_file>
                  [LANGUAGE C | CXX]
                  [CYTHON_ARGS <args> ...]
                  [OUTPUT <OutputFile>]
                  [OUTPUT_VARIABLE <OutputVariable>]
                  )

OUTPUT sets the file path, otherwise it's the same name with new extension in the current binary dir. OUTPUT_VARIABLE contains the file path to the generated output.

  • Add a cached CYTHON_ARGS variable to set the default. It would be useful for adding things like annotation on all calls.
  • Why would you pass multiple files to cythonize? It only produces one output. Should we allow multiple files here, and just treat the first one as special? Edit: leaving this off for now. It could be added, it would be incompatible with OUTPUT and a list of files would be placed in OUTPUT_VARIABLE.

If this works well, we can propose a cythonize function upstream to Cython; if they make changes to the signature, we can wrap those once it's released and provide "cythonize" for previous versions of Cython.

@henryiii henryiii marked this pull request as draft June 24, 2024 22:35
@henryiii henryiii force-pushed the henryiii/feat/rewrite branch 5 times, most recently from a7b332c to 26b1889 Compare June 25, 2024 16:03
@henryiii henryiii mentioned this pull request Jun 26, 2024
Open
@henryiii
Copy link
Contributor Author

@vyasr, thoughts?

@henryiii henryiii force-pushed the henryiii/feat/rewrite branch 2 times, most recently from 4e9afe2 to 4c12c10 Compare June 26, 2024 15:34
@henryiii henryiii marked this pull request as ready for review June 26, 2024 16:14
@henryiii henryiii mentioned this pull request Jun 26, 2024
Open
vyasr
vyasr reviewed Jun 26, 2024
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

What is the back-compat method that you meant? If you mean the add_cython_target, yes I agree that should go. I read through #18 and I think as you noted that some of the ideas were already implemented in my original prototype, but also not all of them made it from my prototype into what's in this repo so there are probably additional improvements still to be made here. This was definitely one of the changes I had intended.

I don't have any powers in this repo unfortunately, but generally the changes look good to me.

README.md Outdated Show resolved Hide resolved
src/cython_cmake/cmake/UseCython.cmake Show resolved Hide resolved
src/cython_cmake/cmake/FindCython.cmake Outdated Show resolved Hide resolved
src/cython_cmake/cmake/FindCython.cmake Outdated
# And the following target:
#
# ``Cython::Cython``
# The Cython executable
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we stop documenting the CYTHON_EXECUTABLE variable altogether and just say that the only supported way to get it is via this target?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are different when cross compiling. Targets will use the cross-compiling emulator, while variables are just variables. Also, target's can't be used in some places like during the configure process, while variables can.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, I guess the cython command could itself have been cythonized and therefore be native to the target system.

src/cython_cmake/cmake/UseCython.cmake Outdated Show resolved Hide resolved
src/cython_cmake/cmake/UseCython.cmake
endif()
# Generated depfile is expected to have the ".dep" extension and be located
# along side the generated source file.
set(depfile_path "${CYTHON_OUTPUT}.dep")
Copy link
Contributor

Choose a reason for hiding this comment

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

We added depfile support in https://github.com/rapidsai/rapids-cmake/pull/579/files, so you may want to double-check there to see if we did anything differently from what you did here (good or bad) since both our code and the new code in this repo are very stripped down relative to what UseCython.cmake looked like before this PR.

Copy link
Contributor

@jcfr jcfr Jun 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I believe all features have been added while maintaining the history and giving credits to the various contributors.

For reference, depfile support has indeed been added through 490f199

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only thing I saw I commented on that PR; that one is missing the depfile as output, while I think it's correct to list it there (since it's not always output, just usually, so CMake likely can't assume it's output).

src/cython_cmake/cmake/UseCython.cmake Outdated Show resolved Hide resolved
src/cython_cmake/cmake/UseCython.cmake Outdated Show resolved Hide resolved
@jcfr
Copy link
Contributor

jcfr commented Jun 27, 2024

For reference, testing of the module using scikit-build-core is being added through:

@henryiii
Copy link
Contributor Author

Added a few more fixes; you can now use UseCython directly if you want. I've also made an F2Py version at https://github.com/scikit-build/f2py-cmake.

@henryiii henryiii mentioned this pull request Jul 16, 2024
Merged
This was referenced Jul 17, 2024
Merged
Merged
Merged
Merged
This was referenced Jul 17, 2024
Merged
Merged
This was referenced Jul 17, 2024
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
This was referenced Jul 18, 2024
Merged
Merged
Merged
Merged
@vyasr
Copy link
Contributor

vyasr commented Jul 27, 2024

Apologies, totally dropped the ball on this one. I'll take a pass at reviewing, but overall it seems like it's come a long way and is a definite improvement.

@henryiii henryiii mentioned this pull request Sep 6, 2024
Merged
@henryiii henryiii force-pushed the henryiii/feat/rewrite branch 2 times, most recently from 0f19767 to 4d122ed Compare September 7, 2024 04:31
@henryiii
feat: rewrite
3c1b93d 
Signed-off-by: Henry Schreiner <[email protected]>

feat: deduce C/CXX if possible

Signed-off-by: Henry Schreiner <[email protected]>

fix: better handling of output

Signed-off-by: Henry Schreiner <[email protected]>

docs: add info

Signed-off-by: Henry Schreiner <[email protected]>
@henryiii
Copy link
Contributor Author

I think most of this is in, minus some stylistic stuff

@henryiii henryiii closed this Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcfr jcfr jcfr left review comments

@vyasr vyasr vyasr left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cleanup ideas
3 participants
@henryiii @jcfr @vyasr