Skip to content

Conversation

arsenm
Copy link
Contributor

@arsenm arsenm commented Sep 19, 2025

This appears to not work, and the documentation only has
examples with a single target checked at a time.

@arsenm arsenm added the cmake Build system in general and CMake in particular label Sep 19, 2025 — with Graphite App
@arsenm arsenm requested a review from asb September 19, 2025 06:19
Copy link
Contributor Author

arsenm commented Sep 19, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@s-barannikov
Copy link
Contributor

I think the issue is that both variables are empty strings. So if(TARGET ${llc_target}) will be evaluates as if(TARGET), which I believe is invalid either.

@arsenm
Copy link
Contributor Author

arsenm commented Sep 19, 2025

I think the issue is that both variables are empty strings. So if(TARGET ${llc_target}) will be evaluates as if(TARGET), which I believe is invalid either.

Undefined value is not a target, which works fine

@mstorsjo
Copy link
Member

This does indeed fix the build errors - but it doesn't seem to do the right thing; if building with -DLLVM_NATIVE_TOOL_DIR=<dir>, we may have preexisting llvm-nm and llc binaries which we wanted to use; then we can just invoke ${llvm_nm_exe} right away without needing to build it. So in that way, the current if (TARGET ${llvm_nm_target}) check makes so that we don't use it at all (and don't build it either) in that case.

So what's the intended purpose of adding these TARGET checks in the first place?

@arsenm
Copy link
Contributor Author

arsenm commented Sep 19, 2025

I was trying to fix the last build error, and figured the targets didn't exist. It turns out they do exist, just aren't executable. so yes probably should drop the target checks

This appears to not work, and the documentation only has
examples with a single target checked at a time.
@arsenm arsenm force-pushed the users/arsenm/cmake/avoid-if-target-and-target branch from e09e8c9 to f6a90e3 Compare September 19, 2025 09:59
Copy link
Member

@mstorsjo mstorsjo left a comment

Choose a reason for hiding this comment

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

This works for me, thanks.

It's still unclear to me in which cases these variables actually would end up empty, but this looks safe (and works in my build config as well).

@arsenm arsenm merged commit f9c9968 into main Sep 19, 2025
9 checks passed
@arsenm arsenm deleted the users/arsenm/cmake/avoid-if-target-and-target branch September 19, 2025 10:39
@asb
Copy link
Contributor

asb commented Sep 19, 2025

Thank you, I can confirm this patch fixes my builds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake Build system in general and CMake in particular
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants