Activating the gcc compiler should not set the install prefix

[vyasr]

Currently the activate-gcc.sh script sets numerous environment variables when run, including populating the CMake variable CMAKE_ARGS. While most of the contents of this variable are fine, the scripts should leave setting CMAKE_INSTALL_PREFIX to individual recipes using this script. I am proposing to remove this line so that the CMAKE_ARGS do not contain the install prefix by default.

[vyasr]

CC @jakirkham

[isuruf]

Why?

[jakirkham]

Setting this flag was causing us some issues in our build scripts.

Given recipes using CMake generally already specify CMAKE_INSTALL_PREFIX (just as --prefix is specified with ./configure), figured this didn't need to be set here, but maybe we are missing some context as to why it is needed.

[isuruf]

Setting this flag was causing us some issues in our build scripts.

How?

[jakirkham]

I'll let Vyas explain the build issue since he's dug into it more

That said, am not understanding why CMAKE_INSTALL_PREFIX needs to be specified here. Would appreciate more context on that

[isuruf]

There are dozens of build scripts with just cmake ${CMAKE_ARGS} . and changing this would break them.

[jakirkham]

Was curious how many recipes might be in this situation so did a quick git grep for CMAKE_ARGS & CMAKE_INSTALL_PREFIX separately and took the difference of the two sets coming up with the list below. Counted 121 of them total (compare this to 370 that specify both and many more that just use CMAKE_INSTALL_PREFIX).

This is more than would be comfortable to PR manually, but maybe it could be done in an automated way.

The change would be s/${CMAKE_ARGS}/${CMAKE_ARGS} -DCMAKE_INSTALL_PREFIX=${PREFIX}/g. Making the change prior to changing the compiler would be non-breaking (and there are plenty of feedstocks doing this already).

* [ ] abseil-cpp * [ ] analisi * [ ] autodiff * [ ] bear * [ ] blas * [ ] boost_mp11 * [ ] cartographer * [ ] carve * [ ] casadi * [ ] cerf * [ ] chimes-calculator * [ ] clad * [ ] clingcon * [ ] clingo-dl * [ ] clingo-lpx * [ ] colmap * [ ] cpp-terminal * [ ] cppad * [ ] cppadcodegen * [ ] crocoddyl * [ ] cudadecon * [ ] cudasirecon * [ ] cutlass * [ ] dftbplus * [ ] ecole * [ ] eigen4rkt * [ ] eigenpy * [ ] elfio * [ ] elsi * [ ] example-robot-data * [ ] folly * [ ] framel * [ ] fresnel * [ ] gfal2 * [ ] gklib * [ ] graphvite * [ ] gtsam * [ ] gwollum * [ ] hpp-fcl * [ ] hypre * [ ] icub-models * [ ] idyntree * [ ] imp * [ ] ispc * [ ] json-c * [ ] jwt-cpp * [ ] kaldi * [ ] latte * [ ] ldas-tools-al * [ ] ldas-tools-diskcacheapi * [ ] ldas-tools-diskcacheapi-swig * [ ] ldas-tools-filters * [ ] ldas-tools-frameapi * [ ] ldas-tools-frameapi-swig * [ ] ldas-tools-framecpp * [ ] ldas-tools-framecpp-swig * [ ] ldas-tools-ldasgen * [ ] ldas-tools-ldasgen-swig * [ ] ldas-tools-utilities * [ ] lfric_reader * [ ] lib3mf * [ ] libastra * [ ] libcaer * [ ] libcbor * [ ] libcnpy * [ ] libdate * [ ] libfido2 * [ ] libignition-tools * [ ] liblsl * [ ] libmamba * [ ] libqdldl * [ ] libuvc * [ ] nds2-client * [ ] nds2-client-swig * [ ] nvcc * [ ] omicron * [ ] onednn * [ ] onnx * [ ] onnxruntime * [ ] optima * [ ] optional-lite * [ ] osqp-eigen * [ ] phreeqc4rkt * [ ] pinocchio * [ ] primecount * [ ] pyccl * [ ] pycm * [ ] pycppad * [ ] pyfd * [ ] python-pdal * [ ] python-symengine * [ ] qpoases * [ ] quartet * [ ] randspg * [ ] readdy * [ ] reaktoro * [ ] rmf * [ ] safeint * [ ] scitokens-cpp * [ ] seekr2_openmm_plugin * [ ] sigtool * [ ] sirius * [ ] soapysdr-module-airspy * [ ] soapysdr-module-airspyhf * [ ] soapysdr-module-hackrf * [ ] spfft * [ ] spla * [ ] superlu_dist * [ ] svt-av1 * [ ] tango-access-control * [ ] tango-starter * [ ] tl-optional * [ ] tomlplusplus * [ ] tracktable * [ ] upv * [ ] vcpkg-tool * [ ] vetoperf * [ ] xeus_octave * [ ] yaml-cpp4rkt * [ ] ycm-cmake-modules * [ ] zlib-ng

[ocefpaf]

@vyasr can you help us understand more what is happening? What kind of issues? Thanks!

[ocefpaf]

Another use case for the install prefix being set are folks prototyping with conda. We do want that set already to avoid path confusion. Also, fixing many feedstocks instead of one does not sounds like the best strategy. @jakirkham and @vyasr is there a fix for you problem that does not require us to change multiple feedstocks and break a user experience on folks who use cmake with conda envs?

[vyasr]

The problem that we ran into may be a bug in scikit-build, and I just documented it here. Some quick background: scikit-build is a build tool that injects CMake into the various setuptools commands to facilitate better integration of Python packages with C(++) libraries and making it easier to develop extension modules with complete build systems for the Python and C(++) components. The issue appears to be a problem with the way that CMAKE_INSTALL_PREFIX is currently used there, specifically that it is being used at an internal "install" step rather than the actual intended installation step.

While our problem may be caused by unexpected behavior in scikit-build, though, I raised this discussion at @jakirkham's request because I think it is independently worth having. As @jakirkham pointed out, it may not really make sense to default the install prefix within this script. Most installations that actually care about the prefix probably should be setting the prefix on their own, and expecting a default behavior seems like it would be unexpected behavior if not for the fact that recipes have been taught to rely on it up to this point.

[ocefpaf]

Most installations that actually care about the prefix probably should be setting the prefix on their own, and expecting a default behavior seems like it would be unexpected behavior if not for the fact that recipes have been taught to rely on it up to this point.

Outside of conda-forge I kind of agree. But in in our case it does make sense b/c we do want to force packages to use that install prefix to keep things consistent. Even when prototyping locally as I mentioned above.

I believe you can unset that if you are using our compilers in another context that is not building packages for conda-forge. Would that help you?

[isuruf]

scikit-build already removes CMAKE_INSTALL_PREFIX from the environ variable CMAKE_ARGS. https://github.com/scikit-build/scikit-build/blob/0.15.0/skbuild/cmaker.py#L313

[isuruf]

While our problem may be caused by unexpected behavior in scikit-build, though, I raised this discussion at @jakirkham's request because I think it is independently worth having.

It's certainly not work having if it would break 160 feedstocks just to make it easier for one feedstock. That's incredibly selfish IMHO. You can remove CMAKE_INSTALL_PREFIX in your 1 feedstock that breaks this. How hard is that?

(Btw as John said, we can fix 160 feedstocks before we do the change, but people expect the variable now)

[vyasr]

I believe you can unset that if you are using our compilers in another context that is not building packages for conda-forge. Would that help you?

Yup, that's the workaround that I already have in place and that does resolve the issue for our use case. Thanks @ocefpaf!

It's certainly not work having if it would break 160 feedstocks just to make it easier for one feedstock. That's incredibly selfish IMHO. You can remove CMAKE_INSTALL_PREFIX in your 1 feedstock that breaks this. How hard is that?

I'm not suggesting that we make this change just to fix a single feedstock, I'm suggesting it because as a developer it would adhere more to the behavior that I would expect. That's no different than a package making a breaking API change in the belief that the breaking change improves the experience in the long run. If you disagree that this is a useful change, perhaps because you agree with @ocefpaf that in the context of conda-forge it does make sense to enforce a standard install prefix (and I do see that point) please say so.

[isuruf]

Yes, I disagree that this is a useful change. Sorry I didn't make it clear. I think what you are asking for will degrade the experience in the long run instead of improve.

Can you also comment on https://github.com/conda-forge/ctng-compiler-activation-feedstock/issues/77#issuecomment-1145110745?

[vyasr]

scikit-build already removes CMAKE_INSTALL_PREFIX from the environ variable CMAKE_ARGS. https://github.com/scikit-build/scikit-build/blob/0.15.0/skbuild/cmaker.py#L313

