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

tabby-terminal: init at 1.0.207 #303924

Closed
wants to merge 1 commit into from

Conversation

ChocolateLoverRaj
Copy link
Contributor

@ChocolateLoverRaj ChocolateLoverRaj commented Apr 13, 2024

Description of changes

Added a package for Tabby. I'm new to creating Nix packages so there is probably a lot of room for improvement of the default.nix file I created. I tested it by adding (callPackage /home/rajas/Documents/nixpkgs/pkgs/applications/terminal-emulators/tabby { }) to my system packages in NixOS config file. Closes #233509.

Things that are missing:

  • ARM package (Tabby does build them)
  • MacOS package (Tabby does build them)
  • Maybe there is a way of adding to LD_LIBRARY_PATH paths instead of overriding it, but I'm not sure how
  • Doesn't follow Electron apps to be built from source #296939

Also the name conflicts with tabby from unstable

Things done

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

Add a 👍 reaction to pull requests you find important.

@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Apr 13, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Apr 14, 2024
@pluiedev
Copy link
Contributor

pluiedev commented Apr 14, 2024

Please follow the standard commit & PR naming scheme of: <package>: init at <version>.

So for example, tabby: init at <x.y.z>

Additionally, new packages should always follow the by-name convention, and so the Nix file containing the derivation for Tabby should be put in pkgs/by-name/ta/tabby/package.nix.


cat > $out/bin/tabby << END
#!/bin/sh
'$out/opt/Tabby/tabby' --no-sandbox $@
Copy link
Contributor

Choose a reason for hiding this comment

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

There should preferably be an explanation on what --no-sandbox does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idk what it does, but that's what the official Tabby deb has

pkgs/applications/terminal-emulators/tabby/default.nix Outdated Show resolved Hide resolved
pkgs/applications/terminal-emulators/tabby/default.nix Outdated Show resolved Hide resolved
@pluiedev
Copy link
Contributor

Also, the name conflict between Tabby the... AI thing (?) and Tabby the terminal emulator definitely has to be resolved somehow. The terminal emulator is older but the AI program was added to nixpkgs earlier.

I wonder if there's precedence to this?

@ChocolateLoverRaj
Copy link
Contributor Author

Idk what this package should be called since tabby is taken. It would be weird if people updated their NixOS and their AI thing was replaced by a terminal.

@pluiedev
Copy link
Contributor

I guess tabby-terminal works, but then you have to rename the commit and the PR to reflect that. I'm fine with going with that name if nobody objects

@ChocolateLoverRaj ChocolateLoverRaj changed the title tabby: add package tabby-terminal: init at 1.0.207 Apr 16, 2024
@ChocolateLoverRaj
Copy link
Contributor Author

I think this PR is ready

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Apr 17, 2024
@ChocolateLoverRaj ChocolateLoverRaj force-pushed the tabby branch 2 times, most recently from a36f276 to 1b9eeda Compare April 18, 2024 00:33
@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-linux: 1-10 10.rebuild-linux: 1 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Apr 18, 2024
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Apr 18, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label May 3, 2024
@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label May 5, 2024
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label May 5, 2024
@yswtrue
Copy link

yswtrue commented May 9, 2024

waiting for pr

Copy link
Contributor

@pluiedev pluiedev left a comment

Choose a reason for hiding this comment

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

Looks good, though one day we should also add Darwin support I think

pkgs/by-name/ta/tabby-terminal/package.nix Outdated Show resolved Hide resolved
ln -s $out/opt/Tabby/tabby $out/bin/tabby

substituteInPlace $out/share/applications/tabby.desktop \
--replace "/opt" $out/opt
Copy link
Member

Choose a reason for hiding this comment

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

Be careful with absolute path in desktop entry. See #308324

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't seem to be fixed:

$ cat result/share/applications/tabby.desktop | grep Exec
Exec=/nix/store/5j7s1cl4y7f6v57iyd998rm1dg828hym-tabby-terminal-1.0.207/opt/Tabby/tabby --no-sandbox %U

pkgs/by-name/ta/tabby-terminal/package.nix Show resolved Hide resolved
postInstall = ''
mkdir -p $out/{opt,bin}
cp -r opt $out
wrapProgram "$out/opt/Tabby/tabby" --add-flags "\''${NIXOS_OZONE_WL:+\''${WAYLAND_DISPLAY:+--ozone-platform=wayland --enable-features=WaylandWindowDecorations}} --no-sandbox" --set LD_LIBRARY_PATH=${lib.makeLibraryPath buildInputs}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
wrapProgram "$out/opt/Tabby/tabby" --add-flags "\''${NIXOS_OZONE_WL:+\''${WAYLAND_DISPLAY:+--ozone-platform=wayland --enable-features=WaylandWindowDecorations}} --no-sandbox" --set LD_LIBRARY_PATH=${lib.makeLibraryPath buildInputs}
wrapProgram "$out/opt/Tabby/tabby" --add-flags "\''${NIXOS_OZONE_WL:+\''${WAYLAND_DISPLAY:+--ozone-platform=wayland --enable-features=WaylandWindowDecorations}} --no-sandbox"

