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

pywalfox-native: fixup manifest installation #282408

Merged

Conversation

tsandrini
Copy link
Contributor

Description of changes

Resolves #281377. At first I tried setting up the manifest file directly from the derivation by outputting it at $out/usr/lib/mozilla/native-messaging-hosts/pywalfox.json, which didn't work and ultimately even if it did it wouldn't be able to cover all the cases

  • nix package manager only on a non NixOS system
  • NixOS only on root (expected to be at /usr/lib/mozilla/native-messaging-hosts/pywalfox.json)
  • NixOS only in the userspace (expected at .mozilla/native-messaging-hosts/pywalfox.json)
  • NixOS with HM
  • HM only on a non NixOS distro

The conclusion is that NixOS/HM modules can also be prepared to automatically set up the manifest file (which is how I was personally using it before), however, an imperative default, that is pywalfox install, should still remain to cover the other use cases if needed. Thus the following PR patches the install.py script to work with the structure of the nix store.

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.

@tsandrini tsandrini self-assigned this Jan 20, 2024
@cole-h
Copy link
Member

cole-h commented Jan 20, 2024

@ofborg eval

@tsandrini
Copy link
Contributor Author

Output of nix run .#nixpkgs-review -- pr 282408:

2 packages built:
pywalfox-native pywalfox-native.dist

@KiaraGrouwstra
Copy link
Contributor

with this PR i can get pywalfox commands install / update / start to run without errors.

that said, the browser extension for me still says disconnected, with the fetch button yielding a not connected error.
this PR seems like an improvement, but should it work better than this?

as per the extension's troubleshooting section, my ~/.cache/wal/colors exists, the debugging log is empty, and the pywalfox.json file is present.

my browser extension uses version 2.0.11.

@tsandrini
Copy link
Contributor Author

tsandrini commented May 31, 2024

with this PR i can get pywalfox commands install / update / start to run without errors.

that said, the browser extension for me still says disconnected, with the fetch button yielding a not connected error. this PR seems like an improvement, but should it work better than this?

as per the extension's troubleshooting section, my ~/.cache/wal/colors exists, the debugging log is empty, and the pywalfox.json file is present.

my browser extension uses version 2.0.11.

@KiaraGrouwstra Hi, thanks for the info. I've tested this on all setups that are available to me (NixOS only VM, NixOS with HM install, HM only). The pywalfox extension works by setting up a native-messaging-host specification at $MOZILLA_HOME/native-messaging-hosts/pywalfox.json which is then at startup executed directly by firefox. In the case of pywalfox-native this runs pywalfox start.

Could you please provide me with more information about your setup?

  1. Are you running on darwin? x86, aarch64? Installation via home-manager or nixos (or nix-darwin)? nix-shell -p nix-info --run "nix-info -m" ?
  2. You said that pywalfox.json exists, I assume you meant $MOZILLA_HOME/native-messaging-hosts/pywalfox.json. Can you paste the contents of this file please?
  3. How have you installed firefox and what is $MOZILLA_HOME in your case? (fyi it's not an actual envvar, just asking where is you mozilla-home)

Also the pywalfox.json file should have a patched path variable, for example mine looks like this:

{
  "name": "pywalfox",
  "description": "Automatically theme your browser using the colors generated by Pywal",
  "path": "/nix/store/9x3lcmq4zjqaxmsnb3vaj7wnjmps05bw-pywalfox-native-2.7.4/lib/python3.11/site-packages/pywalfox/bin/main.sh",
  "type": "stdio",
  "allowed_extensions": [ "[email protected]" ]
}

are you able to manually execute the path? Does your pywalfox firefox extension work if you do it manually like this?

Thanks in advance <3.

# Overwrite the original wrapper script with a new one that has the
# appropriate executable paths.
cat > $out/lib/${python3.libPrefix}/site-packages/pywalfox/bin/main.sh <<'EOF'
#!${stdenv.shell}
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
#!${stdenv.shell}
#!${runtimeShell}

# appropriate executable paths.
cat > $out/lib/${python3.libPrefix}/site-packages/pywalfox/bin/main.sh <<'EOF'
#!${stdenv.shell}
${placeholder "out"}/bin/pywalfox start
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
${placeholder "out"}/bin/pywalfox start
$out/bin/pywalfox start

It may be better if you override this script with makeShellWrapper directly.

Copy link
Contributor Author

@tsandrini tsandrini Jun 3, 2024

Choose a reason for hiding this comment

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

@Aleksanaa Since the original main.sh directly calls python pywalfox (https://github.com/Frewacom/pywalfox-native/blob/master/pywalfox/bin/main.sh#L10) I'd need to write something along these lines

  postInstall = ''
    export PYWALFOX=$out/lib/${python3.libPrefix}/site-packages/pywalfox/

    chmod +x $PYWALFOX/bin/main.sh

    makeShellWrapper $PYWALFOX/bin/main.sh $out/bin/patched-main.sh \
      --prefix PATH : ${lib.makeBinPath [ (python3.withPackages [ self??? ]) ]}

    substituteInPlace $PYWALFOX/config.py \
      --replace "BIN_PATH_UNIX = os.path.join(APP_PATH, 'bin/main.sh')" \
      "BIN_PATH_UNIX = \"$out/bin/patched-main.sh\""

	# ....
  '';

which felt a bit overcomplicated and I am still not entirely sure how would I "correctly" handle the self reference. Hence why I opted to just replace the original script with a new one.

Also the point of the original main.sh is to just run pywalfox start but ensure it works on different setups (under root, macos, etc..), however, in nix we have all the paths under our control, which is why I feel like the original main.sh in not really needed and we can replace it with our one instead of wrapping it, which just brings unnecessary patching on our side.

pywalfox-native: fix install
@tsandrini tsandrini force-pushed the pywalfox-native-add-manifest-install branch from bbb3deb to f4724cf Compare June 4, 2024 08:23
@tsandrini
Copy link
Contributor Author

@Aleksanaa I looked into the improved-installation branch of pywalfox-native (https://github.com/Frewacom/pywalfox-native/tree/improved-installation), I didn't really like the idea of deviating from the pypi master, however this version fixes all the issues with the installation and removes the need for the patches that I did on the nix side. I skimmed through the commits of said branch and it looks like the changes are only related to better environment handling and installation. So I propose to use this version directly instead.

@tsandrini tsandrini requested a review from Aleksanaa June 20, 2024 16:51
@Aleksanaa Aleksanaa merged commit a9bde5c into NixOS:master Jun 21, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pywalfox-native: pywalfox install fails
4 participants