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.

Sign up for GitHub

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

Jump to bottom

bash-kernel: fix and add smoke test #311507

Merged
merged 1 commit into from
May 15, 2024
Merged

bash-kernel: fix and add smoke test #311507

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions pkgs/development/python-modules/bash-kernel/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
, ipykernel
, python
, pexpect
, bash
, bashInteractive
, substituteAll
}:

Expand All @@ -24,7 +24,7 @@ buildPythonPackage rec {
patches = [
(substituteAll {
src = ./bash-path.patch;
bash = lib.getExe bash;
bash = lib.getExe bashInteractive;
})
];

Expand All @@ -45,8 +45,20 @@ buildPythonPackage rec {
${python.pythonOnBuildForHost.interpreter} -m bash_kernel.install --prefix $out
'';

# no tests
doCheck = false;
checkPhase = ''
runHook preCheck

# Create a JUPYTER_PATH with the kernelspec
thomasjm marked this conversation as resolved.
Show resolved Hide resolved
export JUPYTER_PATH=$(mktemp -d)
mkdir -p $JUPYTER_PATH/kernels/bash
echo '{ "language": "bash", "argv": [ "${python}/bin/python", "-m", "bash_kernel", "-f", "{connection_file}" ] }' > $JUPYTER_PATH/kernels/bash/kernel.json

# Evaluate a test notebook with papermill
cd $(mktemp -d)
${python.withPackages (ps: [ps.papermill])}/bin/papermill --kernel bash ${./test.ipynb} out.ipynb
Copy link
Member

Choose a reason for hiding this comment

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

so this would fail without bashInteractive ?
I wonder if it should go through passtru.tests or in checkPhase since papermill becomes a new dependency ? without your fixes, the kernel still works but with a funky prompt ? (sry I read a bit fast)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The kernel is totally broken without bashInteractive. It crashes right away with an error about being unable to parse the prompt.

I don't know about the current best practices regarding passthru.tests. It seems like a good idea to always run a smoke check to prevent regressions like the one we currently have.

If we do the checkPhase option, Papermill would only be a build-time dependency right? So users getting it from the Nixpkgs cache wouldn't experience any change.


runHook postCheck
'';

meta = with lib; {
description = "Bash Kernel for Jupyter";
Expand Down
26 changes: 26 additions & 0 deletions pkgs/development/python-modules/bash-kernel/test.ipynb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
{
"cells": [
{
"cell_type": "code",
"execution_count": null,
"metadata": {},
"outputs": [],
"source": [
"echo hi"
]
}
],
"metadata": {
"kernel_info": {
"display_name": "Unknown",
"name": "bash"
},
"language_info": {
"file_extension": ".ipynb",
"name": "bash",
"version": "5.0"
}
},
"nbformat": 4,
"nbformat_minor": 2
}