-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 CMake build rules for (some) of the generator jit tests #7693
base: main
Are you sure you want to change the base?
Conversation
Amazingly, these have been TODO for ~ever. This adds two of them; registration_test and rungen_test will be added subsequently (assuming this PR works).
@@ -64,6 +64,7 @@ function(_add_halide_libraries TARGET) | |||
FEATURES "${args_FEATURES}" | |||
PARAMS "${args_PARAMS}" | |||
PLUGINS "${args_PLUGINS}" | |||
CPP_STUB cpp_stub_out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unused but really shouldn't be... which target consumes it? It should be listed in the target_sources
of that target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm, ok.
I'm also baffled by the build failures in apps/hannk -- something has somehow tickled the include paths in a way that makes no obvious sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unused but really shouldn't be... which target consumes it? It should be listed in the target_sources of that target.
It's just a header file -- do those need to be in target_sources
in CMake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping re "It's just a header file -- do those need to be in target_sources in CMake?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a header file -- do those need to be in
target_sources
in CMake?
If the header file is generated, then it should be listed in some target, yes. Either:
- Directly among the PRIVATE sources of the unique target that consumes it.
- As a
DEPENDS
of a custom target; other targets that use the header shouldadd_dependencies
to that target.
Monday morning review ping |
Monday morning review ping, again |
Pingity ping ping ping |
♫ Old McDonald had a ping |
Re-adding @alexreinking -- the reason that Ugly options:
|
Post-vacation ping to @alexreinking |
Bride of Return of Ping to @alexreinking |
There's a nice...ish... way to do this in CMake 3.27 (latest) that I outlined in the review suggestions. You create an interface library that propagates the links / includes and object files (filtering out GenGen.cpp.o is 3.27+). For now, I'd prefer to preserve existing usage since an eventual upgrade to CMake will allow us to do that anyway. Maybe only build redundantly if the cpp stubs are requested via the argument? |
This already exists as
I think this is the way to go given our current CMake version. |
Co-authored-by: Alex Reinking <[email protected]>
Co-authored-by: Alex Reinking <[email protected]>
Amazingly, these have been TODO for ~ever. This adds two of them; registration_test and rungen_test will be added subsequently (assuming this PR works).