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

tunnelx: minor refactor and cleanup #243521

Merged
merged 2 commits into from
Jul 16, 2023
Merged

tunnelx: minor refactor and cleanup #243521

merged 2 commits into from
Jul 16, 2023

Conversation

drupol
Copy link
Contributor

@drupol drupol commented Jul 14, 2023

Follow up of #239739

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.05 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.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Jul 14, 2023
@drupol drupol force-pushed the tunnelx/update-buildinputs branch 2 times, most recently from 4ccd72d to 88592ff Compare July 14, 2023 19:45
@drupol
Copy link
Contributor Author

drupol commented Jul 14, 2023

@NixOS/darwin-maintainers I would like some help with this derivation. I'm pretty sure it's possible to run it on Darwin.

@goatchurchprime Could you confirm it works on Darwin architecture?

@drupol drupol force-pushed the tunnelx/update-buildinputs branch from 88592ff to bd7ccfc Compare July 14, 2023 21:01
@drupol
Copy link
Contributor Author

drupol commented Jul 14, 2023

From @Cu3PO42

The log states that the Mesa package is marked as broken on Mac. The package definition references this issue: https://gitlab.freedesktop.org/mesa/mesa/-/issues/8634 which does not appear to be resolved yet.

I'll set platforms to Linux only.

@emilazy
Copy link
Member

emilazy commented Jul 14, 2023

I do not believe Mesa should be required for this package on macOS from what I'm seeing. Investigating now though.

@drupol
Copy link
Contributor Author

drupol commented Jul 14, 2023

Thanks :)

@emilazy
Copy link
Member

emilazy commented Jul 14, 2023

This package builds but I worry I may have broken the GUI components of survex in the process. I'm afraid I've never gone caving outside of Colossal Cave Adventure before; could you tell me how to exercise tunnelx's dependencies on those binaries?

@reckenrode
Copy link
Contributor

There’s also a tracking issue for Mesa on Darwin.

#233265

@drupol
Copy link
Contributor Author

drupol commented Jul 14, 2023

I have no clue, maybe @goatchurchprime can help ?

@emilazy
Copy link
Member

emilazy commented Jul 14, 2023

Okay, I source dived and checked the survex executables tunnelx uses; they all start with this patch, even the aven GUI. Can't guarantee that this hasn't broken something on the survex side but it's clearly an improvement over the status quo; I'm going to call this good enough:

diff --git a/pkgs/applications/gis/tunnelx/default.nix b/pkgs/applications/gis/tunnelx/default.nix
index 174796b3bf5..21cc485e752 100644
--- a/pkgs/applications/gis/tunnelx/default.nix
+++ b/pkgs/applications/gis/tunnelx/default.nix
@@ -36,9 +36,10 @@ stdenv.mkDerivation (finalAttrs: {
 
     mkdir -p $out/bin $out/java
     cp -r symbols Tunnel tutorials $out/java
+    # `SURVEX_EXECUTABLE_DIR` must include trailing slash
     makeWrapper ${jre}/bin/java $out/bin/tunnelx \
       --add-flags "-cp $out/java Tunnel.MainBox" \
-      --set SURVEX_EXECUTABLE_DIR ${lib.getBin survex}/bin \
+      --set SURVEX_EXECUTABLE_DIR ${lib.getBin survex}/bin/ \
       --set TUNNEL_USER_DIR $out/java/
 
     runHook postInstall
diff --git a/pkgs/applications/misc/survex/default.nix b/pkgs/applications/misc/survex/default.nix
index b23cd02220b..6f2098ea477 100644
--- a/pkgs/applications/misc/survex/default.nix
+++ b/pkgs/applications/misc/survex/default.nix
@@ -45,14 +45,16 @@ stdenv.mkDerivation rec {
   buildInputs = [
     ffmpeg
     glib
-    libGLU
-    mesa
     proj
     wxGTK32
   ] ++ lib.optionals stdenv.hostPlatform.isDarwin [
     Carbon
     Cocoa
   ] ++ lib.optionals stdenv.hostPlatform.isLinux [
+    # TODO: libGLU doesn't build for macOS because of Mesa issues
+    # (#233265); is it required for anything?
+    libGLU
+    mesa
     libICE
     libX11
   ];

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Jul 14, 2023
@drupol drupol force-pushed the tunnelx/update-buildinputs branch from bd7ccfc to 3ecdb1d Compare July 15, 2023 17:03
@ofborg ofborg bot requested a review from MatthewCroughan July 15, 2023 17:21
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1 labels Jul 15, 2023
@drupol drupol merged commit 0ccfe60 into master Jul 16, 2023
@drupol drupol deleted the tunnelx/update-buildinputs branch July 16, 2023 13:00
@MatthewCroughan
Copy link
Contributor

Unbelievable work, completely unexpected and brilliant. Thank you so much!

@drupol
Copy link
Contributor Author

drupol commented Jul 16, 2023

I was feeling a little bit concerned for merging the related pr too quickly and noticing too late that I missed plenty of things to do ... And in the meantime, the Darwin maintainers improved the Darwin support :)

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.

5 participants