Skip to content

[CMake] Deduplicate Halide_LLVM_VERSION and LLVM_PACKAGE_VERSION #6646

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.

Sign up for GitHub

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

Jump to bottom
Merged
alexreinking merged 1 commit into from
Mar 16, 2022
Merged

[CMake] Deduplicate Halide_LLVM_VERSION and LLVM_PACKAGE_VERSION #6646

alexreinking merged 1 commit into from
Mar 16, 2022

Conversation

@LebedevRI
Copy link
Contributor

@LebedevRI LebedevRI commented Mar 12, 2022

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.

@LebedevRI
[CMake] Deduplicate Halide_LLVM_VERSION and LLVM_PACKAGE_VERSION
706e99c 
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
Copy link
Member

alexreinking commented Mar 15, 2022 •
edited
Loading

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

LebedevRI 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.

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

alexreinking commented Mar 16, 2022

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

Reviewers

@alexreinking alexreinking alexreinking approved these changes

@halidebuildbots halidebuildbots Awaiting requested review from halidebuildbots

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@LebedevRI @alexreinking @steven-johnson