Conversation
|
Okay, I just pushed another commit to patch |
thomasjm
force-pushed
the
cling-1.2-final
branch
from
ab36de7 to
6608142
Compare
nixpkgs-ci
bot
added
10.rebuild-linux: 1-10
|
Okay yeah, the tests I'm running against |
emilazy
left a comment
emilazy
left a comment
There was a problem hiding this comment.
Thanks, this looks good to me and seems to work fine on Darwin! Happy to merge when you’re satisfied with the xeus-cling changes, as I have not tested that.
I hope that some of the stuff in #439740 can be of use in future to potentially simplify the derivation here and make it more robust to changes in our wrappers (e.g. we’re moving to using the system libc++ on Darwin, which might require adjustments here). But this meets my primary goal of getting the old LLVM versions out of Nixpkgs, so I am very happy with it, thank you! :)
pkgs/by-name/cl/cling/package.nix
Outdated
There was a problem hiding this comment.
These should probably use stdenv.cc.libcxx, as the libc++ libraries are backwards‐compatible with older headers but mixing the library versions is a bad idea. (Not a blocker, as it’s not a new problem.)
There was a problem hiding this comment.
You mean llvmPackages_18.libcxx -> stdenv.cc.libcxx? That doesn't seem like a good idea considering these are library flags. And also that we deliberately use clangStdenv above.
There was a problem hiding this comment.
clangStdenv doesn’t imply the use of libstdc++ vs. libc++, only the C compiler; it keeps the default C++ library for the platform. (libcxxStdenv explicitly picks the combination of Clang and libc++.)
The C++ standard library is ABI‐sensitive, so mixing different versions of libc++, or libstdc++ with libc++, can cause compatibility issues. However, libc++’s headers only work with a specific range of Clang versions, but can be used to target later versions of the library. So it is correct to use a specific include directory here, but you generally want to ensure the use of the same version to actually link with.
That’s why it probably doesn’t make sense to have a separate useLLVMLibcxx here, as opposed to conditioning on the platform (and hence why I dropped it in my PR). So, in that case, stdenv.cc.libcxx would be non‐null exactly when the platform uses libc++, and you’d want to link with that version while using the headers from Cling’s version of LLVM.
Anyway, this is all stuff you don’t have to think about if you reuse our existing wrapper logic, ideally :) And in this case hopefully the things Cling builds don’t link with much, so the scope for problems is limited. But generally linking against a specific C++ standard library isn’t too reliable (and indeed problems with mixing them in obscure cases are what is driving us to switch to linking against the system‐provided library from the SDK on Darwin).
nixpkgs-ci
bot
added
the
12.approvals: 1
thomasjm
force-pushed
the
cling-1.2-final
branch
from
6608142 to
4cbeead
Compare
Huh. I think that if I wasn't familiar with I'm not sure I understand what this part is doing, it looks like it's trying to pretend Cling is a compiler? |
|
I just re-ran my |
|
This closes #338370 btw. |
Yeah, it’s a pain to rebuild Clang. OTOH, splitting out the LLVM build reduces that to an extent – any changes that only affect the Cling package don’t rebuild LLVM/Clang. The issue isn’t with
Well, it basically is a compiler – it reuses Clang’s frontend, and the flags passed along to it are very similar to the ones we pass along to Clang to get the required include directories and libraries set up. So the idea is that you can ideally use the same wrapper to get the flags you need, and remove the platform‐specific logic and complexity that currently has to live in the Cling derivation. (The rename to Unfortunately… Cling’s insistence on trying to find header include paths however it can, and weird Cling behaviour that seems buggy to me, got in the way of making this work totally smoothly before seeing that you were also working on the update :) It would also require some adaptation to work with All that said, this PR currently works and mine certainly doesn’t, and I’m very happy to merge in the current state :) Thanks for working on this! |
|
No problem, thanks for explaining! I'll look into using these ideas next time Cling needs work. |
2 participants
This actually makes the derivation cleaner because upstream improved the build process somewhat.
Also fixes an issue on Darwin where the Jupyter kernel wasn't being copied to the output.
Putting up for review;
it's not ready to merge yet because updating LLVM breaksxeus-cling. I'm working on patching it now.CC @emilazy
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.