[CMake] Deduplicate `Halide_LLVM_VERSION` and `LLVM_PACKAGE_VERSION` by LebedevRI · Pull Request #6646 · halide/Halide · GitHub
Skip to content

[CMake] Deduplicate Halide_LLVM_VERSION and LLVM_PACKAGE_VERSION#6646

Merged
alexreinking merged 1 commit into
halide:mainfrom
LebedevRI:cmake-dedup-llvm-version-vars
Mar 16, 2022
Merged

[CMake] Deduplicate Halide_LLVM_VERSION and LLVM_PACKAGE_VERSION#6646
alexreinking merged 1 commit into
halide:mainfrom
LebedevRI:cmake-dedup-llvm-version-vars

Conversation

@LebedevRI

Copy link
Copy Markdown
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.
@alexreinking

alexreinking commented Mar 15, 2022

Copy link
Copy Markdown
Member

@LebedevRI

Copy link
Copy Markdown
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
Copy Markdown
Member

@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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants