Skip to content

fish: ensure pexpect available for tests#461779

Closed
jeafleohj wants to merge 2 commits intoNixOS:masterfrom
jeafleohj:fish-pexpect-fix
Closed

fish: ensure pexpect available for tests#461779
jeafleohj wants to merge 2 commits intoNixOS:masterfrom
jeafleohj:fish-pexpect-fix

Conversation

@jeafleohj
Copy link
Contributor

@jeafleohj jeafleohj commented Nov 15, 2025 •
edited
Loading

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. labels Nov 15, 2025
@jeafleohj
Copy link
Contributor Author

jeafleohj commented Nov 15, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 461779
Commit: 34ce8ffb49aa38c69483f5584d0fe7881e2bd81a

aarch64-darwin

✅ 28 packages built:
  • bat-extras.batdiff
  • bat-extras.batgrep
  • bat-extras.batman
  • bat-extras.batpipe
  • bat-extras.batwatch
  • bat-extras.core
  • bat-extras.prettybat
  • direnv
  • fish
  • fish-lsp
  • fish.doc
  • fishMinimal
  • fishMinimal.doc
  • fishPlugins.bass
  • fishPlugins.done
  • fishPlugins.fishtape
  • fishPlugins.fishtape_3
  • fishPlugins.humantime-fish
  • fishPlugins.pure
  • fzf-git-sh
  • kitty
  • kitty.kitten
  • kitty.shell_integration
  • kitty.terminfo
  • mise
  • oh-my-fish
  • tests.writers.bin.fish
  • tests.writers.simple.fish

winterqt
winterqt reviewed Nov 15, 2025
View reviewed changes
Copy link
Member

@winterqt winterqt left a comment

Choose a reason for hiding this comment

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

Thanks! Did you end up finding the root cause, or should I spend some time doing that? (Happy to.)

pkgs/by-name/fi/fish/package.nix
preConfigure = ''
patchShebangs ./build_tools/git_version_gen.sh
patchShebangs ./tests/test_driver.py
substituteInPlace ./tests/test_driver.py --replace-warn '"python3",' '"${testPython}/bin/python3",'
Copy link
Member

@winterqt winterqt Nov 15, 2025

Choose a reason for hiding this comment

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

Why is this required on Darwin and not Linux?

Copy link
Contributor Author

@jeafleohj jeafleohj Nov 15, 2025

Choose a reason for hiding this comment

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

On macOS the build runs unsandboxed, so python3 resolves to /usr/bin/python3 instead of the nixpkgs one; pointing to ${testPython}/bin/python3 ensures we always use the interpreter that has pexpect.

Copy link
Member

@szlend szlend Nov 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

But patching shebangs in ./tests/test_driver.py should point it to the nixpkgs python, no? I think something else is going on. Are nativeCheckInputs available in preConfigure (for patchShebangs to pick up)?

Maybe moving python to nativeBuildInputs would fix it. Or moving test shebang patching to the check phase.

Copy link
Member

@gador gador Nov 16, 2025

Choose a reason for hiding this comment

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

It's propably due to a hardcoded subprocess. It seems it finds PEXPECT (due to patchShebangs and python3 haveing PEXPECT available) and then calling a subprocess with python3 which does not resolve to the sandboxed python environment with PEXPECT but to the system python3:

https://github.com/fish-shell/fish-shell/blob/289057f981cc6ff125bfbde87e798027eb69fa88/tests/test_driver.py#L336-L346

@ddogfoodd ddogfoodd mentioned this pull request Nov 15, 2025
Merged
13 tasks
@gador
Copy link
Member

gador commented Nov 15, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 461779
Commit: 6e211d9ab5a4219654e872af1675b143fc01d827

x86_64-linux

✅ 30 packages built:
  • bat-extras.batdiff
  • bat-extras.batgrep
  • bat-extras.batman
  • bat-extras.batpipe
  • bat-extras.batwatch
  • bat-extras.core
  • bat-extras.prettybat
  • direnv
  • fish
  • fish-lsp
  • fish.doc
  • fishMinimal
  • fishMinimal.doc
  • fishPlugins.bass
  • fishPlugins.done
  • fishPlugins.fishtape
  • fishPlugins.fishtape_3
  • fishPlugins.fzf-fish
  • fishPlugins.humantime-fish
  • fishPlugins.pure
  • fzf-git-sh
  • kitty
  • kitty.kitten
  • kitty.shell_integration
  • kitty.terminfo
  • mise
  • oh-my-fish
  • swaynotificationcenter
  • tests.writers.bin.fish
  • tests.writers.simple.fish

aarch64-darwin

✅ 28 packages built:
  • bat-extras.batdiff
  • bat-extras.batgrep
  • bat-extras.batman
  • bat-extras.batpipe
  • bat-extras.batwatch
  • bat-extras.core
  • bat-extras.prettybat
  • direnv
  • fish
  • fish-lsp
  • fish.doc
  • fishMinimal
  • fishMinimal.doc
  • fishPlugins.bass
  • fishPlugins.done
  • fishPlugins.fishtape
  • fishPlugins.fishtape_3
  • fishPlugins.humantime-fish
  • fishPlugins.pure
  • fzf-git-sh
  • kitty
  • kitty.kitten
  • kitty.shell_integration
  • kitty.terminfo
  • mise
  • oh-my-fish
  • tests.writers.bin.fish
  • tests.writers.simple.fish

@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 1 This PR was reviewed and approved by one person. 2.status: merge-bot eligible This PR can be merged by commenting "@NixOS/nixpkgs-merge-bot merge". labels Nov 15, 2025
@matteo-pacini matteo-pacini added this pull request to the merge queue Nov 15, 2025
@winterqt winterqt removed this pull request from the merge queue due to a manual request Nov 15, 2025
@winterqt
Copy link
Member

winterqt commented Nov 15, 2025

After digging into this more, I have a slightly cleaner solution about ready.

@ilyagr ilyagr mentioned this pull request Nov 16, 2025
Closed
3 tasks
@siraben
Copy link
Member

siraben commented Nov 17, 2025

fish transitively causes direnv to fail to build on darwin

$ PAGER=cat nix why-depends --derivation github:NixOS/nixpkgs#direnv github:NixOS/nixpkgs#fish 
/nix/store/2yjg6yfdqgxhjyzsjddb9zv113dph54g-direnv-2.37.1.drv
└───/nix/store/xbcj54iv1y948j6qdqn1bpbwk9cmq9zh-fish-4.2.1.drv

@siraben siraben mentioned this pull request Nov 17, 2025
Closed
@gador
Copy link
Member

gador commented Nov 17, 2025

After digging into this more, I have a slightly cleaner solution about ready.

Do you already have something or should we merge this to fix builds on darwin?

@adamcstephens adamcstephens mentioned this pull request Nov 17, 2025
Merged
13 tasks
@jeafleohj jeafleohj deleted the fish-pexpect-fix branch November 17, 2025 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gador gador gador approved these changes

@winterqt winterqt winterqt left review comments

@adamcstephens adamcstephens Awaiting requested review from adamcstephens

@cole-h cole-h Awaiting requested review from cole-h

@lonerOrz lonerOrz Awaiting requested review from lonerOrz

@r-vdp r-vdp Awaiting requested review from r-vdp

@SigmaSquadron SigmaSquadron Awaiting requested review from SigmaSquadron

+1 more reviewer

@szlend szlend szlend left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

2.status: merge-bot eligible This PR can be merged by commenting "@NixOS/nixpkgs-merge-bot merge". 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jeafleohj @gador @winterqt @siraben @szlend