You are correct, we're running into this problem because we have build scripts that perform their own command line parsing and attempt to combine the environment variables with command line-provided CMAKE_ARGS. The result is then passed as a command line parameter to the scikit-build invocation, which bypasses the check that you pointed out. This is again our problem, not yours.

I'll reiterate that I'm not suggesting the change here as a way to fix our issues, but only if it improves the developer experience with conda-forge and conda-build. I'd like to hear from @jakirkham on that topic since I suspect he'll have a much more informed take than I will.

[ocefpaf]

@vyasr just to put things on perspective. When it comes to packaging there is no right answer. There are so many bells and whistles we need to adjust. For example, coming from a pure cmake perspective, I would say that what we do in conda-forge is outrageous. Really, setting your own prefix in a compiler activation!?

However, from the conda-forge packaging side, that is an option that ensure consistency in our ecosystem. I was an openSUSE packager for a while and we tried to make our recipes consistent with Fedora, jeez, you have no idea of how the build flags and env vars were messy and inconsistent. And it was all "Linux" with the same build tools!

TL;DR we appreciate your input and we want to find the solution that takes the path of least resistance (and minimum change).

[jakirkham]

Hopefully this is clear at this point, but no one is suggesting making a change without considering effects (or just to solve a particular issue). Discussion is certainly a goal here (hence why this is an issue) with an eye towards broader usability (though this came out of an odd edge case).

Think the important point to highlight is that our compilers are used outside conda-forge to build things in a way that is compatible with conda-forge either as part of development or to produce binaries downstream of conda-forge. Setting CMake flags like this complicates these efforts. Can people sed the CMAKE_ARGS or drop them altogether? Sure, but it presents an obstacle to usage.

This could also present other issues for us down the road ( in fact in the more general activation case this already has with r-base: https://github.com/conda-forge/ctng-compiler-activation-feedstock/issues/74 ). For example what if CMake makes a change that breaks these environment variables or requires another one? We will find ourselves needing to update both CMake and the compiler activation scripts.

To this end wonder if there is a better approach. For example these might make more sense in the CMake package itself (maybe in some configuration file). Alternatively maybe we can bake these things into conda-build itself (as was done with pip). Or maybe there could be a compiler("cmake") or build_tool("cmake") or similar that configures CMake usage (this might be handy if we take a similar approach with Autotools or other build tools). Any other thoughts?

[vyasr]

EDIT: This is a response to Filipe's comment two ago, looks like John and I posted at the same time.

Yeah that's very valid. Coming from a CMake perspective I was initially quite confused while debugging because the issue only occurred in a very specific instance that turns out to be our CI step that actually uses conda-build for the packaging. But I agree that packaging is in general a pretty messy issue and there's no right answer, just the least wrong one.

Using scikit-build adds another layer of potential confusion to the mix because various build steps may be making assumptions about how Python builds don't use CMake. I will add that I find the lines that @isuruf linked to in scikit-build a bit unexpected as well; while it's fine for scikit-build to respect a special environment variable SKBUILD_CONFIGURE_OPTIONS that it documents as its own, implicitly handling CMAKE_ARGS (which CMake itself would not do) seems like an easy path to confusion for builds that include both C(++) (i.e. direct invocation of CMake) and use of scikit-build via setup.py since the former requires explicitly specifying the CMAKE_ARGS while the latter will use them implicitly. That seems like a separate issue to potentially upstream here.

[isuruf]

Think the important point to highlight is that our compilers are used outside conda-forge to build things in a way that is compatible with conda-forge either as part of development or to produce binaries downstream of conda-forge. Setting CMake flags like this complicates these efforts. Can people sed the CMAKE_ARGS or drop them altogether? Sure, but it presents an obstacle to usage.

CMAKE_ARGS is an optional variable. If you don't want to use it, don't use it. It's simple as that.

None of the concerns by @jakirkham is particular towards CMAKE_INSTALL_PREFIX. Please keep this discussion relevant.

Alternatively maybe we can bake these things into conda-build itself (as was done with pip).

CMAKE_INSTALL_PREFIX is added only when CONDA_BUILD=1 is added. So, I don't see your point.

I will add that I find the lines that @isuruf linked to in scikit-build a bit unexpected as well

Please raise it with upstream. Upstream added it to support cross-compiling in conda-forge projects specifically.

[vyasr]

Opened https://github.com/scikit-build/scikit-build/issues/722 to discuss the upstream scikit-build question.