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

Jump to bottom

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 •
edited

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.

@ofborg ofborg bot requested a review from qknight June 15, 2020 03:48
@teto
Copy link
Member

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

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 commented Jun 18, 2020 •
edited

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.

@ofborg ofborg bot requested a review from viric June 18, 2020 03:18
@berbiche
Copy link
Member Author

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

FRidh
FRidh requested changes Jul 2, 2020
View reviewed changes
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 July 7, 2020 01:05
@berbiche
Copy link
Member Author

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.

jonringer
jonringer reviewed Jul 7, 2020
View reviewed changes
pkgs/tools/misc/byobu/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/byobu/default.nix Outdated Show resolved Hide resolved
@berbiche berbiche force-pushed the byobu-screen branch 3 times, most recently from 0a26c3c to da55bee Compare July 7, 2020 01:25
@berbiche
Copy link
Member Author

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

/status needs_merger

@marvin-mk2 marvin-mk2 bot requested a review from timokau October 23, 2020 16:54
jonringer
jonringer reviewed Oct 23, 2020
View reviewed changes
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

@jonringer Thanks, all fixed!

@marvin-mk2
Copy link

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

/status needs_merger

timokau
timokau requested changes Oct 26, 2020
View reviewed changes
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 commented Oct 27, 2020 •
edited

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

timokau
timokau approved these changes Oct 27, 2020
View reviewed changes
Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

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

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

@timokau timokau merged commit f331acc into NixOS:master Oct 27, 2020
@knedlsepp knedlsepp mentioned this pull request Oct 1, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glittershark glittershark glittershark approved these changes

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@jonringer jonringer jonringer left review comments

@timokau timokau timokau approved these changes

@qknight qknight Awaiting requested review from qknight

@viric viric Awaiting requested review from viric

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
6.topic: python 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 501-1000 10.rebuild-linux: 501+ awaiting_reviewer marvin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@berbiche @teto @nixos-discourse @glittershark @FRidh @timokau @SuperSandro2000 @jonringer