-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix Debian packaging #8524
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
Fix Debian packaging #8524
Conversation
| CACHE STRING "VERSION to set for custom Halide packaging") | ||
| mark_as_advanced(Halide_VERSION_OVERRIDE) | ||
|
|
||
| if (Halide_VERSION_OVERRIDE) |
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.
Can't you just do if (DEFINED Halide_VERSION_OVERRIDE)?
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.
No, because, as a cache variable, it is always defined.
I guess it might suffice to initialize it to the default and use it unconditionally?
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.
Ah, wait this patch is still wrong because I need a way to include no information at all.
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 think this is better now, @LebedevRI
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 think this is less messy, but there's an issue.
I currently do pass -DHalide_VERSION_OVERRIDE="",
in order to end up with libhalide<major>.so.<soversion> + libhalide<major>.so (symlink to former)...
I would really suggest if(DEFINED <???>) (i.e. use any override
without any attempts at verifying it), but in the end it doesn't matter.
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.
As i've said, for all i care, this patch is unnessesary, i will still have to patch stuff on the packaging side.
If you set
VERSIONto the empty string and setSOVERSIONto something meaningful, you get:libHalide..dylib # real lib libHalide.19.dylib -> libHalide.dylib libHalide.dylib -> libHalide.19.dylibThat first entry, with the truly empty version component, is insane.
I agree it is, but that is not what i get here. That insanity might be osx-specific.
If you leave
VERSIONundefined, however, you get what you described:
No, i don't. I need major version to be a suffix of the name, without .-separator.
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.
No, i don't. I need major version to be a suffix of the name, without
.-separator.
What you're describing here would be something like CMAKE_RELEASE_POSTFIX, no?
Docs: https://cmake.org/cmake/help/latest/variable/CMAKE_CONFIG_POSTFIX.html
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.
With this patch, you can set:
-DCMAKE_RELEASE_POSTFIX=19 -DHalide_VERSION_OVERRIDE="" -DHalide_SOVERSION_OVERRIDE=0
I get libHalide19.0.so and a symlink to it called libHalide19.so.
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.
Maybe.
Let me put it another way: why must this cmakefile refuse to take "" as valid version/soversion override?
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 doesn't have to, but it has to make some compromise between:
- Defaulting to having a VERSION and SOVERSION
- Enabling users to conveniently override "not defined"
- Enabling users to conveniently override to "empty string"
I'm not willing to compromise on (1) because having library versions is the correct default for too many users. So then in order to override, the override variable must be defined. If it's defined, how should I specify that I should remove the VERSION and SOVERSION properties, as I need to do for pip packaging? I can either:
- Use
<empty>as a special value leading to the empty string and let an empty string lead to undefined. - Use
<undef>as a special value leading to undefined and let an empty string be an empty string.
The mathematician in me likes (2) because "an empty string [should] be an empty string". However, I chose (1) because of the vexing .. behavior. I still haven't seen a use for the empty string that isn't covered by CMAKE_RELEASE_POSTFIX. Well, I didn't quite choose (1) because I didn't implement <empty>, but again, I haven't seen a need.
You clearly have a reason, otherwise we wouldn't be in this discussion. Can you enlighten me?
63bf3e1 to
f526d74
Compare
LebedevRI
left a comment
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.
No.
|
Thanks! |
Fix a couple build issues for Debian introduced during the 18-19 build modernization.
Will need to backport to release/19.x ASAP
ATTN @LebedevRI
Fixes #8521
Fixes #8522