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.

Sign up for GitHub

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

Jump to bottom

swipl: include GUI tools #70724

Merged
merged 1 commit into from
Oct 14, 2019
Merged

swipl: include GUI tools #70724

merged 1 commit into from
Oct 14, 2019
+5 −1

Conversation

yrashk
Copy link
Contributor

@yrashk yrashk commented Oct 8, 2019 •
edited
Loading

swi-prolog currently misses GUI packages / tools (xpce, prolog_ide)
due to missing X dependencies.

Motivation for this change

SWI-Prolog offers a good range of GUI tools

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.
Notify maintainers

cc @meditans

Sorry, something went wrong.

@jonringer
Copy link
Contributor

adding xorg to a closure can cause it to become huge. I would put in some flags on whether or not to build with xorg in mind. that way we can have a cli and gui package. Something like:

swiProlog
swiPrologWithGui

@worldofpeace thoughts?

example package: https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/tools/build-managers/cmake/default.nix
example all-packages overrides:

nixpkgs/pkgs/top-level/all-packages.nix

Line 9413 in 9acb145

cmake = libsForQt5.callPackage ../development/tools/build-managers/cmake { };

@yrashk
Copy link
Contributor Author

yrashk commented Oct 10, 2019

@jonringer Jon, I updated the PR. Does this look any better?

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Oct 10, 2019
jonringer
jonringer reviewed Oct 10, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

just a few suggestions, otherwise looks good code wise.

@worldofpeace thoughts?

pkgs/development/compilers/swi-prolog/default.nix Outdated
}:

let
version = "8.1.4";
packInstall = swiplPath: pack:
''${swiplPath}/bin/swipl -g "pack_install(${pack}, [package_directory(\"${swiplPath}/lib/swipl/pack\"), silent(true), interactive(false)])." -t "halt."
'';
guiInputs = (if stdenv.isDarwin then [] else [ libXpm libX11 libXext libXft libXinerama libjpeg ]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
guiInputs = (if stdenv.isDarwin then [] else [ libXpm libX11 libXext libXft libXinerama libjpeg ]);
guiInputs = lib.optionals !stdenv.isDarwin [ libXpm libX11 libXext libXft libXinerama libjpeg ];

pkgs/development/compilers/swi-prolog/default.nix Outdated
@@ -25,6 +28,7 @@ stdenv.mkDerivation {
buildInputs = [ cacert git cmake gmp readline openssl
libarchive libyaml db pcre libedit libossp_uuid
zlib pkgconfig ]
++ (if gui then guiInputs else [])
Copy link
Contributor

Choose a reason for hiding this comment

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

generally i see a "withXXX" prefix used when passing flags to an expression, that way it's not confused with a package.

Suggested change
++ (if gui then guiInputs else [])
++ lib.optionals withGui guiInputs

@yrashk
Copy link
Contributor Author

yrashk commented Oct 10, 2019

@jonringer I've updated the PR. Looks good now?

@jonringer
Copy link
Contributor

hmm, now on darwin, if you do swiPrologWithGui you'll just get the none-gui version, which isn't a good experience. We'll probably want to edit the supported platforms to reflect this

meta = ... {
  platforms = with platforms; linux ++ lib.optional !withGui darwin;
  ...
};

this way darwin will get a "this is not a supported platform" when trying to build, it will also make hydra avoid building the gui version

@worldofpeace thoughts?

@yrashk
swipl: include GUI tools
e0d7a96 
swi-prolog currently misses GUI packages / tools (xpce, prolog_ide)
due to missing X dependencies.
@yrashk
Copy link
Contributor Author

yrashk commented Oct 10, 2019

@jonringer Makes sense. Updated the PR.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin and removed 10.rebuild-darwin: 1-10 labels Oct 10, 2019
@worldofpeace
Copy link
Contributor

This is also an interesting alternative @jonringer:

diff --git a/pkgs/development/compilers/swi-prolog/default.nix b/pkgs/development/compilers/swi-prolog/default.nix
index 8c79deb9832..ea91ca9b458 100644
--- a/pkgs/development/compilers/swi-prolog/default.nix
+++ b/pkgs/development/compilers/swi-prolog/default.nix
@@ -5,9 +5,11 @@
 , libjpeg, libX11, libXext, libXft, libXinerama
 , extraLibraries ? [ jdk unixODBC libXpm libSM libXt freetype fontconfig ]
 , extraPacks     ? []
-, withGui ? false
+, withGui ? false && (!stdenv.isDarwin)
 }:
 
+assert stdenv.lib.assertMsg (withGui -> !stdenv.isDarwin) "swi-prolog cannot be built with gui's on Darwin.";
+
 let
   version = "8.1.4";
   packInstall = swiplPath: pack:
@@ -27,7 +29,7 @@ stdenv.mkDerivation {
   buildInputs = [ cacert git cmake gmp readline openssl
     libarchive libyaml db pcre libedit libossp_uuid
     zlib pkgconfig ]
-  ++ stdenv.lib.optionals (withGui && !stdenv.isDarwin) [ libXpm libX11 libXext libXft libXinerama libjpeg ]
+  ++ stdenv.lib.optionals withGui [ libXpm libX11 libXext libXft libXinerama libjpeg ]
   ++ extraLibraries
   ++ stdenv.lib.optional stdenv.isDarwin makeWrapper;
 
@@ -71,7 +73,7 @@ stdenv.mkDerivation {
     description = "A Prolog compiler and interpreter";
     license = "LGPL";
 
-    platforms = stdenv.lib.platforms.linux ++ stdenv.lib.optionals (!withGui) stdenv.lib.platforms.darwin;
+    platforms = stdenv.lib.platforms.unix;
     maintainers = [ stdenv.lib.maintainers.meditans ];
   };
 }

I think either is fine. Though here we get across what's not supported.

@jonringer
Copy link
Contributor

I'm good with either case. As long as darwin users aren't expecting a gui installation and get something else, im good :)

@yrashk
Copy link
Contributor Author

yrashk commented Oct 14, 2019

@jonringer @worldofpeace should I update the PR with the above change?

worldofpeace
worldofpeace approved these changes Oct 14, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

LGTM.

@worldofpeace
Copy link
Contributor

@yrashk I think it's sufficient currently.

@worldofpeace worldofpeace merged commit 516e61a into NixOS:master Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonringer jonringer

@worldofpeace worldofpeace

Assignees
No one assigned
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@yrashk @jonringer @worldofpeace