Check for required headers before compilation#514
Conversation
Also fix conditional EGL API inclusions. They should always be included because all of their types are already redefined and available.
vzhurba01
added
enhancement
|
/ok to test |
This comment has been minimized.
This comment has been minimized.
|
/ok to test |
| {{if 'cudaEGLStreamProducerPresentFrame' in found_functions}}cdef cudaError_t _cudaEGLStreamProducerPresentFrame(cudaEglStreamConnection* conn, cudaEglFrame eglframe, cudaStream_t* pStream) except ?cudaErrorCallRequiresNewerDriver nogil{{endif}} | ||
| {{if 'cudaEGLStreamProducerReturnFrame' in found_functions}}cdef cudaError_t _cudaEGLStreamProducerReturnFrame(cudaEglStreamConnection* conn, cudaEglFrame* eglframe, cudaStream_t* pStream) except ?cudaErrorCallRequiresNewerDriver nogil{{endif}} | ||
| {{if 'cudaGraphicsResourceGetMappedEglFrame' in found_functions}}cdef cudaError_t _cudaGraphicsResourceGetMappedEglFrame(cudaEglFrame* eglFrame, cudaGraphicsResource_t resource, unsigned int index, unsigned int mipLevel) except ?cudaErrorCallRequiresNewerDriver nogil{{endif}} | ||
| {{if True}}cdef cudaError_t _cudaEGLStreamProducerPresentFrame(cudaEglStreamConnection* conn, cudaEglFrame eglframe, cudaStream_t* pStream) except ?cudaErrorCallRequiresNewerDriver nogil{{endif}} |
There was a problem hiding this comment.
Is there an easy way to leave a helpful hint here?
What I have in mind:
# For tagging <explanation here>:
helpful_hint_here = True
Then, instead of {{if True}} → {{if helpful_hint_here}}
There was a problem hiding this comment.
For a complete solution it's not that easy. Updating this file and placing the explanation in setup.py is simple, the problem is that this pattern is used all across cuda.bindings as part of the auto-generation.
On a major release we'll resolve issue #488 and I think most if not all of these instances will disappear, so lets revisit this then.
There was a problem hiding this comment.
Sounds good. Thanks for the explanation!
| return err | ||
|
|
||
| {{endif}} | ||
| {{if True}} |
There was a problem hiding this comment.
Could we use the helpful_hint_here, too?
Out of curiosity: Are these {{if True}} actually required? — Even if not, I think helpful hints here are useful.
There was a problem hiding this comment.
They're not needed but I want it there for consistency with the rest of the source base. Visually at least, it helps identify which sections use redefined types rather than extern with a C header.
Overall same comment as above, lets revisit this after #488.
| # - cuda_device_runtime_api.h | ||
| } | ||
|
|
||
| # Assert that all headers exist |
There was a problem hiding this comment.
It's significantly safer / less error prone to move non-trivial code into functions. This avoids accidentally using or overwriting variables from elsewhere, among other things.
For example here:
def assert_that_all_headers_exist():
...
assert_that_all_headers_exist()
| break | ||
| if not os.path.exists(path): | ||
| missing_headers += [header] | ||
|
|
There was a problem hiding this comment.
Suggested change:
for path in path_candidate:
if os.path.exists(path):
header_paths.append(path)
break
else:
missing_headers.append(header)
Related ChatGPT convo with rationale: https://chatgpt.com/share/67d35f63-57d8-8008-97ae-cbcc582781a6
There was a problem hiding this comment.
Done
Yeah that function felt really weird to me too but my personal queries with gpt didn't turn out the same. The for-else is a neat trick! Thanks!
| if not os.path.exists(path): | ||
| print(f"Missing header {header}") | ||
|
|
||
| for library, header_paths in header_dict.items(): |
There was a problem hiding this comment.
Optional, since you're not changing much here, but I'd move this loop into a function, too.
|
/ok to test |
| {{if 'cudaEGLStreamProducerPresentFrame' in found_functions}}cdef cudaError_t _cudaEGLStreamProducerPresentFrame(cudaEglStreamConnection* conn, cudaEglFrame eglframe, cudaStream_t* pStream) except ?cudaErrorCallRequiresNewerDriver nogil{{endif}} | ||
| {{if 'cudaEGLStreamProducerReturnFrame' in found_functions}}cdef cudaError_t _cudaEGLStreamProducerReturnFrame(cudaEglStreamConnection* conn, cudaEglFrame* eglframe, cudaStream_t* pStream) except ?cudaErrorCallRequiresNewerDriver nogil{{endif}} | ||
| {{if 'cudaGraphicsResourceGetMappedEglFrame' in found_functions}}cdef cudaError_t _cudaGraphicsResourceGetMappedEglFrame(cudaEglFrame* eglFrame, cudaGraphicsResource_t resource, unsigned int index, unsigned int mipLevel) except ?cudaErrorCallRequiresNewerDriver nogil{{endif}} | ||
| {{if True}}cdef cudaError_t _cudaEGLStreamProducerPresentFrame(cudaEglStreamConnection* conn, cudaEglFrame eglframe, cudaStream_t* pStream) except ?cudaErrorCallRequiresNewerDriver nogil{{endif}} |
There was a problem hiding this comment.
Sounds good. Thanks for the explanation!
|
2 participants
Close #283
Since graphics APIs and types are redefined, we don't need to include or parse those headers. Note that in the future, issue #488 will extend the required headers to graphics headers as well.