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

Jump to bottom

nushell: init at 0.5.0 #73121

Merged
merged 2 commits into from Nov 13, 2019
Merged

nushell: init at 0.5.0 #73121

merged 2 commits into from Nov 13, 2019

Conversation

marsam
Copy link
Contributor

@marsam marsam commented Nov 9, 2019

Motivation for this change

Add https://github.com/nushell/nushell

supersedes and closes #68279

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 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 @evanjs @teto I think you were interested in this package

@evanjs
Copy link
Member

evanjs commented Nov 9, 2019 •
edited

Sweet! Figured we’d have to skip most if not all of the tests.
If only we could set the home directory for the duration of the checkPhase, I think?

@marsam
Copy link
Contributor Author

marsam commented Nov 9, 2019 •
edited

If only we could set the home directory for the duration of the checkPhase, I think?

Thanks for the hint. I've added it as preCheck phase and the tests pass

@evanjs
Copy link
Member

evanjs commented Nov 9, 2019

If only we could set the home directory for the duration of the checkPhase, I think?

Thanks for the hint. I've added it as preCheck phase and the tests pass

Wow. That’s all it took?!?
Duly noted. Thanks!

@evanjs
Copy link
Member

evanjs commented Nov 10, 2019

Tested on NixOS 19.09 and macOS 10.13.6.
Seems to compile and work fine.

Thanks again!

@evanjs
Copy link
Member

evanjs commented Nov 10, 2019

I do wonder if we should allow feature flags to be passed through, though.
If enabled, pass through --all-features with necessary dependencies, or something like that? 🤷‍♂️
https://github.com/nushell/nushell#local

@evanjs
Copy link
Member

evanjs commented Nov 10, 2019

Okay, I think I got it working for both systems.
Compiles fine on Linux and Darwin and they both have the binaryview command which is an optional feature.

diff --git a/pkgs/shells/nushell/default.nix b/pkgs/shells/nushell/default.nix
index 6e075b7ceee..767aa9eecfa 100644
--- a/pkgs/shells/nushell/default.nix
+++ b/pkgs/shells/nushell/default.nix
@@ -3,8 +3,12 @@
 , rustPlatform
 , openssl
 , pkg-config
+, allFeatures ? true
+, xorg
+, python3
 , libiconv
 , Security
+, AppKit
 }:
 
 rustPlatform.buildRustPackage rec {
@@ -23,7 +27,10 @@ rustPlatform.buildRustPackage rec {
   nativeBuildInputs = [ pkg-config ];
 
   buildInputs = stdenv.lib.optionals stdenv.isLinux [ openssl ]
-    ++ stdenv.lib.optionals stdenv.isDarwin [ libiconv Security ];
+    ++ stdenv.lib.optionals stdenv.isDarwin [ libiconv Security ]
+    ++ stdenv.lib.optionals allFeatures [ (if stdenv.isDarwin then AppKit else xorg.libX11) python3 ];
+
+  cargoBuildFlags = stdenv.lib.optional allFeatures "--features all";
 
   preCheck = ''
     export HOME=$TMPDIR
diff --git a/pkgs/top-level/all-packages.nix b/pkgs/top-level/all-packages.nix
index ee936c9a1b2..3c6ef4685d7 100644
--- a/pkgs/top-level/all-packages.nix
+++ b/pkgs/top-level/all-packages.nix
@@ -16531,7 +16531,7 @@ in
   musl = callPackage ../os-specific/linux/musl { };
 
   nushell = callPackage ../shells/nushell {
-    inherit (darwin.apple_sdk.frameworks) Security;
+    inherit (darwin.apple_sdk.frameworks) AppKit Security;
   };
 
   nettools = if stdenv.isLinux then callPackage ../os-specific/linux/net-tools { }

The only weird item I think is the dep on python3 for all-features.
Can't figure out where it comes from (at least with just (rip)grep 🤷‍♂️

@marsam @evanjs
nushell: add withAllFeatures flag
a8942c6 
Co-authored-by: Evan Stoll <evanjs@users.noreply.github.com>
@marsam
Copy link
Contributor Author

marsam commented Nov 12, 2019

@evanjs sorry for the delay, I've applied your patch adding you as co-author :)

Can't figure out where it comes from (at least with just (rip)grep

cargoDeps is accesible outside of the derivation, so you could do something like:

$ rg python3 $(nix-build -A nushell.cargoDeps)

image
in this case, the python3 requirement comes from xcb as build dependency

@marsam marsam merged commit 1933ae3 into NixOS:master Nov 13, 2019
@marsam marsam deleted the init-nushell branch November 13, 2019 03:33
@teto
Copy link
Member

teto commented Nov 19, 2019

finally got around to testing it. Cool ! thanks for doing it <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@marsam @evanjs @teto