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

byobu: add screen backend #90430

Merged
merged 1 commit into from Oct 27, 2020
Merged

byobu: add screen backend #90430

merged 1 commit into from Oct 27, 2020

Conversation

@berbiche
Copy link
Member

@berbiche berbiche commented Jun 15, 2020

Motivation for this change

Screen backend was missing.

[nix-shell:~/dev/nixpkgs/result]$ byobu-screen - -v
Screen version 4.08.00 (GNU) 05-Feb-20

Closure size:

before: /nix/store/44rqjxl15zqlizgkwnxjxl8428asjm57-byobu-5.133	 156.1M
after:  /nix/store/a2lzmr53zvjbg9d5jdcxsj4v1nfvmc0f-byobu-5.133	 226.6M
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@teto
Copy link
Contributor

@teto teto commented Jun 17, 2020

missing byobu-config seems like a bother (not a byobu user) ? would you feel like adding it as well ? it shouldn't increase closure size too much.

@berbiche
Copy link
Member Author

@berbiche berbiche commented Jun 18, 2020

The missing dependency is part of the package newt.

The package includes a python library (a single python file snack.py) that is not available in the package output.

I'm not sure how to make this library available to byobu-config.

@berbiche
Copy link
Member Author

@berbiche berbiche commented Jun 18, 2020

My understanding:

  • The newt derivation will have to be modified to compile the snack.so required by byobu-config. This snack.so is compiled against Python.h.
  • The snack.so along with libnewt.so will have to be available in byobus environment.
    I don't know whether including newt is enough for the proper environment variables to be available to byobu-config.

Update 1:

  • I added PYTHONVERS=${python3.pname}/ to newts makeFlags. The snack.so is now compiled and available in lib/python3/site-packages
    I now need to find a way to have the site-packages in Byobu's PYTHONPATH.

pkgs/tools/misc/byobu/default.nix Outdated Show resolved Hide resolved
@berbiche
Copy link
Member Author

@berbiche berbiche commented Jun 24, 2020

I think I could just use the patches from: #22774

pkgs/development/libraries/newt/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/byobu/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/byobu/default.nix Outdated Show resolved Hide resolved
@berbiche berbiche requested a review from jonringer as a code owner Jul 7, 2020
@berbiche
Copy link
Member Author

@berbiche berbiche commented Jul 7, 2020

Thanks for the comments @FRidh.
I merged the patches from #22774.

The pythonEnv does not seem to have the newt snack package in it. I've looked at many python packages that exported a python library and I still don't understand why the snack package is not picked up.

pkgs/tools/misc/byobu/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/byobu/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/byobu/default.nix Outdated Show resolved Hide resolved
@berbiche
Copy link
Member Author

@berbiche berbiche commented Sep 30, 2020

I came back to work on this.

I had to hack around with makeWrapper since arg0 is NOT the one set with exec -a within the bash scripts.
For instance, calling byobu-screen calls .byobu-screen and arg0 within the script is the path to .byobu-screen.

Byobu reads the arg0 to know which backend (between tmux and screen) it should start which is why all packages had to be wrapped with the non-default name.

@glittershark
Copy link
Member

@glittershark glittershark commented Oct 23, 2020

/status needs_merger

pkgs/development/libraries/newt/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/newt/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/newt/default.nix Outdated Show resolved Hide resolved
pkgs/top-level/python-packages.nix Outdated Show resolved Hide resolved
@berbiche
Copy link
Member Author

@berbiche berbiche commented Oct 23, 2020

@jonringer Thanks, all fixed!

@marvin-mk2
Copy link

@marvin-mk2 marvin-mk2 bot commented Oct 26, 2020

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

@glittershark
Copy link
Member

@glittershark glittershark commented Oct 26, 2020

/status needs_merger

pkgs/development/libraries/newt/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/byobu/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/byobu/default.nix Outdated Show resolved Hide resolved
@berbiche
Copy link
Member Author

@berbiche berbiche commented Oct 27, 2020

byobu has po files but does not compile them? Looking at the .deb it doesn't have the po files either...
The rpm version has it though, so I will copy the logic from it.

Update: Verified that gettext worked with LANGUAGE=fr byobu-config

Copy link
Member

@timokau timokau left a comment

Thank you for your patience and all the work on this!

@timokau timokau merged commit f331acc into NixOS:master Oct 27, 2020
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment