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

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

This comment has been minimized.

Copy link
Member

evanjs commented Nov 9, 2019

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 marsam force-pushed the marsam:init-nushell branch from 8b39664 to d4c9e56 Nov 9, 2019
@marsam

This comment has been minimized.

Copy link
Contributor Author

marsam 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

@evanjs

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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 🤷‍♂

Co-authored-by: Evan Stoll <evanjs@users.noreply.github.com>
@marsam marsam force-pushed the marsam:init-nushell branch from d4c9e56 to a8942c6 Nov 12, 2019
@marsam

This comment has been minimized.

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
15 checks passed
15 checks passed
nushell on aarch64-linux Failure
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
nushell on x86_64-linux Success
Details
@marsam marsam deleted the marsam:init-nushell branch Nov 13, 2019
@teto

This comment has been minimized.

Copy link
Contributor

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
Projects
None yet
3 participants
You can’t perform that action at this time.