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

starship: 0.10.1 -> 0.12.0 #66947

Merged
merged 1 commit into from Aug 23, 2019
Merged

starship: 0.10.1 -> 0.12.0 #66947

merged 1 commit into from Aug 23, 2019

Conversation

bbigras
Copy link
Contributor

@bbigras bbigras commented Aug 19, 2019

Motivation for this change

Fix the parallel tests on darwin issue and other stuff. @lilyball

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.

nix path-info -S /nix/store/lax8qmvfkdj634ism8jafw4djvxjsxv6-starship-0.10.1
/nix/store/lax8qmvfkdj634ism8jafw4djvxjsxv6-starship-0.10.1 35764408

nix path-info -S /nix/store/sb4iaff8xksw7x37b4llbiw0938wvh45-starship-0.11.0
/nix/store/sb4iaff8xksw7x37b4llbiw0938wvh45-starship-0.11.0 35769720

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.

nix-review passes on NixOS
diff LGTM (trivial bump)
executable seems to work
leaf package

[4 built, 37 copied (595.2 MiB), 161.4 MiB DL]
https://github.com/NixOS/nixpkgs/pull/66947
1 package were build:
starship

@lilyball
Copy link
Member

Alas, more test failures when building this on macOS 🤦‍♀

@lilyball
Copy link
Member

Looks like this is already being tracked at starship/starship#199

@lilyball
Copy link
Member

It seems the test failures boil down to starship module directory --path=somethingThatDoesntExist panicking. I'm surprised you were able to build it on NixOS since the check phase fails for me when HOME=/homeless-shelter or when building with sandboxing (due to /etc not being available).

This is being tracked in starship/starship#199. In the meantime, we could just disable the check phase on the package.

@jonringer
Copy link
Contributor

I would first try HOME=$TMPDIR, if that doesnt work then, checkPhase = "cargo test --all --exclude=directory". I haven't done too much with rust packages, but there should be a way to selectively disable a subset of tests

@bbigras
Copy link
Contributor Author

bbigras commented Aug 19, 2019

or when building with sandboxing (due to /etc not being available).

