search

conda-forge / llvmdev-feedstock

A conda-smithy repository for llvmdev.
BSD 3-Clause "New" or "Revised" License
8 stars 41 forks source link

ensure {LibXml2,ZLIB,zstd}_ROOT is respected & set it in NATIVE_FLAGS #204

Closed h-vetinari closed 1 year ago

h-vetinari commented 1 year ago

This isn't strictly speaking necessary for this feedstock, but I believe we need it for clang; otherwise libxml2 & zstd will be found in the host environment, with the wrong symbols.

conda-forge-webservices[bot] commented 1 year ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

h-vetinari commented 1 year ago

@isuruf, this was a bit fiddly to get right, but I think this is a better way than what we did in #203 / https://github.com/conda-forge/llvmdev-feedstock/commit/345e7fffc8c896442d18d2f559c9d26a6c352a72. In particular, I believe patch 0003 is necessary for the clang side, where we always end up having zlib/zstd in the host env, which end up being picked up even if I set NATIVE_FLAGS="${NATIVE_FLAGS};-DLLVM_ENABLE_LIBXML2=OFF;-DLLVM_ENABLE_ZSTD=OFF".

By using the patch, we have a chance to point the respective _ROOT to the build env, and this PR serves as the proof of that approach by doing the same adaptation for NATIVE_FLAGS already here.

Unless there are comments from your side, I'll merge this in ~24h - it's only the rc-branch, so not that critical, and we'll have a more in-depth review for 16.0 GA anyway.

isuruf commented 1 year ago

I don't understand you. What does this gain? Nothing.

h-vetinari commented 1 year ago

I don't understand you. What does this gain? Nothing.

I've tried to describe the issue I encountered on clangdev as well as I could. Even before my PR there, it was already using ZLIB_ROOT. I did add the same LLVM_ENABLE_{LIBXML2,ZSTD}=OFF as you did here, and it failed (because it kept finding zstd in the host). The obvious alternative to me is to ensure that we can set the root for those other package as well - and that'd be something upstreamable as well IMO.

The llvm cmake infrastructure is quite impenetrable (even more so than CMake is already to me), and a bunch of subprojects will pick up the llvm(-the-lib)-cmake files from our llvmdev installation. Seeing how the file I'm patching gets templated, I believe that it might end up getting installed. Perhaps I'm wrong?

In any case, my goal here was to clean up the dependencies in the llvm stack (following the various link checks); I may misunderstand certain details occasionally, but if I only ever did things with total certainty, I'd get nothing done. So I try out hypotheses.

h-vetinari commented 1 year ago

But yes, just pointing the CMAKE_PREFIX_PATH to the BUILD_PREFIX is a much better solution.

isuruf commented 1 year ago

I've tried to describe the issue I encountered on clangdev as well as I could. Even before my PR there, it was already using ZLIB_ROOT. I did add the same LLVMENABLE{LIBXML2,ZSTD}=OFF as you did here, and it failed (because it kept finding zstd in the host). The obvious alternative to me is to ensure that we can set the root for those other package as well - and that'd be something upstreamable as well IMO.

The patch doesn't make sense to me.

  1. You don't have to set libxml2_ROOT here as you can set it in the clang side
  2. It's not upstreamable as the llvm installation will not be relocatable. (Not something we have to worry about in cmake)
  3. If setting libxml2_ROOT works here, then it should work in the clang side as well. Since it didn't work in the clang side, this patch achieves nothing. (You need to set the cmake policy CMP0074 for _ROOT to work.)

Enabling deps in native build here does not make sense to me.

  1. Enabling the deps for the native build are not necessary as the native build is used only for a small amount of functionality and these deps are not needed at all.

I may misunderstand certain details occasionally, but if I only ever did things with total certainty, I'd get nothing done. So I try out hypotheses.

Then, please don't give ultimatums like "review in 24 hours or I'll merge". I don't appreciate comments like that.

h-vetinari commented 1 year ago

I may misunderstand certain details occasionally, but if I only ever did things with total certainty, I'd get nothing done. So I try out hypotheses.

Then, please don't give ultimatums like "review in 24 hours or I'll merge". I don't appreciate comments like that.

OK. Can we agree that there are degrees of severity of the changes in any given PR? For high impact things, I'll wait as long as necessary (like in the case of libcxx 15, months), for medium impact things, I'll try to get a response for at least a week or two, but for low-impact changes that have no risk of breakage, I feel that "merge in 24h if there are no comments[^1]" is a decent trade-off between people being busy and drowning in notifications on the one hand, and keeping some forward momentum on the other.

And doubly so for a build that's only going to an rc label... I just don't see the harm TBH; if you happen to be unavailable during those 24h and tell me to fix something after a merge, I'll happily do it. From my POV, the worst case is 1-2 extra CI runs, and that's it. The alternative is constantly pinging (annoying you or others on trivial matters), and lots more waiting (I've had my share of PRs where I didn't get responses for weeks), and there has to be some degree of autonomy here, otherwise things just blow up, because it concentrates too much responsibility/work on too few people.

[^1]: In particular, it's not necessary to review, just saying "wait" if something isn't right would be enough.