-
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
Flesh out CPack packaging for releases. #5135
Conversation
We generally don't break user code without a release with a deprecation warning first, so I'm not sure '0.' is necessary. We'll just bump the major every time we delete some deprecated APIs or backends. It happens infrequently enough. |
@@ -199,7 +199,7 @@ RISCV_CXX_FLAGS=$(if $(WITH_RISCV), -DWITH_RISCV, ) | |||
RISCV_LLVM_CONFIG_LIB=$(if $(WITH_RISCV), riscv, ) | |||
|
|||
INTROSPECTION_CXX_FLAGS=$(if $(WITH_INTROSPECTION), -DWITH_INTROSPECTION, ) | |||
EXCEPTIONS_CXX_FLAGS=$(if $(WITH_EXCEPTIONS), -DWITH_EXCEPTIONS -fexceptions, ) | |||
EXCEPTIONS_CXX_FLAGS=$(if $(WITH_EXCEPTIONS), -DHALIDE_WITH_EXCEPTIONS -fexceptions, ) |
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.
Why is this renaming necessary?
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.
Because the behavior of Error.cpp changes depending on whether or not Halide was built with exceptions, so our users might need to know whether it was enabled. The old name, WITH_EXCEPTIONS
, is almost guaranteed to conflict with another project so I prefixed it. It also now matches HALIDE_ENABLE_RTTI
.
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.
I don't think this macro is ever exported though. It's only used in Error.cpp, and no headers. This is now inconsistent with all the other WITH flags in the makefile.
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.
If we think clients might need to check it dynamically should we add an api like we did for llvm_version?
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.
The use cases for checking it would seem to need to be compile-time mostly, I would think (e.g. so that you can wrap try/catch or not).
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.
So perhaps it should be something exported by the Halide package in cmake (not sure of correct terminology here) as the variable HALIDE_WITH_EXCEPTIONS? But this is independent of what the #define is called.
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.
Linking with varying exception-handling capabilities is generally risky, as is linking with varying RTTI info, which is why packages that allow users to disable them have to propagate that information at build time.
Question: What is the expectation of build compatibility across versions? E.g. we're talking about renaming cmake config vars in another thread. Does this require bumping the major because it requires downstream users to adjust their build setup? |
That's a good question. Semantic versioning is fundamentally about link-time compatibility. The hard requirement is that you should always be able to load a newer library of the same major version than the one you linked to. I think we should extend that to our build API. However, major version 0 has no such requirements and we should feel free to make breaking changes in the CMake until a numbered release is issued. |
Suggestion for versioning from offline discussion: Just match the version number of the included LLVM, and try to release at the same cadence (6 months) |
SGTM. |
Question: do we have any need to support the following scenarios:
|
By "Halide" you mean the compiler, right? Not Halide-generated code? Assuming you mean the compiler, I don't think we should expend any effort in supporting that. |
Yes, I mean the compiler / library, not the generator outputs. |
Yeah, these multiple version issues normally only come up for large projects with multiple subteams, and large projects right now typically use Halide as an AOT compiler, so multiple Halide versions wouldn't ever end up in the same process. |
(Monday morning check: is this still pending more work to do, or is it ready for review?) |
Still a couple TODOs:
|
A mac mini just arrived today, so I'll be able to test on mac imminently. |
PR now includes version 10.0 bump and macOS packaging script (using zsh since I did everything on Catalina). Still TODO: bundling static libs on Linux / macOS. This will likely be a hack. |
1. Support selecting library type in `find_package`. 2. Support linking to shared LLVM or bundling static LLVM in libHalide.a 3. Add packaging scripts.
All the major things are here. It would be nice to get the |
Can I bump this? I think it's in a merge-able state. |
Is that on my plate to get done, and is it a blocker for landing this? |
You touched that code most recently, right? But it is not a blocker, no. |
Yep. Working on it now. |
#5183 is ready to review if you want |
We can go ahead and land this now. The |
Main changes:
a. Linux packages include Debug/Release x Static/Shared
b. macOS packages include Release x Static/Shared
c. Windows packages include Debug/Release x Shared
WITH_EXCEPTIONS
macro toHALIDE_WITH_EXCEPTIONS
a.
LLVM_USE_SHARED_LLVM_LIBS
->Halide_SHARED_LLVM
. This was never an LLVM option, so we need to stay out of their namespace.b.
Halide_BUNDLE_LLVM
-> On all major platforms, unpacks LLVM's static libraries and includes their objects in the Halide static library.When using
find_package
, users can now request either the static or shared versions of Halide. This works in one of several ways (in order of preference):find_package(Halide REQUIRED static)
. If it cannot load the requested libraries, it dies.Halide_SHARED_LIBS
variable. When true-y, it loads the shared libs, else the static ones. If it cannot load the requested libraries, it dies.Halide_SHARED_LIBS
is not defined, it imitates the behavior of FetchContent by examining theBUILD_SHARED_LIBS
variable. If it is true-y or not defined, it tries to load the shared libraries first. Otherwise it tries to load the static libraries first. Finally, it falls back to the other type.As an advanced use-case, the user may request both static and shared by specifying both components. In this case, the targets
Halide::shared::{Halide,Generator,RunGenMain}
andHalide::static::{Halide,Generator,RunGenMain}
are created, butHalide::Halide
is unavailable. As a consequence, the convenience methodadd_halide_library
is also not usable.Fixes #3971
Fixes #3972
Fixes #4254