search

jupyter-xeus / xeus-cling

Jupyter kernel for the C++ programming language
BSD 3-Clause "New" or "Revised" License
3.01k stars 294 forks source link

Don't pass extra includes; configure this with flags #493

Open thomasjm opened 11 months ago

thomasjm commented 11 months ago

This fixes an issue that came up when working on Nixpkgs packaging for xeus-cling here.

It's weird for xeus-cling to stick extra arguments on to pass to the interpreter when it already passes the user-provided ones from the command line. The user is perfectly capable of adding this LLVM include directory if desired.

JohanMabille commented 11 months ago

This sounds like asking developers to add the include directories of the standard library on a regular project. I'm not sure that would be convenient for regular users. On the other hand, I totally understand the issue for packaging xeus-cling; I think a conditional compilation could help here, but maybe @SylvainCorlay has a better idea?

By the way, if you remove the extra arg, you can also remove the extra interpreter_args and pass argc and argv directly to the interpreter.

thomasjm commented 11 months ago

By the way, if you remove the extra arg, you can also remove the extra interpreter_args and pass argc and argv directly to the interpreter.

Not quite, as it's still changing the process name in the 0th argument.

This sounds like asking developers to add the include directories of the standard library on a regular project. I'm not sure that would be convenient for regular users.

Do regular users actually invoke xcpp manually? My impression was that xeus-cling is packaged as part of Anaconda somehow and that such flags are handled there. (I'd love to see how, if you happen to know where that happens.)

Also, which standard library do you mean? In my experience of packaging cling and xeus-cling a number of things are needed, such as the C standard library, the C++ standard library, the includes and library paths for Cling itself, etc.

It wasn't clear to me what the purpose of this extra argument was, as it doesn't come with a flag before it such as -I or -isystem.

SylvainCorlay commented 11 months ago

Do regular users actually invoke xcpp manually? My impression was that xeus-cling is packaged as part of Anaconda somehow and that such flags are handled there. (I'd love to see how, if you happen to know where that happens.)

Pardon my brevity as I am commenting from my phone.

I think it would be fine to remove this and pass the command line argument from the kernelspec. @thomasjm, this is specified in the kernel.json.in file in this repository.

SylvainCorlay commented 11 months ago

@thomasjm here is such a kernel spec file: https://github.com/jupyter-xeus/xeus-cling/blob/main/share/jupyter/kernels/xcpp14/kernel.json.in

thomasjm commented 11 months ago

Thanks! I just tried this in the latest commit. I haven't tested it yet, had a little trouble using conda on NixOS.

SylvainCorlay commented 11 months ago

I don't think we actually define LLVM_DIR as a cmake variable.

thomasjm commented 11 months ago

Okay, how about using LLVM_MAIN_INCLUDE_DIR? Based on my reading of CMakeLists.txt, this is the most proper variable to use:

https://github.com/jupyter-xeus/xeus-cling/blob/471b1b68e3cad35746652cf14ce40ce0b7797ba0/CMakeLists.txt#L98

Incidentally, the variable called LLVM_BINARY_DIR, which ends up being passed to the preprocessor as -DLLVM_DIR, is rather confusingly named since it's actually the LLVM prefix, i.e. the result of calling llvm-config --prefix.

thomasjm commented 11 months ago

Okay, I was able to get a Conda environment running on a normal Ubuntu machine and test this out. With the latest commit it seems to work!