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

survex: init at 1.2.44 #126145

Merged
merged 3 commits into from
Jun 17, 2021
Merged

survex: init at 1.2.44 #126145

merged 3 commits into from
Jun 17, 2021

Conversation

MatthewCroughan
Copy link
Contributor

@MatthewCroughan MatthewCroughan commented Jun 8, 2021

Motivation for this change

Adds survex, a package used by cavers in order to survey and map out caves in the real world. Usually this is only packaged on Debian due to its age and complexity.

Things done
  • 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/)
  • 21.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.

variety of platforms, including Linux/Unix, macOS, and Microsoft Windows.
'';
homepage = "https://www.gnu.org/software/hello/manual/";
changelog = "https://git.savannah.gnu.org/cgit/hello.git/plain/NEWS?h=v${version}";
Copy link
Member

Choose a reason for hiding this comment

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

There is a changelog in the repo but it seems out-dated.

Copy link
Contributor

Choose a reason for hiding this comment

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

NEWS seems to have updated changelog information.

@OPNA2608
Copy link
Contributor

OPNA2608 commented Jun 8, 2021

Screenshot_macOS_2021-06-08_20:20:39

Changes for building on Darwin:

pkgs/data/fonts/unscii/default.nix
diff --git a/pkgs/data/fonts/unscii/default.nix b/pkgs/data/fonts/unscii/default.nix
index 066a4d4d922..2bedf6c4f6f 100644
--- a/pkgs/data/fonts/unscii/default.nix
+++ b/pkgs/data/fonts/unscii/default.nix
@@ -2,6 +2,11 @@
 , fontforge, SDL, SDL_image, mkfontscale
 }:
 
+let
+  perlenv = perl.withPackages (p: with p; [
+    TextCharWidth
+  ]);
+in
 stdenv.mkDerivation rec {
   pname = "unscii";
   version = "1.1";
@@ -12,11 +17,23 @@ stdenv.mkDerivation rec {
   };
 
   nativeBuildInputs =
-    [ (perl.withPackages (p: [ p.TextCharWidth ]))
+    [ perlenv
       bdftopcf fontforge SDL SDL_image
       mkfontscale
     ];
 
+  # Fixes shebang -> wrapper problem on Darwin
+  postPatch = ''
+    for perltool in *.pl; do
+      substituteInPlace Makefile \
+        --replace "./$perltool" "${perlenv}/bin/perl ./$perltool"
+    done
+  '';
+
+  makeFlags = [
+    "CC=${stdenv.cc.targetPrefix}cc"
+  ];
+
   preConfigure = ''
     patchShebangs .
   '';
pkgs/applications/misc/survex/default.nix
diff --git a/pkgs/applications/misc/survex/default.nix b/pkgs/applications/misc/survex/default.nix
index 1db4b735eb8..24942b3e4fd 100644
--- a/pkgs/applications/misc/survex/default.nix
+++ b/pkgs/applications/misc/survex/default.nix
@@ -4,6 +4,7 @@
 , autoreconfHook
 , pkg-config
 , wxGTK30-gtk3
+, wxmac
 , ffmpeg
 , proj
 , perl532
@@ -14,15 +15,26 @@
 , xlibsWrapper
 , docbook2x
 , docbook5
+, Carbon
+, Cocoa
 }:
 
+let
+  perlenv = perl532.withPackages (perlPackages: with perlPackages; [ LocalePO ] );
+in
 stdenv.mkDerivation rec {
   pname = "survex";
   version = "1.2.44";
 
-  nativeBuildInputs = [ docbook5 docbook2x autoreconfHook pkg-config ];
+  nativeBuildInputs = [ docbook5 docbook2x autoreconfHook pkg-config perlenv python ];
 
-  buildInputs = [ xlibsWrapper libGL libGLU wxGTK30-gtk3 ffmpeg proj python (perl532.withPackages (perlPackages: with perlPackages; [ LocalePO ] )) ];
+  buildInputs = [
+    libGL libGLU ffmpeg proj
+  ] ++ lib.optionals stdenv.hostPlatform.isDarwin [
+    wxmac Carbon Cocoa
+  ] ++ lib.optionals stdenv.hostPlatform.isLinux [
+    wxGTK30-gtk3 xlibsWrapper
+  ];
 
   src = fetchgit {
     url = "git://git.survex.com/survex";
@@ -30,20 +42,25 @@ stdenv.mkDerivation rec {
     sha256 = "11gaqmabrf3av665jy3mr0m8hg76fmvnd0g3rghzmyh8d8v6xk34";
   };
 
+  enableParallelBuilding = true;
+
   # Docs rely on sgmltools-lite, a package that would be quite complex to
   # provide as it is quite old. So this preConfigure hook effectively disables
   # the doc generation. An example of packaging sgmltools-lite from Gentoo can
   # be found here:
   # https://gitweb.gentoo.org/repo/gentoo.git/tree/app-text/sgmltools-lite/sgmltools-lite-3.0.3-r15.ebuild?id=0b8b716331049599ea3299981e3a9ea6e258c5e0
 
-  preConfigure = ''
+  postPatch = ''
+    patchShebangs .
     echo "" > doc/Makefile.am
-  '';
-
-  postConfigure = ''
 #    substituteInPlace doc/Makefile --replace "docbook2man" "docbook2man --sgml" # Will be needed once sgmltools-lite is packaged.
+    for perltool in './extract-msgs.pl' './gettexttomsg.pl' '$(srcdir)/gdtconvert' '$(srcdir)/gen_img2aven'; do
+      substituteInPlace src/Makefile.am \
+        --replace "$perltool" "${perlenv}/bin/perl $perltool"
+    done
+    substituteInPlace lib/Makefile.am \
+      --replace '$(srcdir)/make-pixel-font' '${perlenv}/bin/perl $(srcdir)/make-pixel-font'
     substituteInPlace lib/make-pixel-font --replace /usr/share/unifont/unifont.hex ${unscii.extra}/share/fonts/misc/unifont.hex
-    patchShebangs .
   '';
 
   meta = with lib; {
pkgs/top-level/all-packages.nix
diff --git a/pkgs/top-level/all-packages.nix b/pkgs/top-level/all-packages.nix
index 86eab6704c4..088e1decffe 100644
--- a/pkgs/top-level/all-packages.nix
+++ b/pkgs/top-level/all-packages.nix
@@ -26738,7 +26738,9 @@ in
     git = gitMinimal;
   };
 
-  survex = callPackage ../applications/misc/survex { };
+  survex = callPackage ../applications/misc/survex {
+    inherit (darwin.apple_sdk.frameworks) Carbon Cocoa;
+  };
 
   sunvox = callPackage ../applications/audio/sunvox { };
 

@MatthewCroughan
Copy link
Contributor Author

Screenshot_macOS_2021-06-08_20:20:39

Changes for building on Darwin:
pkgs/data/fonts/unscii/default.nix
pkgs/applications/misc/survex/default.nix
pkgs/top-level/all-packages.nix

That's so great.

@MatthewCroughan
Copy link
Contributor Author

@OPNA2608 I've added your patches as a separate commit and squashed all of the previous commits we have made. I think having a clean patch on supporting the Darwin build is important for historical purposes, and it's something cool I can reference to people and show them how Darwin support was added for this.

@ofborg ofborg bot requested a review from 7c6f434c June 9, 2021 04:21
@MatthewCroughan
Copy link
Contributor Author

Actually, I see that you've modified uniscii, that'll have to be modified in a separate commit right?

Co-authored-by: Fabian Affolter <mail@fabian-affolter.ch>
Co-authored-by: Christoph Neidahl <christoph.neidahl@gmail.com>
Comment on lines +57 to +62
for perltool in './extract-msgs.pl' './gettexttomsg.pl' '$(srcdir)/gdtconvert' '$(srcdir)/gen_img2aven'; do
substituteInPlace src/Makefile.am \
--replace "$perltool" "${perlenv}/bin/perl $perltool"
done
substituteInPlace lib/Makefile.am \
--replace '$(srcdir)/make-pixel-font' '${perlenv}/bin/perl $(srcdir)/make-pixel-font'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@OPNA2608 What exactly does this do differently? My solution seems to work, but this is much more complex. What is the benefit from the added complexity?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I wanted to leave a link to the issue but I forgot about it. Same problem as

# Fixes shebang -> wrapper problem on Darwin

in unscii, relevant issue is #65351 (seems to apply to perl.withPackages too).

Those perl scripts don't execute properly on Darwin. The platform doesn't seem to like when a shebang for an executable file points to a shell script (like the wrapper that perl.withPackages produces so its perl binary can see the packages), so it tries to run those files with the regular shell instead. It's therefore necessary to manually call them with the perl interpreter so their shebangs don't get tried & ignored.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I remove those 6 lines again:

Screenshot_macOS_2021-06-09_19:14:27

@OPNA2608
Copy link
Contributor

Actually, I see that you've modified uniscii, that'll have to be modified in a separate commit right?

Yeah.

@MatthewCroughan
Copy link
Contributor Author

Actually, I see that you've modified uniscii, that'll have to be modified in a separate commit right?

Yeah.

What would the commit message be for what you have done to unscii?

@OPNA2608
Copy link
Contributor

unscii: Fixed Darwin build, because it had the same shebang -> wrapper issue with its perl.withPackages that survex had.

@7c6f434c
Copy link
Member

Looks reasonable

@MatthewCroughan MatthewCroughan force-pushed the add-survex branch 2 times, most recently from 017c70f to 9007ca3 Compare June 14, 2021 01:15
@MatthewCroughan
Copy link
Contributor Author

Looks like this can be merged now. Can anybody take a look? 😄

@7c6f434c 7c6f434c merged commit 6f910f4 into NixOS:master Jun 17, 2021
MatthewCroughan added a commit to tunnelvr/tunnelvr that referenced this pull request Sep 27, 2021
My survex PR to Nixpkgs has been merged, this input is no longer necessary

NixOS/nixpkgs#126145
@MatthewCroughan MatthewCroughan deleted the add-survex branch March 8, 2022 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants