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

cling, xeus-cling: vendor llvm #283989

Closed
wants to merge 2 commits into from
Closed

cling, xeus-cling: vendor llvm #283989

wants to merge 2 commits into from

Conversation

wegank
Copy link
Member

@wegank wegank commented Jan 26, 2024

Description of changes

This PR attempts to build Cling with root's LLVM fork, as the packages involved are the last blockers for the removal of LLVM 8-11 in Nixpkgs (see #283954). The refactor basically follows https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=cling.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab label Jan 26, 2024
@ofborg ofborg bot requested a review from thomasjm January 26, 2024 12:26
@wegank wegank force-pushed the cling-llvm branch 2 times, most recently from 5ec4e99 to 09636ac Compare January 26, 2024 13:47
@lorenz
Copy link
Contributor

lorenz commented Jan 26, 2024

FYI there seems to be a small patchset which allows using LLVM 17 with xeus-cling: jupyter-xeus/xeus-cling@main...lackhole:xeus-cling:llvm-16

@wegank
Copy link
Member Author

wegank commented Jan 26, 2024

FYI there seems to be a small patchset which allows using LLVM 17 with xeus-cling: jupyter-xeus/xeus-cling@main...lackhole:xeus-cling:llvm-16

I guess this won't work unless Cling is based on LLVM 16...

@ghost ghost mentioned this pull request Jan 26, 2024
@wegank wegank marked this pull request as ready for review January 27, 2024 01:06
@wegank
Copy link
Member Author

wegank commented Jan 27, 2024

Result of nixpkgs-review pr 283989 run on aarch64-darwin 1

2 packages built:
  • cling
  • xeus-cling

@thomasjm
Copy link
Contributor

A few thoughts here, it's been a few years so I don't remember everything perfectly:

  • Cling is pretty tightly bound to a given LLVM release, in this case LLVM 9. I don't think you're going to have luck trying to build Cling with a totally different LLVM version.
  • Building with Root's LLVM fork could potentially work, but why? You'll just have replaced the current Nixpkgs build of LLVM 9 with another (slightly weird/patched) version.
  • Over its history, Cling has sometimes required patches to both LLVM and Clang. IIRC, an earlier version of this derivation used a vendored LLVM. But this was not preferred, because then Hydra had to build another whole other copy of LLVM. It's more efficient to use Nixpkgs' own LLVM build when possible.

In short, I don't think it'll be easy to get rid of LLVM 9. It might be worth getting in touch with the Cling team to see if a new release might be coming soon.

@ghost
Copy link

ghost commented Jan 27, 2024

Result of nixpkgs-review pr 283989 run on x86_64-linux 1

2 packages built:
  • cling
  • xeus-cling

Result of nixpkgs-review pr 283989 run on x86_64-darwin 1

2 packages built:
  • cling
  • xeus-cling

@ghost
Copy link

ghost commented Jan 27, 2024

A few thoughts here, it's been a few years so I don't remember everything perfectly:

* Cling is pretty tightly bound to a given LLVM release, in this case LLVM 9. I don't think you're going to have luck trying to build Cling with a totally different LLVM version.

it builds -- do you have any suggestions on a good way to test? note that the current darwin build is generating errors when run (missing incudes) and this PR seems to get further but generates different errors (finds includes but has unresolved symbols). linux can include iostream and write to cout. that's the extent i've tested.

* Building with Root's LLVM fork could potentially work, but why? You'll just have replaced the current Nixpkgs build of LLVM 9 with another (slightly weird/patched) version.

there is a maintenance burden. LLVM9 is different enough from newer LLVMs that the toolchain requires a bunch of special cases for everything to work. there are some darwin bugs with the current toolchains i'm working on and not having to support older toolchains will make the changes and the code simpler. there is a cost to just leaving things as is.

* Over its history, Cling has sometimes required patches to both LLVM and Clang. IIRC, an earlier version of this derivation used a vendored LLVM. But this was not preferred, because then Hydra had to build another whole other copy of LLVM. It's more efficient to use Nixpkgs' own LLVM build when possible.

given cling is the only package using LLVM9 it already is requiring Hydra to build a whole other LLVM.

In short, I don't think it'll be easy to get rid of LLVM 9. It might be worth getting in touch with the Cling team to see if a new release might be coming soon.