That's weird. I'm pretty sure I have sandboxing (it's been enabled by default for a while, right?).

➜ nix run nixpkgs.nix-info -c nix-info -m

  • system: "x86_64-linux"
  • host os: Linux 4.19.67, NixOS, 19.03.173310.35841f87afa (Koi)
  • multi-user?: yes
  • sandbox: yes
  • version: nix-env (Nix) 2.2.2
  • channels(bbigras): "home-manager-19.03, unstable-19.09pre188690.8746c77a383"
  • channels(root): "nixos-19.03.173310.35841f87afa"
  • nixpkgs: /nix/var/nix/profiles/per-user/root/channels/nixos

I'm the one who set /etc in that test. It was using /usr before and didn't work for me.
I modified the test to list all directories in / and I saw:

  • /dev
  • /etc
  • /nix
  • /build
  • /tmp
  • /bin
  • /proc

Is there something wrong on my side?

@bbigras
Copy link
Contributor Author

bbigras commented Aug 19, 2019

I would first try HOME=$TMPDIR, if that doesnt work then, checkPhase = "cargo test --all --exclude=directory". I haven't done too much with rust packages, but there should be a way to selectively disable a subset of tests

There's cargo test -- --skip some_test

@lilyball
Copy link
Member

Maybe sandboxing on NixOS allows access to /etc? I'm only assuming it disallows it on macOS because the test failed. I'm not sure how to actually test what's available right now.

@lilyball
Copy link
Member

Looking at https://github.com/NixOS/nix/blob/6924bdf2bf8f50f2e6ec5d490571594450aba13a/src/libstore/sandbox-defaults.sb, the macOS sandbox allows for reading the metadata of /etc but does not otherwise allow for reading this directory, which means starship will fail as I assume it's looking for $path/.git.

@bbigras
Copy link
Contributor Author

bbigras commented Aug 20, 2019

The test using /etc that I was referring to is test directory::directory_in_root. Not directory::home_directory.

If you want to list all the directory while in the sandbox you can build starship on the starship-list-dirs branch of my fork http://github.com/bbigras/nixpkgs. I added the read_dir() stuff in a new unit test.

@bbigras
Copy link
Contributor Author

bbigras commented Aug 20, 2019

If I change test directory::directory_in_root to read /etc2, create it and run the test with strace -f I get:

$ rg etc2 strace.txt
6603:[pid  8080] execve("./target/debug/starship", ["./target/debug/starship", "module", "directory", "--path=/etc2"], 0x7f20c0002180 /* 2 vars */ <unfinished ...>
6800:[pid  8080] openat(AT_FDCWD, "/etc2", O_RDONLY|O_NONBLOCK|O_CLOEXEC|O_DIRECTORY) = 3
6906:[pid  8080] lstat("/etc2", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
6907:[pid  8080] stat("/etc2/.git", 0x7ffef836f9a0) = -1 ENOENT (No such file or directory)
6908:[pid  8080] stat("/etc2", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
6909:[pid  8080] stat("/etc2/commondir", 0x7ffef836f770) = -1 ENOENT (No such file or directory)
6910:[pid  8080] stat("/etc2/HEAD", 0x7ffef836f770) = -1 ENOENT (No such file or directory)
6922:[pid  8080] write(1, "in \33[1;36m/etc2\33[0m ", 20) = 20
6927:[pid  8079] <... read resumed> "in \33[1;36m/etc2\33[0m ", 32) = 20

@lilyball
Copy link
Member

Listing the contents of / isn't helpful, that would only tell you something different if you're building a new FHS userenv (which macOS doesn't even have).

@bbigras
Copy link
Contributor Author

bbigras commented Aug 20, 2019

I updated the PR. directory::home_directory should be skipped now.

@lilyball
Copy link
Member

directory::directory_in_root still fails on macOS though.

@bbigras
Copy link
Contributor Author

bbigras commented Aug 20, 2019

OK I excluded that one too.

@lilyball
Copy link
Member

This now builds on macOS

[2 built, 0.0 MiB DL]
https://github.com/NixOS/nixpkgs/pull/66947
1 package were build:
starship

@bbigras
Copy link
Contributor Author

bbigras commented Aug 20, 2019

There's already another release.
Should I update this PR, open a new one or just wait a couple of days between PRs so I don't waste everyone's time. I don't mind doing a PR for every single releases but I know there's a lot of PRs to be reviewed every day.

@jonringer
Copy link
Contributor

I would just bump, rebase, and push. You already went through quite a gauntlet of reviews :)

@bbigras bbigras changed the title starship: 0.10.1 -> 0.11.0 starship: 0.10.1 -> 0.12.0 Aug 20, 2019
@bbigras
Copy link
Contributor Author

bbigras commented Aug 20, 2019

done

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.

nix-review passes on NixOS
diff LGTM
binary seems to work
leaf package

[4 built, 0.0 MiB DL]
https://github.com/NixOS/nixpkgs/pull/66947
1 package were build:
starship

Copy link
Member

@lilyball lilyball left a comment

Choose a reason for hiding this comment

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

This fails to build on macOS, It needs the Security framework now.

builder for '/nix/store/nmfi9qzqg6acrkfkcmvfwjcb6kxcggw7-starship-0.12.0-vendor.drv' failed with exit code 1; last 10 log lines:
    Referenced from: /nix/store/2bbgqwpafjv7pzfa0027wkqp9k56s60a-cargo-1.36.0/bin/.cargo-wrapped
    Reason: no suitable image found.  Did find:
  	/System/Library/Frameworks/Security.framework/Versions/A/Security: file system sandbox blocked stat()
  	/System/Library/Frameworks/Security.framework/Versions/A/Security: file system sandbox blocked stat()
  Traceback (most recent call last):
    File "/nix/store/84c6nr9wqk77wlasa0nyxjnp2nj5bxd4-cargo-vendor-normalise/bin/.cargo-vendor-normalise-wrapped", line 42, in <module>
      main()
    File "/nix/store/84c6nr9wqk77wlasa0nyxjnp2nj5bxd4-cargo-vendor-normalise/bin/.cargo-vendor-normalise-wrapped", line 17, in main
      assert list(data.keys()) == ["source"]
  AssertionError
cannot build derivation '/nix/store/vsr8ha88ggagv5f7y34pvl1afsw1vnrl-starship-0.12.0.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/a654z46clgvl76msiyh009pn41w8g4fm-env.drv': 1 dependencies couldn't be built
[2 built (1 failed), 132 copied (1645.9 MiB), 373.2 MiB DL]
error: build of '/nix/store/a654z46clgvl76msiyh009pn41w8g4fm-env.drv' failed
https://github.com/NixOS/nixpkgs/pull/66947
1 package failed to build:
starship

};

buildInputs = [ openssl ] ++ stdenv.lib.optionals stdenv.isDarwin [ libiconv darwin.apple_sdk.frameworks.Security ];
nativeBuildInputs = [ pkgconfig ];

cargoSha256 = "126y8q19qr37wrj6x4hqh0v7nqr9yfrycwqfgdlaw6i33gb0qam9";
cargoSha256 = "0qlgng5j6l1r9j5vn3wnq25qr6f4nh10x90awiqyzz8jypb0ng2c";
checkPhase = "cargo test -- --skip directory::home_directory --skip directory::directory_in_root";
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: Add a comment above this phase linking to starship/starship#204 as that's the issue that tracks the reason for skipping these tests. This way we'll have an easy reference for when this overridden phase can be deleted.

@lilyball
Copy link
Member

Looking at the derivation, it already includes Security, so I guess the problem is actually that it's referencing the system Security framework rather than the Nix one. I'm not sure what's going on there.

@lilyball
Copy link
Member

The failure is actually when trying to execute the wrapped cargo binary, so maybe something happened upstream.

@lilyball
Copy link
Member

Ok I've figured out what's going on. This review was done on a different machine, whose /etc/nix/nix.conf doesn't match the one on my previous machine (in particular it's missing the extra-sandbox-paths that allows access to things like /System/Library/Frameworks because darwin impurity requires linking against the system for some of these frameworks). I'm re-running the review on my original machine and I'll figure out how to fix nix.conf on this one later.

@jonringer
Copy link
Contributor

darwin sounds fun... :)

Copy link
Member

@lilyball lilyball left a comment

Choose a reason for hiding this comment

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

Looks good on darwin.

[5 built, 0.0 MiB DL]
https://github.com/NixOS/nixpkgs/pull/66947
1 package were build:
starship

The binary executes as well.

Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

Merging entirely based on the good word of @lilyball and @jonringer. Thanks all!

@aanderse aanderse merged commit 5ffe9b8 into NixOS:master Aug 23, 2019
@bbigras bbigras deleted the starship branch August 29, 2019 17:07
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.

None yet

4 participants