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

Unified packaging process #252

Merged
merged 3 commits into from
Apr 23, 2023

Conversation

Ballasi
Copy link
Contributor

@Ballasi Ballasi commented Apr 22, 2023

Hey!

It seems to me that xdg-ninja is packaged quite poorly in some cases, like putting a symlink to the programs/ directory directly in the bin/ folder of the system or modifying the script in place to patch the location of the programs/ directory.

This mostly is a consequence of the way the script currently fetches the programs/ directory:

$(jq 'inputs as $input | $input.files[] as $file | $input.name, $file.path, $file.movable, $file.help' "$(realpath "$0" | xargs dirname)"/programs/* | sed -e 's/^"//' -e 's/"$//')

i.e., directly in the current working directory of the xdg-ninja script.

This works fine when working within the git repository but does poorly when trying to package the program as there needs to implement hacks to get this working.

On a useless note, I wouldn't have hacked it this way on my side, I would've just packaged xdg-ninja in the same directory as programs in, for instance, /usr/share/xdg-ninja/ and would create a wrapper script that would call /usr/share/xdg-ninja/xdg-ninja.sh.

I am unsure whether or not this is a goal of your program to be packaged, but if so, I have implemented an simple way to unify the packaging process.


My main goal, when modifying the script, was to get it backwards compatible with how it was previously used, i.e. when working in the git repository, it does fetch the current programs/ dir rather than somewhere else.

So I have just set the script to check in the current working directory and replace ending instances of /bin to /share/xdg-ninja (where the programs/ dir is located when packaged).

If all cases it fails, the XN_PROGRAMS_DIR can be used to override that value. I have preferred to use this instead of a --programs-dir= argument as this isn't something meant for common usage either.

I've also allowed people to override where the script is supposed to be located with the PREFIX variable in the Makefile, I believe package managers should package apps to /usr instead of /usr/local so at least it can be changed. This make variable and DESTDIR and both common naming standards.

In order to foresee the work on the unified packaging process, the
programs/ directory (which should be located in /usr/share/xdg-ninja or
/usr/local/share/xdg-ninja) is automatically found depending on where
the xdg-ninja executable is located.

This also allows for backwards compatibility as when the xdg-ninja
script is located in a working directory (not in a bin/), xdg-ninja
automatically picks the programs/ folder in the current working
directory, just like previously.

If the directory fails to find the programs/ dir, the user can override
the variable XN_PROGRAMS_DIR through environment variables.

This change is to prevent people from packaging xdg-ninja with the
programs/ directory within /usr(/local)/bin or by modifying the script
in place.
@b3nj5m1n
Copy link
Owner

Hi! Thank you very much for this, it seems like a much-needed change.

The only concern I have is with the Install section in the README. I'm not sure that it makes sense to present this to users as the best/only option.

There is another big issue with packaging, I think perhaps I should finally think of something to fix that, update the packages, (I'm aware of arch, homebrew and nix) then write a proper installation section which recommends installing through a package manager first. The README needs a bit of an overhaul anyway tbh.

If you're okay with it, I'd say remove the Install section for now.

And again, thanks for putting time into this!


Btw, this breaks the nix flake, I tried to update it but it seems I don't have write permissions for this PR, I'll just leave the patch here, if you want to apply it that'd be great, otherwise I'll do it after merging.

From 640977d61030c576da3b47edfa09fd651b75a6ee Mon Sep 17 00:00:00 2001
From: b3nj5m1n <b3nj4m1n@gmx.net>
Date: Sun, 23 Apr 2023 08:00:13 +0200
Subject: [PATCH] Update flake

---
 flake.nix | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/flake.nix b/flake.nix
index 91adf01..8f3a5bf 100644
--- a/flake.nix
+++ b/flake.nix
@@ -26,7 +26,25 @@
       in rec {
         packages = flake-utils.lib.flattenTree {
           # The shell script and configurations, uses derivation from offical nixpkgs
-          xdg-ninja = pkgs.xdg-ninja;
+          xdg-ninja = pkgs.stdenv.mkDerivation rec {
+            pname = "xdg-ninja";
+            version = "0.1.0";
+
+            src = ./.;
+
+            nativeBuildInputs = with pkgs; [ makeWrapper ];
+
+            installPhase = ''
+              runHook preInstall
+
+              DESTDIR="$out" PREFIX="/usr" make install
+
+              wrapProgram "$out/usr/bin/xdg-ninja" \
+                --prefix PATH : "${pkgs.lib.makeBinPath [ pkgs.glow pkgs.jq ]}"
+
+              runHook postInstall
+            '';
+          };
           # Pre-built binary of xdgnj tool downloaded from github
           xdgnj-bin = pkgs.stdenvNoCC.mkDerivation {
             name = "xdgnj-bin";
@@ -45,7 +63,7 @@
         };
         defaultPackage = packages.xdg-ninja;
         apps = {
-          xdg-ninja = flake-utils.lib.mkApp { drv = packages.xdg-ninja; };
+          xdg-ninja = flake-utils.lib.mkApp { drv = packages.xdg-ninja; exePath = "/usr/bin/xdg-ninja"; };
           xdgnj-bin = flake-utils.lib.mkApp { drv = packages.xdgnj-bin; exePath = "/bin/xdgnj"; };
         };
         defaultApp = apps.xdg-ninja;
-- 
2.39.2

@b3nj5m1n b3nj5m1n added the enhancement New feature or request label Apr 23, 2023
As to unify packaging process, the following changes adds install and
uninstall commands of xdg-ninja to the system.
@Ballasi
Copy link
Contributor Author

Ballasi commented Apr 23, 2023

I haven't taken the time to take a look at nix as I am unfamiliar with this distro, so that's a good thing you were able to test that on your side. I have tested this on Ubuntu and Arch.

I have added your patch on my branch and removed the Install section of the README as discussed.

@b3nj5m1n
Copy link
Owner

Great, thank you very much again!

@b3nj5m1n b3nj5m1n merged commit 619f172 into b3nj5m1n:main Apr 23, 2023
@Ballasi
Copy link
Contributor Author

Ballasi commented Apr 23, 2023

No problem, I can see that the AUR package for xdg-ninja-git has already been updated too!

From there, I can only recommend you to do a new GitHub release to push an update to the xdg-ninja AUR package which relies on tags/releases rather than the git repository version itself.

@b3nj5m1n
Copy link
Owner

I can only recommend you to do a new GitHub release

Yeah, I'm still thinking about how to handle releases, I could either use them exclusively for xdg-ninja, (the script & new json files) or have a prefix for xdg-ninja/xdgnj to distinguish between the two.

I can see that the AUR package for xdg-ninja-git has already been updated too!

Wow that was quick!

@Ballasi Ballasi deleted the unified_packaging_process branch April 24, 2023 03:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants