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

gitkraken: 6.4.1 -> 6.5.1 #78420

Merged
merged 1 commit into from Feb 10, 2020
Merged

gitkraken: 6.4.1 -> 6.5.1 #78420

merged 1 commit into from Feb 10, 2020

Conversation

evanjs
Copy link
Member

@evanjs evanjs commented Jan 24, 2020

Motivation for this change

https://support.gitkraken.com/release-notes/current/#version-651

Things done
  • use ar instead of dpkg for unpackCmd to avoid attempts by the package
    to change permissions and etc.
  • add at-spi2-core dependency
  • remove dpkg dependency

Open to comments on the change to the unpackCmd. As far as I can tell, the application seems to work the same as before.

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

@megheaiulian
Copy link
Contributor

@evanjs If you want to drop dpkg dependency it might also make sense to fetch the *.tar.gz file instead of the *.deb one.

Copy link
Contributor

@megheaiulian megheaiulian left a comment

Choose a reason for hiding this comment

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

Tested this and the executable works correctly. If you want to go with the suggestion to fetch tar.gz it would be great. Otherwise it can be done in a future update.

@evanjs
Copy link
Member Author

evanjs commented Jan 31, 2020

diff --git a/pkgs/applications/version-management/gitkraken/default.nix b/pkgs/applications/version-management/gitkraken/default.nix
index 83368932e25..73d376396a6 100644
--- a/pkgs/applications/version-management/gitkraken/default.nix
+++ b/pkgs/applications/version-management/gitkraken/default.nix
@@ -1,7 +1,7 @@
 { stdenv, libXcomposite, libgnome-keyring, makeWrapper, udev, curl, alsaLib
 , libXfixes, atk, gtk3, libXrender, pango, gnome3, cairo, freetype, fontconfig
 , libX11, libXi, libxcb, libXext, libXcursor, glib, libXScrnSaver, libxkbfile, libXtst
-, nss, nspr, cups, fetchurl, expat, gdk-pixbuf, libXdamage, libXrandr, dbus
+, nss, nspr, cups, fetchzip, expat, gdk-pixbuf, libXdamage, libXrandr, dbus
 , makeDesktopItem, openssl, wrapGAppsHook, at-spi2-atk, at-spi2-core, libuuid
 , e2fsprogs, krb5
 }:
@@ -15,11 +15,14 @@ stdenv.mkDerivation rec {
   pname = "gitkraken";
   version = "6.5.1";
 
-  src = fetchurl {
-    url = "https://release.axocdn.com/linux/GitKraken-v${version}.deb";
-    sha256 = "1zcbbszz7vnwgd4hxwhbp28gminc3pqk0v7id7jc8ylnbgpiy2bk";
+  src = fetchzip {
+    url = "https://release.axocdn.com/linux/GitKraken-v${version}.tar.gz";
+    sha256 = "0dwjwismv4rfw58801g2ay51h9qrffcxgbl910frd4i530w0y44p";
   };
 
+  dontBuild = true;
+  dontConfigure = true;
+
   libPath = makeLibraryPath [
     stdenv.cc.cc.lib
     curlWithGnuTls
@@ -73,29 +76,24 @@ stdenv.mkDerivation rec {
   nativeBuildInputs = [ makeWrapper wrapGAppsHook ];
   buildInputs = [ gtk3 gnome3.adwaita-icon-theme ];
 
-  unpackCmd = ''
-    mkdir out
-    pushd out
-
-    # manually unpack deb to avoid attempts at changing utime, permissions, etc
-    ar x $curSrc
-    tar -xf data.tar.xz
-    popd
-  '';
-
   installPhase = ''
     runHook preInstall
-    mkdir $out
-    pushd usr
-    pushd share
-    substituteInPlace applications/gitkraken.desktop \
-      --replace /usr/share/gitkraken $out/bin
-    popd
-    rm -rf bin/gitkraken share/lintian
-    cp -av share bin $out/
-    popd
 
+    mkdir -p $out/share/gitkraken/
+    cp -R $src/* $out/share/gitkraken/
+
+    mkdir -p $out/bin
     ln -s $out/share/gitkraken/gitkraken $out/bin/gitkraken
+
+    mkdir -p $out/share/applications
+    cp ${desktopItem}/share/applications/* $out/share/applications/
+
+    substituteInPlace $out/share/applications/gitkraken.desktop \
+      --replace $out/usr/share/gitkraken $out/bin
+
+    mkdir -p $out/share/pixmaps
+    cp gitkraken.png $out/share/pixmaps/gitkraken.png
+
     runHook postInstall
   '';
 

Mrm this seems to work, just not sure on the cleanliness

@megheaiulian
Copy link
Contributor

That looks good to me and is in fact cleaner that what's being done for the deb version. I think you should commit that.

- use fetchzip to retrieve the tarball for GitKraken, as the deb now tries to change permissions and etc which nix will not like
- add at-spi2-core dependency
- remove dpkg dependency
- refactor expression to properly handle the GitKraken tarball (compared to the deb archive)
@evanjs
Copy link
Member Author

evanjs commented Jan 31, 2020

That looks good to me and is in fact cleaner that what's being done for the deb version. I think you should commit that.

Sounds good. Thanks for the review!

@evanjs evanjs changed the title gitkraken: 6.4.1 -> 6.5.0 gitkraken: 6.4.1 -> 6.5.1 Feb 3, 2020
@evanjs evanjs requested a review from jonringer February 7, 2020 05:31
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/need-help-upgrade-a-package-gitkraken/5829/2

@worldofpeace worldofpeace merged commit 9c22a82 into NixOS:master Feb 10, 2020
@evanjs evanjs removed the request for review from jonringer February 10, 2020 02:52
@evanjs evanjs deleted the gitkraken-6.5.0 branch February 10, 2020 02:53
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.

None yet

4 participants