Don't prioritize LD_LIBRARY_PATH. Try using autoPatchelfHook instead. See https://nixos.org/manual/nixpkgs/unstable/#setup-hook-autopatchelfhook (You have to look carefully how buildInputs works in this case, and use some runtimeDependencies if that doesn't work)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this:

{
  stdenv,
  fetchurl,
  pkgs,
  lib,
  alsa-lib,
}:

stdenv.mkDerivation rec {
  name = "tabby-terminal";
  version = "1.0.207";
  src = fetchurl {
    url = "https://github.com/Eugeny/tabby/releases/download/v${version}/tabby-${version}-linux-x64.deb";
    hash = "sha256-PTTkL3+mYb7KM8fDUmgCuAF2lU88fYOstGWp/O5WZas=";
  };

  nativeBuildInputs = with pkgs; [
    dpkg
    autoPatchelfHook
  ];

  buildInputs = with pkgs; [
    glib
    nss
    nspr
    atk
    cups
    dbus
    libdrm
    gtk3
    pango
    cairo
    xorg.libX11
    xorg.libXcomposite
    xorg.libXdamage
    xorg.libXext
    xorg.libXfixes
    xorg.libXrandr
    libxkbcommon
    mesa
    expat
    xorg.libxcb
    alsa-lib
    libGL
    libsecret
    musl
  ];

  runtimeDependencies = buildInputs;

  postInstall = ''
    mkdir -p $out/{opt,bin}
    cp -r opt $out
    # wrapProgram "$out/opt/Tabby/tabby" --add-flags "\''${NIXOS_OZONE_WL:+\''${WAYLAND_DISPLAY:+--ozone-platform=wayland --enable-features=WaylandWindowDecorations}} --no-sandbox"
    cp -r usr/* $out

    mv $out/opt/Tabby/tabby $out/bin/tabby

    substituteInPlace $out/share/applications/tabby.desktop \
      --replace "/opt/Tabby/tabby" tabby
  '';

  meta = with lib; {
    homepage = "https://tabby.sh";
    description = "A terminal for a more modern age";
    license = licenses.mit;
    maintainers = with maintainers; [ ChocolateLoverRaj ];
    mainProgram = "tabby";
    platforms = platforms.linux;
    downloadPage = "https://github.com/Eugeny/tabby/releases/latest";
  };
}

When I run it I get this error:

[rajas@nixos:~/Documents/nixpkgs]$ ./result/bin/tabby 
[0516/104608.334415:ERROR:icu_util.cc(240)] Invalid file descriptor to ICU data received.
Trace/breakpoint trap (core dumped)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe ico is missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SuperSandro2000 should I try adding xorg.ico?

pkgs/by-name/ta/tabby-terminal/package.nix Show resolved Hide resolved
@ChocolateLoverRaj ChocolateLoverRaj force-pushed the tabby branch 2 times, most recently from 74e3bdb to 82fea25 Compare May 16, 2024 18:04
pkgs/by-name/ta/tabby-terminal/package.nix Outdated Show resolved Hide resolved
postInstall = ''
mkdir -p $out/{opt,bin}
cp -r opt $out
wrapProgram "$out/opt/Tabby/tabby" --add-flags "\''${NIXOS_OZONE_WL:+\''${WAYLAND_DISPLAY:+--ozone-platform=wayland --enable-features=WaylandWindowDecorations}} --no-sandbox" --set LD_LIBRARY_PATH=${lib.makeLibraryPath buildInputs}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe ico is missing?

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label May 22, 2024
@@ -0,0 +1,64 @@
{ stdenv, fetchurl, pkgs, lib, alsa-lib }:
Copy link
Contributor

@eclairevoyant eclairevoyant Jun 28, 2024

Choose a reason for hiding this comment

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

Suggested change
{ stdenv, fetchurl, pkgs, lib, alsa-lib }:
{ stdenv, fetchurl, lib, alsa-lib }:

Definitely don't pass pkgs in; pass in the dependencies individually.


stdenv.mkDerivation rec {
pname = "tabby-terminal";
version = "1.0.207";
Copy link
Contributor

Choose a reason for hiding this comment

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

nonblocking - but fyi, 1.0.208 is out

@eclairevoyant
Copy link
Contributor

Besides the feedback above, this doesn't run as-is:

result/bin/tabby: line 3: /nix/store/5j7s1cl4y7f6v57iyd998rm1dg828hym-tabby-terminal-1.0.207/opt/Tabby/.tabby-wrapped: cannot execute: required file not found

@ChocolateLoverRaj ChocolateLoverRaj marked this pull request as draft June 28, 2024 01:50
@ChocolateLoverRaj
Copy link
Contributor Author

I'm stopping my work on this PR because I'm no longer interested in using Tabby. I've been using Konsole and I'm planning on switching to COSMIC Term when it's ready. I don't want to put more time into this, but if anyone else wants to, feel free to continue working on this PR. I'm not sure if I should close this PR since I won't be adding any more commits to it.

@eclairevoyant
Copy link
Contributor

Closing would make sense if you have no intention to work on it further.

@ChocolateLoverRaj
Copy link
Contributor Author

Hopefully this PR will help whoever wants to make another PR for Tabby package

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 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 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Package request: tabby
9 participants