a new version was released in December.

chmod -R u+w include
git apply ${./fix-llvm-include.patch}
'';
version = "0.9";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be 1.0 to pick up the latest support. We may need to ping the Cling team to get them to cut a Git release with a tag.

@thomasjm
Copy link
Contributor

thomasjm commented Jan 27, 2024

a new version was released in december.

Oh, really? They didn't tag it on GitHub. That would change things.

I went digging around and found this: https://github.com/vgvassilev/cling/blob/v1.0/docs/ReleaseNotes.md. It indicates you can build Cling with LLVM 13.

Somewhat confusingly, the build instructions on this page mention a LLVM branch called cling-llvm16 🤔

do you have any suggestions on a good way to test?

A good start would be to run the full sample Jupyter notebook provided by xeus-cling: https://github.com/jupyter-xeus/xeus-cling/blob/main/notebooks/xcpp.ipynb.

@wegank
Copy link
Member Author

wegank commented Jan 27, 2024

They did tag the release, it was just oddly announced in vgvassilev's repo.

Also, the official instruction is always to build in-tree with their own fork of LLVM, and building with llvmPackages_13 is likely broken and unsupported anyway (vgvassilev/cling#230).

@ghost
Copy link

ghost commented Jan 27, 2024

a new version was released in december.

Oh, really? They didn't tag it on GitHub. That would change things.

I went digging around and found this: https://github.com/vgvassilev/cling/blob/v1.0/docs/ReleaseNotes.md. It indicates you can build Cling with LLVM 13.

I was messing around yesterday and put together the derivation below. i got it to build and hello world worked but was pulling header files from my system so still needed work (only tested on x64 linux)

default.nix
{ lib
, stdenv
, cmake
, fetchFromGitHub
, ninja
, python3
, ncurses
, zlib
}:
let
  clingRepo = "cling";
  llvmRepo = "llvm-project";
in
stdenv.mkDerivation (finalAttrs: {
  pname = "cling2"; 
  version = "1.0";
  srcs = let owner = "root-project"; in [
    (fetchFromGitHub {
      inherit owner; 
      repo = clingRepo;
      rev = "refs/tags/v${finalAttrs.version}";
      hash = "sha256-Ye8EINzt+dyNvUIRydACXzb/xEPLm0YSkz08Xxw3xp4=";
      name = clingRepo;
    })
    (fetchFromGitHub {
      inherit owner; 
      repo = llvmRepo;
      #rev = "refs/tags/cling-llvm16-20231130-01";
      #hash = "sha256-h/cstR1z6SLooVcNUvT2FsY53bBsZbU81Wwi9AUZbOIr";
      rev = "refs/tags/cling-llvm13-20231016-01";
      hash = "sha256-Unw5RzYcVYJsViaM9t+QhuBXzTY/r6qo9TDzwZkFi5c=";
      name = llvmRepo;
    })
  ];  
  sourceRoot = ".";
  postUnpack = ''
    chmod u+w -R .
  '';
  strictDeps = true;
  nativeBuildInputs = [
    cmake
    ninja
    python3
  ];
  buildInputs = [
    ncurses
    zlib
  ];
  cmakeDir = "../${llvmRepo}/llvm";
  cmakeFlags = [
    "-DLLVM_BUILD_TOOLS=OFF"
    "-DLLVM_ENABLE_PROJECTS=clang"
    "-DLLVM_EXTERNAL_CLING_SOURCE_DIR=../${clingRepo}"
    "-DLLVM_EXTERNAL_PROJECTS=cling"
    "-DLLVM_TARGETS_TO_BUILD=host;NVPTX"
  ];
  meta = with lib; {
    description = "The Interactive C++ Interpreter";
    homepage = "https://root.cern/cling/";
    license = with licenses; [ lgpl21 ncsa ];
    maintainers = with maintainers; [ thomasjm ];
    platforms = platforms.unix;
  };
})  

Somewhat confusingly, the build instructions on this page mention a LLVM branch called cling-llvm16 🤔

using the latest cling rev on main and the the latest cling-llvm16 from the llvm fork will build in the above derivation and hello world works.

I saw @wegank working on this PR and was hoping it would solve all our problems.

do you have any suggestions on a good way to test?

A good start would be to run the full sample Jupyter notebook provided by xeus-cling: https://github.com/jupyter-xeus/xeus-cling/blob/main/notebooks/xcpp.ipynb.

thanks!

@wegank
Copy link
Member Author

wegank commented Jan 27, 2024

using the latest cling rev on main and the the latest cling-llvm16 from the llvm fork will build in the above derivation and hello world works.

Oh, great! Would you like to create a PR? I'll close mine in favour of yours.

@thomasjm
Copy link
Contributor

Usually when there's uncertainty about how to build I've pinged some Cling people. Historically this has been @vgvassilev or @Axel-Naumann -- any chance one of you could help us pick which version of LLVM to use with Cling these days?

Like I mentioned, if we have to choose between a vendored LLVM and using one that Nixpkgs contains anyway, we usually prefer the latter.

@thomasjm
Copy link
Contributor

the official instruction is always to build in-tree with their own fork of LLVM, and building with llvmPackages_13 is likely broken and unsupported anyway (vgvassilev/cling#230).

Ah I see. Well, we've gotten by with unpatched LLVM 9 so far ¯\_(ツ)_/¯. IIRC we did that with Cling maintainer blessing but maybe things have changed.

@ghost
Copy link

ghost commented Jan 27, 2024

using the latest cling rev on main and the the latest cling-llvm16 from the llvm fork will build in the above derivation and hello world works.

Oh, great! Would you like to create a PR? I'll close mine in favour of yours.

it's not very far along. I can make it a WIP but your PR seems like it is further along. i haven't tried to integrate with xeus-cling.

@ghost ghost mentioned this pull request Jan 27, 2024
13 tasks
@ghost
Copy link

ghost commented Jan 27, 2024

#284167

@vgvassilev
Copy link

We should use cling v1.0 and llvm13. In principle we can use vanilla llvm but we need our clang because it contains patches that make cling work.

@thomasjm
Copy link
Contributor

Thanks @vgvassilev!

Could you tell me where to find the necessary version of Clang? In the past I've used the tagged commits from the repo at http://root.cern/git/clang.git. But this lacks a cling-v1.0 tag -- the latest available is cling-v0.9.

Also, would it be possible to create a GitHub release for the new Cling 1.0 on https://github.com/root-project/cling ?

@vgvassilev
Copy link

Thanks @vgvassilev!

Could you tell me where to find the necessary version of Clang? In the past I've used the tagged commits from the repo at http://root.cern/git/clang.git. But this lacks a cling-v1.0 tag -- the latest available is cling-v0.9.

We have switched to the llvm monorepo: https://github.com/root-project/llvm-project/ the tag that goes with v1.0 is cling-llvm13. @hahnjo, we might need to create an alias for this tag cling-v1.0 which will have more meaningful relationship between the cling version and the llvm version...

Also, would it be possible to create a GitHub release for the new Cling 1.0 on https://github.com/root-project/cling ?

Done.

@thomasjm thomasjm mentioned this pull request Jan 29, 2024
13 tasks
@RossComputerGuy
Copy link
Member

I'm wondering what the progress on this is, I see the linux targets pass but not the darwin ones.

@wegank
Copy link
Member Author

wegank commented Apr 7, 2024

The darwin ones should build fine, they were failing on ofborg due to timeout.

@thomasjm
Copy link
Contributor

thomasjm commented Apr 8, 2024

The Linux one is not working yet, it builds but still has some include path issues to resolve.

@RossComputerGuy
Copy link
Member

The darwin ones should build fine, they were failing on ofborg due to timeout.

That's great.

The Linux one is not working yet, it builds but still has some include path issues to resolve.

Well, we should get those fixed then heh. I'm hoping we can get this merged since it's one of the things I would like to see before LLVM 19 is released. From the looks of it, LLVM 19 will be coming out in July but no rush.

@thomasjm
Copy link
Contributor

thomasjm commented Apr 9, 2024

Okay, turned out to be easier than I thought to fix the flags up. #284865 is ready for review now.

@wegank
Copy link
Member Author

wegank commented Apr 9, 2024

Superseded by #284865.

@wegank wegank closed this Apr 9, 2024
@wegank wegank deleted the cling-llvm branch April 9, 2024 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants