Skip to content
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

[CMake] Deduplicate Halide_LLVM_VERSION and LLVM_PACKAGE_VERSION #6646

Merged
merged 1 commit into from
Mar 16, 2022

Conversation

LebedevRI
Copy link
Contributor

As far as i can tell, they are always exactly identical,
and i don't believe the claim that LLVM_PACKAGE_VERSION
doesn't exist in upper scopes is valid.

As far as i can tell, they are always exactly identical,
and i don't believe the claim that `LLVM_PACKAGE_VERSION`
doesn't exist in upper scopes is valid.
@steven-johnson steven-johnson added the buildbot_test_everything Buildbots should run all available tests on this PR (unless build_test_nothing is set). label Mar 15, 2022
@alexreinking
Copy link
Member

alexreinking commented Mar 15, 2022

Do all versions set it as a cache variable?

Even if they do, is that a guarantee going forward? Generally, modern CMake packages are moving towards not caching such things so that sibling folders can load different versions. Seems like the kind of thing we'd just need to add back later.

Also, is this every use of Halide_LLVM_VERSION? IIRC that variable gets configure_file'd into our package.

@LebedevRI
Copy link
Contributor Author

Do all versions set it as a cache variable?

Even if they do, is that a guarantee going forward? Generally, modern CMake packages are moving towards not caching such things so that sibling folders can load different versions. Seems like the kind of thing we'd just need to add back later.

I don't follow at all, but you already rely on LLVM_PACKAGE_VERSION being set into cache variable:
https://github.com/halide/Halide/search?q=LLVM_PACKAGE_VERSION

Also, is this every use of Halide_LLVM_VERSION? IIRC that variable gets configure_file'd into our package.

I mean, can you point me at the other uses?
https://github.com/halide/Halide/search?q=Halide_LLVM_VERSION

@alexreinking
Copy link
Member

Ah, I was thinking of packaging/CMakeLists.txt which uses LLVM_PACKAGE_VERSION. Thanks for the clean-up!

@alexreinking alexreinking merged commit 9ab3566 into halide:main Mar 16, 2022
@LebedevRI LebedevRI deleted the cmake-dedup-llvm-version-vars branch March 16, 2022 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buildbot_test_everything Buildbots should run all available tests on this PR (unless build_test_nothing is set).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants