[bug] Hard coded environment variables

[AntoinePrv]

Looking at a kernel.json from a Conda-forge install, I see the following.

"env": {
    "PATH":"/Users/runner/miniforge3/conda-bld/bld/rattler-build_xeus-cpp_1760716326/build_env/bin:/Users/antoine/workspace/github.com/apache/arrow/.pixi/envs/jupyter/bin:/Users/runner/miniforge3/bin:/Users/runner/miniforge3/condabin:/usr/local/lib/ruby/gems/3.3.0/bin:/usr/local/opt/ruby@3.3/bin::/usr/local/bin:/usr/local/sbin:/Users/runner/bin:/Library/Frameworks/Python.framework/Versions/Current/bin:/Library/Frameworks/Mono.framework/Versions/Current/Commands:/usr/bin:/bin:/usr/sbin:/sbin:/Users/runner/.dotnet/tools",
    "LD_LIBRARY_PATH":""
}

The first is an artifact of the build system, which is useless for the user, and the second is just empty.
When using the kernel, I see the same value and my own values of PATH and LD_LIBRARY_PATH are ignored.
A direct impact of this is that dynamic libraries must be linked with an absolute path, which is not very portable.

In fact this is working as designed given the placeholder:

xeus-cpp/share/jupyter/kernels/xc17/kernel.json.in

Line 5 in c357fd6

"LD_LIBRARY_PATH":"@XEUS_CPP_LD_LIBRARY_PATH@"

And the default values.

xeus-cpp/CMakeLists.txt

Line 128 in c357fd6

set(XEUS_CPP_LD_LIBRARY_PATH "$ENV{LD_LIBRARY_PATH}")

Personally I don't see the point of setting these values from what is available at build time. I would simply remove it and let the runtime values apply by default. If the feature is still needed (maybe for conda), I think it should default in CMake to ${LD_LIBRARY_PATH} (instead of $Env), so that it is resolved at runtime and set to ${PREFIX}/lib:\${LD_LIBRARY_PATH}.

[AntoinePrv]

CC @anutosh491 @JohanMabille

[mcbarton]

@AntoinePrv Although LD_LIBRARY_PATH is currently blank, its needed to get openmp kernels working. You can see it being put to use in this PR #389 . I cannot remember exactly why the PATH environment variable is defined in the kernel, but @vgvassilev might be able to give you some context. I think it was determined to be needed in some way for xeus-clang-repl.

[AntoinePrv]

@mcbarton thanks for the feedback. IMHO this is not the proper solution. It will eventually create a combinatorial explosion of the number of kernels (to be fair I don't know enough about openMP to really know if it requires dedicated compilation options). E.g. having magic like %openmp, %c++17 %future compilation option seems like a better user experience to me, just like on https://godbolt.org/, there is only one language and a way to configure options (I don't know enough about the internal of this project to know how feasible that is).

Back to environment variables for the matter at hand, I think this is solved at the wrong level. Defining what goes into LD_LIBRARY_PATH is a matter of packaging, not of the source code. IMHO CMake projects tend to do too much (due to lack of good C++ packaging solution) and if someone install from source, configuring these things is a reasonable requirement. At the minimum all options should be fully configurable during installation.

A sufficient patch from conda-forge/xeus-cpp-feedstock#35 is to make all relevant CMake variable as CACHE so they can be overridden from the CLI. Would that be an acceptable patch?

[vgvassilev]

@mcbarton thanks for the feedback. IMHO this is not the proper solution. It will eventually create a combinatorial explosion of the number of kernels (to be fair I don't know enough about openMP to really know if it requires dedicated compilation options). E.g. having magic like %openmp, %c++17 %future compilation option seems like a better user experience to me, just like on https://godbolt.org/, there is only one language and a way to configure options (I don't know enough about the internal of this project to know how feasible that is).

We really need a "box" where we can say how kernels should start. It's not feasible to do what we do currently and also not really a good idea to expect to have magics for all compiler flags (just do clang -cc1 --help)

Back to environment variables for the matter at hand, I think this is solved at the wrong level. Defining what goes into LD_LIBRARY_PATH is a matter of packaging, not of the source code. IMHO CMake projects tend to do too much (due to lack of good C++ packaging solution) and if someone install from source, configuring these things is a reasonable requirement. At the minimum all options should be fully configurable during installation.

I agree that's not the best approach. I was hoping to land #389 to make sure the openmp is covered and refactor it. Right now that's the main motivation for holding back.

A sufficient patch from conda-forge/xeus-cpp-feedstock#35 is to make all relevant CMake variable as CACHE so they can be overridden from the CLI. Would that be an acceptable patch?

Can we do it after we land #389?

[AntoinePrv]

Can we do it after we land #389?

Absolutely, I subscribed myself to that other PR.