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

bashtop: init at 0.8.23 #86673

Open
wants to merge 1 commit into
base: master
from
Open

bashtop: init at 0.8.23 #86673

wants to merge 1 commit into from

Conversation

@filalex77
Copy link
Contributor

filalex77 commented May 3, 2020

Motivation for this change

https://github.com/aristocratos/bashtop/releases/tag/v0.8.23

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 nixpkgs-review --run "nixpkgs-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.
sha256 = "0y6yxm2vmbz0373cfdl6mjh8vhs0r0wcng82n304klx90qxg3ljp";
};

buildInputs = [ bash_5 curl lm_sensors procps-ng sysstat ];

This comment has been minimized.

Copy link
@cole-h

cole-h May 3, 2020

Member
Suggested change
buildInputs = [ bash_5 curl lm_sensors procps-ng sysstat ];
buildInputs = [ bash_5 curl lm_sensors procps sysstat ];

Sorry, forgot to suggest this change, too.

This comment has been minimized.

Copy link
@filalex77

filalex77 May 3, 2020

Author Contributor

And I forgot to try building it. Thanks!

Fixups:
- bashtop: use procps instead of procps-ng alias

Co-authored-by: Cole Helbling <cole.e.helbling@outlook.com>
Copy link
Member

cole-h left a comment

When I run this in a pure nix-shell (nix-shell -p bashtop --pure -I nixpkgs=.), the net and processes widgets don't work, nor does it detect the name of my CPU, and exiting leaves artifacts from the UI. Sorry I can't help much more than that, at the moment. Image of uncleared terminal attached below:

image

That said, in a normal nix-shell (like the one nixpkgs-review drops you into), it works perfectly. Probably impurities of my Arch+Nix system leaking through.

@ehmry
Copy link
Member

ehmry commented May 4, 2020

Works fine for me on NixOS.

@ehmry
ehmry approved these changes May 4, 2020
@ehmry
Copy link
Member

ehmry commented May 4, 2020

Can make a feature request for a .desktop launcher file?

@filalex77
Copy link
Contributor Author

filalex77 commented May 4, 2020

@ehmry Do you mean for upstream or as part of this PR?

@ehmry
Copy link
Member

ehmry commented May 4, 2020

As part of the PR to make life easy for the Nix hostages. Just a suggestion, definitely not a blocker.

@cole-h
cole-h approved these changes May 4, 2020
Copy link
Member

cole-h left a comment

If it works fine on NixOS, that's good enough for me. Diff LGTM, runs fine (in a not-pure shell :P)

[3 built, 3 copied (2.2 MiB), 0.6 MiB DL]
https://github.com/NixOS/nixpkgs/pull/86673
1 package built:
bashtop
Copy link
Contributor

srhb left a comment

The buildInputs are not being carried over as runtime deps, which is the cause of the failure under a pure nix shell -- they're simply expected to (impurely) be present.

You can easily reproduce this from a nixpkgs-review shell by running nix-shell --pure. Then, try editing the nixpkgs-review shell.nix, manually adding the dependencies. Rerun the pure shell, and now it's working.

The paths to sed, ps, ... should either be patched in or the entire tool should be wrapped with a PATH that pulls in the deps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.