Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

thomasjm
Copy link
Contributor

@thomasjm thomasjm commented Aug 5, 2023

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
Copy link
Member

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
Copy link
Contributor Author

thomasjm commented Aug 7, 2023

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
Copy link
Member

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
Copy link
Member

@thomasjm
Copy link
Contributor Author

thomasjm commented Aug 9, 2023

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

@SylvainCorlay
Copy link
Member

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

@thomasjm
Copy link
Contributor Author

thomasjm commented Aug 9, 2023

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

set(LLVM_MAIN_INCLUDE_DIR ${INCLUDE_DIR} CACHE PATH "Path to llvm/include")

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
Copy link
Contributor Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants