starship: init at 0.10.1 #66730

bbigras · 2019-08-16T20:07:08Z

Motivation for this change

Add the starship package. Which is a promising simple prompt.

Things done

mmahut · 2019-08-16T21:43:20Z

jonringer

since the package is being called from all-packages, just pass the package names in question. Also makes overriding a lot easier.

pkgs/tools/misc/starship/default.nix

bbigras · 2019-08-16T23:00:06Z

Thanks @risicle and @jonringer for the suggestions.

jonringer · 2019-08-16T23:25:41Z

if you could squash all the commits, then the diff LGTM

git reset HEAD~3
git commit --amend --no-edit
git push <fork> <branch> --force

bbigras · 2019-08-16T23:42:05Z

@jonringer I think they have a button in github for that. I squashed them in one of my last PRs and I was told not too. If I remember correctly.

EDIT:

I heard it was better this way so the review don't get obsoleted or something like that. Not sure.

EDIT 2:

Yeah I was told "Next time, could you avoid force-pushing please? We've got a button that lets us squash all the commits at merge time :-)" in one of my last PR.. but on another project. haha.

If you still prefer that I squash it, I'll do it.

risicle · 2019-08-17T00:00:18Z

Hmm fails to build on macos 10.13

error occurred: Command "/nix/store/wmb4c95cyqryqs6adv9hw8gdbnbcffgj-clang-wrapper-7.1.0/bin/cc" "-O3" "-ffunction-sections" "-fdata-sections" "-fPIC" "-m64" "-I" "/private/tmp/nix-build-starship-0.10.1.drv-0/source/target/x86_64-apple-darwin/release/build/libgit2-sys-e37308ba0464081a/out/include" "-I" "libgit2/src" "-I" "libgit2/deps/http-parser" "-I" "libgit2/deps/pcre" "-I" "/private/tmp/nix-build-starship-0.10.1.drv-0/source/target/x86_64-apple-darwin/release/build/libssh2-sys-8abdf09389b53244/out/include" "-I" "/private/tmp/nix-build-starship-0.10.1.drv-0/source/target/x86_64-apple-darwin/release/build/libz-sys-df19de8444dce4eb/out/include" "-fvisibility=hidden" "-DGIT_REGEX_BUILTIN=1" "-DHAVE_STDINT_H=1" "-DHAVE_MEMMOVE=1" "-DNO_RECURSE=1" "-DNEWLINE=10" "-DPOSIX_MALLOC_THRESHOLD=10" "-DLINK_SIZE=2" "-DPARENS_NEST_LIMIT=250" "-DMATCH_LIMIT=10000000" "-DMATCH_LIMIT_RECURSION=MATCH_LIMIT" "-DMAX_NAME_SIZE=32" "-DMAX_NAME_COUNT=10000" "-DSHA1DC_NO_STANDARD_INCLUDES=1" "-DSHA1DC_CUSTOM_INCLUDE_SHA1_C=\"common.h\"" "-DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C=\"common.h\"" "-o" "/private/tmp/nix-build-starship-0.10.1.drv-0/source/target/x86_64-apple-darwin/release/build/libgit2-sys-e37308ba0464081a/out/build/libgit2/src/commit_list.o" "-c" "libgit2/src/commit_list.c" with args "cc" did not execute successfully (status code exit code: 1).

Looking closer...

jonringer · 2019-08-17T00:05:21Z

@bbigras depends on the communit/repo. for nixpkgs, you want to make it so that the maintainers just need to click a button. nixpkgs cares a lot about the cleanliness of their commit history, and generally don't care much about you force pushing.

jonringer · 2019-08-17T00:06:43Z

For example at work, we just squash everything, and you get these `MERGE PR#XXXXXX: Some message" commits, which are hard to quickly read. But no one really cares about commit history at my work either.

jonringer · 2019-08-17T00:10:37Z

Another reason to keep your commits to a minimum is for reverting possible regressions, if you have 4 commits, then they would need to revert the 4, which may not be obvious depending on how you're viewing the history. However, if all of your changes are done in an atomic commit, then it's easy to revert or cherry-pick it.

bbigras · 2019-08-17T00:15:25Z

I totally agree that squashing commits is worth it and I prefer it that way too. Especially in this case where there's one useless commit for every single review suggestions.

I just misremembered which project asked me to let them do it with github.

risicle · 2019-08-17T00:15:55Z

Fix for macos is simple enough, just needs a couple of extra buildInputs:

  ++ stdenv.lib.optional stdenv.isDarwin [ libiconv darwin.apple_sdk.frameworks.Security ]

jonringer · 2019-08-17T00:17:47Z

@bbigras nixpkgs prefers rebasing your commits to keep them as tidy as possible, usually 1 commit = changes to 1 package.

https://nixos.org/nixpkgs/manual/#submitting-changes-hotfixing-pull-requests

bbigras · 2019-08-17T00:24:38Z

nixpkgs prefers rebasing your commits to keep them as tidy as possible, usually 1 commit = changes to 1 package.

Thanks @jonringer, it's clear then.

Fix for macos is simple enough, just needs a couple of extra buildInputs

@risicle I made the change, thanks!

aanderse

Thanks for choosing to contribute this package to nixpkgs! Looks pretty neat, I think I'll give it a try.

I've left some suggestions I hope you will find useful.

pkgs/tools/misc/starship/default.nix

aanderse · 2019-08-17T01:41:19Z

pkgs/tools/misc/starship/default.nix

@@ -0,0 +1,24 @@
+{ stdenv, fetchFromGitHub, rustPlatform, openssl, pkgconfig, libiconv, darwin }:


Does anyone know if it is preferred to pass Security here instead of darwin?

Every time you pass one of the inner frameworks as an argument it has to be assigned specifically in all-packages.nix, which is why I tend to pass the full darwin, which is taken care of by callPackage.

So I can resolve this suggestion and leave it like it is?

@risicle yes, this is why you often see declarations like this inside all-packages.nix:

sit = callPackage ../applications/version-management/sit { inherit (darwin.apple_sdk.frameworks) CoreFoundation Security; };

I'm not a darwin person, and I haven't ever really looked at the documentation about working with darwin in nix, so I'm genuinely asking which is preferred in nixpkgs 🤷‍♂️

Right now, in majority cases we import the entire darwin, but maybe @aanderse has a valid point and we should inherit the frameworks instead.

Well I think I've adequately derailed this PR just about enough. Sorry @bbigras I didn't mean to use your PR to try and set new darwin policy... especially since I've never used the platform!

For future reference it would be nice to know if there is a preferred style. That should probably be a decision left up to people like @matthewbauer and @LnL7, with input from people like @lilyball.

I've seen both, but I personally just take darwin, I don't see any benefit to requiring all-packages.nix to do the inherit. I suppose it makes it a little easier to override the frameworks, but I can't think of any reason to override them.

bbigras · 2019-08-17T04:24:41Z

@aanderse thanks for the review.

pkgs/tools/misc/starship/default.nix

bbigras · 2019-08-17T15:10:04Z

Does anyone know if it is preferred to pass Security here instead of darwin?

Anyone can help with that?

aanderse

Thanks @bbigras! This package looks awesome!

aanderse · 2019-08-18T11:14:16Z

@GrahamcOfBorg build starship

bbigras · 2019-08-18T16:04:45Z

Thanks everyone for the help.

lilyball · 2019-08-19T04:21:18Z

I just tried to build this on darwin and got a test failure:

builder for '/nix/store/paw7d7vrpxxh5riwpznp2488vk4al75p-starship-0.10.1.drv' failed with exit code 101; last 10 log lines:
   right: `None`', src/modules/python.rs:118:9
  note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
  
  
  failures:
      modules::python::tests::test_no_virtual_env
  
  test result: FAILED. 25 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
  
  error: test failed, to rerun pass '--bin starship'
[0 built (1 failed), 114 copied (1724.7 MiB), 372.1 MiB DL]
error: build of '/nix/store/paw7d7vrpxxh5riwpznp2488vk4al75p-starship-0.10.1.drv' failed

lilyball · 2019-08-19T04:26:00Z

I think this is an upstream package issue caused by running tests in parallel, based on the test behavior and implementation. I'll file it upstream.

Edit: Filed as starship/starship#188

ofborg bot added 8.has: package (new) 11.by: package-maintainer 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Aug 16, 2019

jonringer reviewed Aug 16, 2019

View reviewed changes

pkgs/tools/misc/starship/default.nix Outdated Show resolved Hide resolved

pkgs/tools/misc/starship/default.nix Outdated Show resolved Hide resolved

risicle reviewed Aug 16, 2019

View reviewed changes

pkgs/tools/misc/starship/default.nix Outdated Show resolved Hide resolved

bbigras force-pushed the starship branch from fef6873 to bd8589d Compare August 17, 2019 00:22

aanderse requested changes Aug 17, 2019

View reviewed changes

bbigras force-pushed the starship branch from bd8589d to ee48ec6 Compare August 17, 2019 04:21

alexarice reviewed Aug 17, 2019

View reviewed changes

pkgs/tools/misc/starship/default.nix Outdated Show resolved Hide resolved

starship: init at 0.10.1

d90a224

bbigras force-pushed the starship branch from ee48ec6 to d90a224 Compare August 17, 2019 14:18

aanderse approved these changes Aug 18, 2019

View reviewed changes

aanderse merged commit fbff757 into NixOS:master Aug 18, 2019

bbigras deleted the starship branch August 18, 2019 16:04

bbigras mentioned this pull request Aug 19, 2019

starship: 0.10.1 -> 0.12.0 #66947

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

starship: init at 0.10.1 #66730

starship: init at 0.10.1 #66730

bbigras commented Aug 16, 2019

mmahut commented Aug 16, 2019

jonringer left a comment

bbigras commented Aug 16, 2019

jonringer commented Aug 16, 2019 •

edited

bbigras commented Aug 16, 2019 •

edited

risicle commented Aug 17, 2019

jonringer commented Aug 17, 2019

jonringer commented Aug 17, 2019

jonringer commented Aug 17, 2019

bbigras commented Aug 17, 2019

risicle commented Aug 17, 2019

jonringer commented Aug 17, 2019

bbigras commented Aug 17, 2019

aanderse left a comment

aanderse Aug 17, 2019

risicle Aug 17, 2019

bbigras Aug 17, 2019

aanderse Aug 18, 2019

mmahut Aug 18, 2019 •

edited

aanderse Aug 18, 2019

lilyball Aug 19, 2019 •

edited

bbigras commented Aug 17, 2019

bbigras commented Aug 17, 2019

aanderse left a comment

aanderse commented Aug 18, 2019

bbigras commented Aug 18, 2019

lilyball commented Aug 19, 2019

lilyball commented Aug 19, 2019 •

edited

		@@ -0,0 +1,24 @@
		{ stdenv, fetchFromGitHub, rustPlatform, openssl, pkgconfig, libiconv, darwin }:

starship: init at 0.10.1 #66730

starship: init at 0.10.1 #66730

Conversation

bbigras commented Aug 16, 2019

Motivation for this change

Things done

mmahut commented Aug 16, 2019

jonringer left a comment

Choose a reason for hiding this comment

bbigras commented Aug 16, 2019

jonringer commented Aug 16, 2019 • edited

bbigras commented Aug 16, 2019 • edited

risicle commented Aug 17, 2019

jonringer commented Aug 17, 2019

jonringer commented Aug 17, 2019

jonringer commented Aug 17, 2019

bbigras commented Aug 17, 2019

risicle commented Aug 17, 2019

jonringer commented Aug 17, 2019

bbigras commented Aug 17, 2019

aanderse left a comment

Choose a reason for hiding this comment

aanderse Aug 17, 2019

Choose a reason for hiding this comment

risicle Aug 17, 2019

Choose a reason for hiding this comment

bbigras Aug 17, 2019

Choose a reason for hiding this comment

aanderse Aug 18, 2019

Choose a reason for hiding this comment

mmahut Aug 18, 2019 • edited

Choose a reason for hiding this comment

aanderse Aug 18, 2019

Choose a reason for hiding this comment

lilyball Aug 19, 2019 • edited

Choose a reason for hiding this comment

bbigras commented Aug 17, 2019

bbigras commented Aug 17, 2019

aanderse left a comment

Choose a reason for hiding this comment

aanderse commented Aug 18, 2019

bbigras commented Aug 18, 2019

lilyball commented Aug 19, 2019

lilyball commented Aug 19, 2019 • edited

jonringer commented Aug 16, 2019 •

edited

bbigras commented Aug 16, 2019 •

edited

mmahut Aug 18, 2019 •

edited

lilyball Aug 19, 2019 •

edited

lilyball commented Aug 19, 2019 •

edited