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

xeus-cling: init at 0.15.3 #244777

Merged
merged 1 commit into from
Nov 22, 2023
Merged

xeus-cling: init at 0.15.3 #244777

merged 1 commit into from
Nov 22, 2023

Conversation

thomasjm
Copy link
Contributor

@thomasjm thomasjm commented Jul 22, 2023

Description of changes

The Jupyter kernel for C++ based on Cling.

This is a draft until I can get #247253 and #245340 merged, and then it will be ready to go.

There are currently 2 patches included with this PR. One has already been merged upstream and the other is in progress, so hopefully we'll have a new release soon incorporating both and then we can remove the patches.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.05 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.

@thomasjm thomasjm marked this pull request as draft July 22, 2023 06:20
@thomasjm thomasjm mentioned this pull request Jul 22, 2023
12 tasks
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Jul 22, 2023
@thomasjm thomasjm changed the title xeus-cling: init at 0.15.2 xeus-cling: init at 0.15.3 Aug 5, 2023
@thomasjm thomasjm requested review from veprbl and RaitoBezarius and removed request for veprbl and RaitoBezarius August 5, 2023 01:06
@thomasjm thomasjm marked this pull request as ready for review September 8, 2023 19:15
@thomasjm thomasjm requested a review from tjni September 8, 2023 19:16
@thomasjm
Copy link
Contributor Author

thomasjm commented Sep 8, 2023

Hi @tjni, I was wondering if you might be willing to review this?

@teto teto added the 6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab label Nov 3, 2023
@thomasjm thomasjm requested a review from GTrunSec November 9, 2023 00:32
@thomasjm thomasjm removed the request for review from tjni November 16, 2023 01:11
@GTrunSec
Copy link
Contributor

@thomasjm
Did you test the nix build .\#xeus-cling locally?

error: builder for '/nix/store/md5jjwjw2wj04699m3kdpbdg4gj3i4qb-xeus-cling-0.15.3.drv' failed with exit code 2;
       last 10 log lines:
       > /build/source/include/xeus-cling/xoptions.hpp:21:24: note: copy constructor of 'argparser' is implicitly deleted
because base class 'argparse::ArgumentParser' has a deleted copy constructor
       >     struct argparser : public argparse::ArgumentParser
       >                        ^
       > /nix/store/2b054ars50sifp1b5lr7dw1s33iawizq-argparse-3.0/include/argparse/argparse.hpp:1477:3: note: 'ArgumentPar
ser' has been explicitly marked deleted here
       >   ArgumentParser(const ArgumentParser &other) = delete;
       >   ^
       > 1 error generated.
       > make[2]: *** [CMakeFiles/xeus-cling.dir/build.make:146: CMakeFiles/xeus-cling.dir/src/xmagics/executable.cpp.o] E
rror 1
       > make[1]: *** [CMakeFiles/Makefile2:166: CMakeFiles/xeus-cling.dir/all] Error 2
       > make: *** [Makefile:136: all] Error 2
       For full logs, run 'nix log /nix/store/md5jjwjw2wj04699m3kdpbdg4gj3i4qb-xeus-cling-0.15.3.drv'.

@thomasjm
Copy link
Contributor Author

No, I just rebased on master. The problem is that Nixpkgs now has version 3 of the argparse dependency, while this package needs ~2.9.

I added a commit to accomplish this, but I'm not sure if this is how it's normally done.

@GTrunSec
Copy link
Contributor

nixpkgs-review pr 244777

git worktree add /home/guangtao/.cache/nixpkgs-review/pr-244777-1/nixpkgs aba8303[15/19940] Preparing worktree (detached HEAD aba8303) Updating files: 100% (38022/38022), done. HEAD is now at aba8303 Merge pull request #267782 from kirillrdy/ruby_3_3 $ nix-env --extra-experimental-features no-url-literals --option system x86_64-linux -f --nix-path nixpkgs=/home /guangtao/.cache/nixpkgs-review/pr-244777-1/nixpkgs nixpkgs-overlays=/run/user/1000/tmplv9p4bk0 -qaP --xml --out-path --sh ow-trace --no-allow-import-from-derivation $ git merge --no-commit --no-ff 86262c024073fba280bcb377edc47f6c4bdbb288 Automatic merge went well; stopped before committing as requested $ nix-env --extra-experimental-features no-url-literals --option system x86_64-linux -f --nix-path nixpkgs=/home /guangtao/.cache/nixpkgs-review/pr-244777-1/nixpkgs nixpkgs-overlays=/run/user/1000/tmplv9p4bk0 -qaP --xml --out-path --sh ow-trace --no-allow-import-from-derivation --meta 1 package added: xeus-cling (init at 0.15.3)

$ nix build --nix-path nixpkgs=/home/guangtao/.cache/nixpkgs-review/pr-244777-1/nixpkgs nixpkgs-overlays=/run/user/1000/tm
plv9p4bk0 --extra-experimental-features nix-command no-url-literals --no-link --keep-going --no-allow-import-from-derivati
on --option build-use-sandbox relaxed -f /home/guangtao/.cache/nixpkgs-review/pr-244777-1/build.nix

Link to currently reviewing PR:
#244777

1 package built:
xeus-cling

$ /nix/store/vxx4c6gc2zgfw870b40f06dmli6ljp34-nix-2.17.0/bin/nix-shell /home/guangtao/.cache/nixpkgs-review/pr-244777-1/sh
ell.nix --nix-path nixpkgs=/home/guangtao/.cache/nixpkgs-review/pr-244777-1/nixpkgs nixpkgs-overlays=/run/user/1000/tmplv9
p4bk0

@thomasjm, Could you reset your changes to one commit? thanks

@thomasjm
Copy link
Contributor Author

Done

@thomasjm
Copy link
Contributor Author

Pinging @teto here as well, this should be ready to merge!

@GTrunSec
Copy link
Contributor

@natsukium ready to merge, thanks

@@ -17579,6 +17579,10 @@ with pkgs;
jre = jre8;
};

inherit (callPackage ../applications/editors/jupyter-kernels/xeus-cling { })
cpp11-kernel cpp14-kernel cpp17-kernel cpp2a-kernel;
xeus-cling = callPackage ../applications/editors/jupyter-kernels/xeus-cling/xeus-cling.nix { };
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I am annoyed to see those kernels at the top-level still since we dont make much of it without jupyter. Would it make sense to have the kernels in a package set like luaPackages/pythonPackages -> jupyterKernels = { xeus-cling , coq_kernel, ihaskell ... }. Is that on your roadmap. What do you think ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I did argue just now against exposing kernels at the top-level :)

It's definitely on my roadmap. I could either do it in this PR or a subsequent one. Maybe a subsequent one would be cleaner? But I'm up for whatever you prefer.

@teto
Copy link
Member

teto commented Nov 17, 2023

here are currently 2 patches included with this PR. One has already been merged upstream and the other is in progress, so hopefully we'll have a new release soon incorporating both and then we can remove the patches.

no release since then ?

@thomasjm
Copy link
Contributor Author

no release since then ?

I once waited 33 months for the maintainers to land a 10-line change :)

@teto teto merged commit cf9a7cd into NixOS:master Nov 22, 2023
9 checks passed
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 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants