Skip to content

Conversation

@alexreinking
Copy link
Member

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

@alexreinking alexreinking added the backport me This change should be backported to release versions label Dec 18, 2024
CACHE STRING "VERSION to set for custom Halide packaging")
mark_as_advanced(Halide_VERSION_OVERRIDE)

if (Halide_VERSION_OVERRIDE)
Copy link
Contributor

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)?

Copy link
Member Author

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?

Copy link
Member Author

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.

Copy link
Member Author

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

Copy link
Contributor

@LebedevRI LebedevRI Dec 18, 2024

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.

Copy link
Contributor

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 VERSION to the empty string and set SOVERSION to something meaningful, you get:

libHalide..dylib  # real lib
libHalide.19.dylib -> libHalide.dylib
libHalide.dylib -> libHalide.19.dylib

That 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 VERSION undefined, however, you get what you described:

No, i don't. I need major version to be a suffix of the name, without .-separator.

Copy link
Member Author

@alexreinking alexreinking Dec 18, 2024

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

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Member Author

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:

  1. Defaulting to having a VERSION and SOVERSION
  2. Enabling users to conveniently override "not defined"
  3. 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:

  1. Use <empty> as a special value leading to the empty string and let an empty string lead to undefined.
  2. 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?

Copy link
Contributor

@LebedevRI LebedevRI left a comment

Choose a reason for hiding this comment

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

No.

@alexreinking alexreinking merged commit ac2cd23 into main Dec 19, 2024
13 of 16 checks passed
@alexreinking alexreinking deleted the fix-debian-packaging branch December 19, 2024 17:53
alexreinking added a commit that referenced this pull request Dec 19, 2024
* Handle DESTDIR in HalidePackageConfigHelpers.cmake

Fixes #8521

* Fix Halide_[SO]VERSION_OVERRIDE when value is 0

Fixes #8522

* Upgrade LLVM to 19.1.6

(cherry picked from commit ac2cd23)
alexreinking added a commit that referenced this pull request Dec 19, 2024
Fix Debian packaging (#8524)

* Handle DESTDIR in HalidePackageConfigHelpers.cmake

Fixes #8521

* Fix Halide_[SO]VERSION_OVERRIDE when value is 0

Fixes #8522

* Upgrade LLVM to 19.1.6

(cherry picked from commit ac2cd23)
@LebedevRI
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport me This change should be backported to release versions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SOVERSION handling broken _Halide_install_pkgdeps() is broken

4 participants