-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
cling: init at 0.7.0 #64319
cling: init at 0.7.0 #64319
Conversation
Thanks for the review! I resolved everything except for two outstanding discussions. |
Okay, second round of review responses is done. There are 3 outstanding discussions, about
|
Resolved the unpacking code one. |
Okay, all feedback has been addressed now. |
Oh, I just realized that after changing
This is probably not desirable, maybe we should go back to overriding |
What would you put in the |
Oh you're right, this problem existed before that change. I'm not sure...we definitely don't want Furthermore, the output currently contains other folders like But for all I know the What do you think about creating another derivation that wraps this one and only outputs |
I think cling doesn't need llvm or clang binaries to operate, so we could strip those away. When we build out clang and llvm in separate derivations, first build @Axel-Naumann, if you know, could you please tell us, is it possible to build |
cling needs a patched clang but can work on an original llvm. It's very sensitive to versions though - cling requires llvm 5.0. An upgrade to a newer llvm will happen still this year, but that, too, will then require a specific llvm version. We don't have the resources to maintain compatibility across llvm versions. |
@Axel-Naumann Thank you for your answer. We are not planning to compile cling with potentially incompatible versions of clang or llvm, this will only confuse and inconvenience our cling users. The actual question we have is whether we could make separate builds for llvm, clang and cling. Basically, we already know that we can do cd /src/llvm
cmake -DCMAKE_INSTALL_PATH=/foo/llvm .
make install and then use this installation of llvm when building clang: cd /src/clang
PATH=/foo/llvm/bin:$PATH cmake -DCMAKE_INSTALL_PATH=/foo/clang .
make install` Now the question is: is it okay to also build cling in a separate build? I would imagine it will work something like: cd /src/cling
PATH=/foo/llvm/bin:/foo/clang/bin:$PATH cmake -DCMAKE_INSTALL_PATH=/foo/cling .
make install The problem with building cling in-tree is that it will install llvm and clang binaries/headers/libraries, which we would not want to have in the same path. We would also like to have separate builds to improve caching, e.g. we don't want to rebuild llvm on any minor change to cling. |
Thanks, now I understand. Yes that should work: we support this for building cling as part of ROOT, against already built llvm and clang. Please let me know if things don't work as expected, or better yet: let me know which changes to CMake you needed to make it work :-) |
@Axel-Naumann Thanks again for your insight! @thomasjm Are you interested in implementing this? I think we could just reuse |
Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on NixOS (x86_64-linux)
Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
I don't suppose there's some way to parse the output of a derivation to get a list of strings back as a Nix expression? Would be nice to add |
There is, it's called Import From Derivation (aka IFD), but we generally don't use this feature in nixpkgs and Hydra will not evaluate that at all. |
Gotcha. Okay, I don't think there's anything else outstanding here. |
@GrahamcOfBorg build cling |
Thanks everyone, especially @thomasjm, @Axel-Naumann and @stephane-rolland for their efforts! This is a great work and I'm happy to see this finally getting in. Feel free to open follow up Issues and PR's if we missed something. |
thanks all! i'm super excited this is available now 🎉 |
Hooray! Thanks so much to everyone who reviewed and commented :) |
Wooohooo!! |
Motivation for this change
Add Cling to Nixpkgs. Resolves #29661.
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)