Record the absolute path of llvm-config when found in PATH

[jberdine]

When conf-llvm finds the llvm-config executable in PATH, currently the config var only records the basename of the executable. If the PATH later changes, dependent packages can fail to build, or be built with the wrong llvm-config. This patch adds the absolute path the the found llvm-config executable to the config var to avoid this instability.

Cc: @kit-ty-kate

[jberdine]

Thanks for the update @kit-ty-kate! This LGTM, though I haven't tested the old versions.

[mseri]

The few that we can test, llvm 9 10 and 11 are failing as follows

#=== ERROR while installing llvm.11.0.0 =======================================#
# context              2.0.9 | linux/x86_64 | ocaml-base-compiler.4.12.0 | file:///home/opam/opam-repository
# path                 ~/.opam/4.12/.opam-switch/build/llvm.11.0.0
# command              ~/.opam/opam-init/hooks/sandbox.sh install bash -ex install.sh  /home/opam/.opam/4.12/lib cmake make
# exit-code            1
# env-file             ~/.opam/log/llvm-4466-0a5907.env
# output-file          ~/.opam/log/llvm-4466-0a5907.out
### output ###
# + llvm_config=
# + libdir=/home/opam/.opam/4.12/lib
# + cmake=cmake
# + make=make
# + '' --link-static --libs
# install.sh: line 36: : command not found
# + '' --link-static --libs
# install.sh: line 40: : command not found
# + '' --link-shared --libs
# install.sh: line 43: : command not found
# + echo WTF...
# WTF...
# + exit 1

i think they used to work, are you sure about the fix?

[jberdine]

I have been using conf-llvm pinned to this change without trouble. It seems that no llvm_config is being found at all for you, but there isn't enough info in the error message to know why. What do you see from running e.g. bash -ex configure.sh 11.0.0 in an environment where the desired llvm-config executable is configured? For example with PATH="/usr/local/opt/llvm@11/bin:$PATH" bash -ex configure.sh 11.0.0 |& grep "llvm_version=." I see + llvm_version=11.1.0.

[jberdine]

@kit-ty-kate, there are extra quote characters that prevent ${llvm_config} from being evaluated. I fixed version 11.0.0 and tested, but wonder if you have some automation for propagating across all the versions?

[jberdine]

@mseri if you wanted to test to see if this resolves the failure you observed, it would be very helpful.

[avsm]

@jberdine might you have a view on how to progress this? The opam-repo maintainers are all a bit overloaded with PRs at the moment...

[jberdine]

In the projects I am involved in that use llvm, a vendored version of conf-llvm using this change is used with no trouble. As far as I understand, @mseri saw some issues with just the first commit ea11b7e of this PR after it was force-pushed, but I am unaware of any issue with the PR including both commits.

For how to proceed, I would merge unless someone volunteers more testing.

[mseri]

Shouldn’t the quotes be removed from all the configuree.sh files? I would imagine the same failure may happen with the others on certain systems (cannot check the logs anynlonger unfortunately)

[jberdine]

Oh yes, you are right. I forgot that I only removed them in the latest version to get feedback from @kit-ty-kate as I was changing her code. This makes me realize that a few versions have been released in the meantime that also need updating.

[shonfeder]

Hello! I hope this isn't an unwelcome ping, but I wonder if we can either mover this PR forward or close it. WDYT?

[mseri]

I’d be fine for merging it if it is fixed and the new patches are moved to the opam-source-archive in agreement with the new policy

[shonfeder]

I am going to close this issue, provisionally. Given the accrued merge conflicts, and the fact that it has been stalled for 2 years, it seems pretty clear no one has the motivation or bandwidth to land this as is.

If I am in error, please re-open, open will an alternative fix, or open an issue to track the unmet need here.

Thanks!