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

stacer: init at 1.1.0 #77808

Closed
wants to merge 1 commit into from
Closed

stacer: init at 1.1.0 #77808

wants to merge 1 commit into from

Conversation

jonringer
Copy link
Contributor

@jonringer jonringer commented Jan 16, 2020

Motivation for this change

wanted a nice looking monitoring tool

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.
$ nix path-info -Sh ./result
/nix/store/gagj7l3izf0vs91p42cp312d9lrzigv5-stacer-1.1.0	 283.7M

@dtzWill
Copy link
Member

dtzWill commented Jan 16, 2020

LGTM, ran it-- it does look nice!

Some of the features don't make sense for NixOS, so didn't try them to find out how that went but I don't think there's anything to be done about that.

Haven't looked through the expression yet, will do it in the morning if not merged by then already ;).

@jonringer
Copy link
Contributor Author

@GrahamcOfBorg build stacer

maintainers = with maintainers; [ jonringer ];
};
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change


buildInputs = [ qtcharts ];

enableParallelBuilding = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is done by default with cmake

Suggested change
enableParallelBuilding = true;

@worldofpeace
Copy link
Contributor

The program is not completely hermetic because it tries to launch executables out of band with https://github.com/oguzhaninan/Stacer/search?p=1&q=CommandUtil%3A%3Aexec&unscoped_q=CommandUtil%3A%3Aexec

I'm not sure if it's ok for the administration tool to have hardcoded paths to executables where appropriate, because some could be optional features or ones, as @dtzWill, don't make sense in NixOS. Example of what I recommend #65252 (comment). It's also cool to inject into PATH via the wrapper, but that's less reliable.

@jonringer
Copy link
Contributor Author

jonringer commented Jan 16, 2020

The program is not completely hermetic because it tries to launch executables out of band with https://github.com/oguzhaninan/Stacer/search?p=1&q=CommandUtil%3A%3Aexec&unscoped_q=CommandUtil%3A%3Aexec

I'm fine with wrapping, it doesn't look like they use any paths, just command names

@worldofpeace
Copy link
Contributor

I'm having problems with it starting/using on my machine like oguzhaninan/Stacer#349. Strace reveals weird stuff not related to any nixpkgs shenanigans so I'm going to assume it's an undiscovered bug/s upstream.

@jonringer
Copy link
Contributor Author

I'm not sure the nature of the issues, closing as I've lost interest in this

@jonringer jonringer closed this Feb 22, 2020
@jonringer jonringer deleted the add-stracer branch February 22, 2020 18:28
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

3 participants