`TCLLIBPATH` does not get passed to git’s gui tools (gitk & git-gui).

ithinuel commented 8 months ago

Describe the bug

TCLLIBPATH does not get passed to git’s gui tools (gitk & git-gui).

Steps To Reproduce

Steps to reproduce the behavior:

Add the following package to your nixpkgs

{ pkgs, lib, tcl, tk }:

tcl.mkTclDerivation rec {
  pname = "awthemes";
  version = "10.4.0";

  src = pkgs.fetchzip {
    url = "mirror://sourceforge/tcl-awthemes/awthemes-${version}.zip";
    hash = "sha256-eObNyKgW7KvcRoYy/xmbzkA3ymw3fkywDzoC6gjZM0s=";
  };

  buildInputs = [ tcl tk ];
  enableParallelBuilding = true;

  postInstall = ''
    mkdir -p $out/lib
    cp -r pkgIndex.tcl $(cat pkgIndex.tcl | grep -o '\w\+.tcl' | uniq -0) i/ $out/lib
  '';

  meta = {
    homepage = "https://sourceforge.net/projects/tcl-awthemes/";
    description = "awthemes";
    licenses = with lib.licences; [ zlib libpng ];
    maintainers = with lib.maintainers; [ agbrooks ]; # provisional
  };
}

include it in your environment along with gitFull.
You may need to create ~/.Xresources containing *TkTheme: awdark
run git gui

Expected behavior

Git GUI should show with a dark interface using the awdark theme from the tcl-awthemes package.

Additional context

This is my first attempt a creating a nix package, so I might be doing something terribly wrong. If so, please educate me.

Notify maintainers

@agbrooks @primeos @wmertens

Metadata

 - system: `"x86_64-linux"`
 - host os: `Linux 6.1.80, NixOS, 23.11 (Tapir), 23.11.5060.617579a78725`
 - multi-user?: `yes`
 - sandbox: `yes`
 - version: `nix-env (Nix) 2.18.1`
 - channels(root): `"home-manager-23.11.tar.gz, nixos-23.11"`
 - nixpkgs: `/nix/var/nix/profiles/per-user/root/channels/nixos`

lolbinarycat commented 8 months ago

looked through the source for git, tk, and tcl, didn't see anything super obvious.

what happens when you run git gui? do you get any errors?
how do you know the error is with TCLLIBPATH not getting passed to git?
what you get from running echo $TCLLIBPATH?
what do you get from running echo "puts [ttk::style theme names]; exit" | wish?

ithinuel commented 8 months ago

With the package above added to my packages,

git gui does not pick up the theme and stays in its default blinding white background & grey frames.
if I export this variable in my environment git gui picks the theme as expected.
Nothing. The variable is not in my environment
From a nix-shell -p tk, I get clam alt default classic. If I set TCLLIBPATH to awthemes’s outPath, that command shows: clam alt default awdark classic

I am very new to nix so I might get things terribly wrong. My understanding is that mkTclDerivation sets up the hook handling the TCLLIBPATH update.

On packages featuring binaries, the hook wrap the program as required.

As far as I understand, the git package does not rely on this hook to wrap the programs.

Out of curiosity I vim’ed wish and noticed the argument passed to makeCWrapper only have the tk library. This makes me wonder whether the reason gitk does not see the theme is because I’m adding gitFull in my /etc/nix/configuration.nix while the theme is only added later on (via home manager).

Then I tried:

let
  pkgs = import <nixpkgs> { config = {}; overlays = []; };
in
pkgs.mkShellNoCC {
    packages = with pkgs; [
        (pkgs.callPackage ./awtheme.nix {})
    ];
}

And in this shell, TCLLIBPATH was set appropriately, so I’m starting to wonder if that’s not an issue with home-manager possibly not keeping that extra environment variable?

lolbinarycat commented 8 months ago

ok, so environment hooks run at build time to make a package able to find its dependencies. in the case of Tcl, since it finds its dependencies at runtime, this build time value is saved into the tcl programs via wrapping.

mkShell actually runs setup hooks (this is useful if you want to compile a program within the shell), so that's why that works.

environment hooks are run on all of a packages dependencies before it is built, so i think what you need to do is inject your theme as a dependency of gitFull using override (appending it to the list of buildInputs).

unfortunately, i don't have experience with home-manager, so i can't tell you exactly how to do that.

ithinuel commented 8 months ago

Thank you for the explanation. I added the following snippet which caused the git package to be rebuilt:

nixpkgs.overlays = [
  (final: prev: {
    gitFull = prev.gitFull.overrideAttrs (old: {
      buildInputs = old.buildInputs ++ [ awthemes ];
    });
  })
];

But it didn’t solve the problem. I think it might be because:

Git does not pass the updated TCLLIBPATH to the underlying wish executable
The underlying wish executable isn’t aware of awtheme
awtheme cannot be made a dependency of tk (which provides wish) as it depends itself on tk.

Still, I tried removing the tk dependency from awthemes and add it to tk’s build input. I wasn’t expecting that so much would be rebuilt (opencascade, kicad, freecad, python3.11 just to name a few). This reminded me of my Gentoo days :D

After a long while (and disabling freecad, kicad and overriding pytest-benchmark python’s attribute to disable test_calibration) I can confirm that while incorrect (wrt dependency definitions), this did fix the issue.

Does that meant that wish should be made into another package so that its buildInputs (and more specifically its TCLLIBPATH) can be overriden ?
If I open a PR for awthemes, what would be most sensical way to get all packages providing a TCL binary to get their TCLLIBPATH (and consequently wrappers) updated accordingly?

lolbinarycat commented 8 months ago

i think handling theming should be done by home-manager, not nixpkgs itself.

nixpkgs has no way of knowing what other themes are available, and which it should use. NixOS maybe could, but theming at the system level seems wrong.

ithinuel commented 8 months ago

i think handling theming should be done by home-manager, not nixpkgs itself.

I’m not sure that this being a library really matters here, but

just to name a few. A search for theme can give you a more exhaustive view. So themes being available as nixpkgs is a thing.

The selection of the theme itself is done by the user setting the *TkTheme: value in ~/.Xresources. But for this to work, the relevant library needs to be in TCLLIBPATH/discoverable by the executables.

I’m fine with the override solution, however it’s triggering the rebuild of loads of packages that should be rebuilt. So I believe wish should be moved to a different package from tk so that the former can be rebuilt/overriden without requirement all dependencies of tk to be rebuilt too.

lolbinarycat commented 8 months ago

i meant that the process of selecting and customizing themes should be handled outside of nixpkgs, not that themes can't be packages within nixpkgs.

you could try overriding gitFull to apply wrapProgram --prefix TCLLIBPATH /* ... */ to git-gui in postFixup, maybe that would work? it should also result in a lot less rebuilds i think.

ithinuel commented 8 months ago

Thanks, I will try that. 👍

If that works, I guess that'd be something worth a wrapTclProgram (or some updateTkWish) since it seems like something quite recurrent.

EDIT: I guess they'd still need to be overridden to get the updated TCLLIBPATH variable.

ithinuel commented 8 months ago

I gave it a try but this might be too much for my limited understanding of nix at this time.

NixOS / nixpkgs