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/llvm: Don't unset LLVM_CONFIG_EXE #19971

Conversation

beholders-eye
Copy link

Left a comment in #12136, asking why we decided to unset LLVM_CONFIG_EXE.

@nektro
Copy link
Contributor

nektro commented May 15, 2024

I think it'd be better to let ZIG_USE_LLVM_CONFIG be overwritten in the root cmake file

@BratishkaErik
Copy link
Contributor

BratishkaErik commented May 15, 2024

Left a comment in #12136, asking why we decided to unset LLVM_CONFIG_EXE.

You can add -DCMAKE_PREFIX_PATH="${where_is_llvm_bin_and_libraries", but I too don't know why we unset it.

I think it'd be better to let ZIG_USE_LLVM_CONFIG be overwritten in the root cmake file

It's already can be overwritten.

@BratishkaErik
Copy link
Contributor

Is it for Exherbo's dev-lang/zig BTW? Just looked at your repositories on GitHub, I can share my (not published yet) 0.12.0 updates for Gentoo here if you want, there should be something helpful for Exherbo too

@beholders-eye
Copy link
Author

Is it for Exherbo's dev-lang/zig BTW? Just looked at your repositories on GitHub, I can share my (not published yet) 0.12.0 updates for Gentoo here if you want, there should be something helpful for Exherbo too

Yes, please. Thank you.

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Since it is not obvious why to unset, and there is no comment explaining why, it can be removed, and if it needs to be added back then it should come with a comment explaining why.

@andrewrk andrewrk enabled auto-merge (rebase) May 20, 2024 16:01
@andrewrk andrewrk merged commit 28476a5 into ziglang:master May 20, 2024
10 checks passed
@beholders-eye beholders-eye deleted the beholders-eye/rever-unset-llvm-config-exe branch May 21, 2024 13:50
@Snektron
Copy link
Collaborator

I found out why: At least on my machine, cmake hangs without this when the right LLVM version is not found.

@rohlem
Copy link
Contributor

rohlem commented May 24, 2024

In my understanding, to answer what the unset is supposed to do:

  • There is a while(1) loop that wants to find the right version of llvm-config here.
  • To do this, it looks for llvm-config using find_program, then performs a bunch of checks.
  • If one of the checks fails, it appends the found directory path to an ignore list, then continues the while loop.
    It seems like this approach was approved in the previous post-merge comment.
  • It seems like if we don't unset the variable, the same result is returned even though the ignore list has changed.
  • This leads to the configure process not terminating as @Snektron pointed out.

Unsetting the variable seems critical on the paths that continue().
A solution to allow a user-specified LLVM_CONFIG_EXE would then be to either not unset in the first iteration of the loop (use a local variable? I know nothing about CMake) or copy-paste the unset before every continue().
It would probably be better to abort if the user-specified LLVM_CONFIG_EXE fails one of the checks though, instead of ignoring it and looking in other paths.
So a local variable to see whether it was user-supplied seems the best to me (but again I know nothing about CMake).
Adding an explanatory comment either way would be beneficial.

@andrewrk
Copy link
Member

OK let's have it back with a comment explaining its purpose then please

@andrewrk
Copy link
Member

Reverted in 793f820

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.

None yet

6 participants