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

Add NixOS support #70

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Add NixOS support #70

wants to merge 7 commits into from

Conversation

balsoft
Copy link

@balsoft balsoft commented Jun 9, 2018

This PR makes this extension work in NixOS, and presumably makes it work in any distro without installation. If user doesn't want to get prompted for every time they change settings, they need to install.
Stuff left to do:

  • Check that extension works in NixOS/GuixSD on multiple machines (i.e. CPU's)
    • AMD
    • Intel
      • intel_pstate
      • ACPI
  • Check that this PR does not make any regression (Extension still works in other distros)
    • Debian family
    • RedHat family
    • Arch family
  • Check that extension works without installation
  • Check that installation is possible

@balsoft
Copy link
Author

balsoft commented Jun 9, 2018

@konkor I can do testing for non-NixOS distros in VM. Could you do some native testing?

@konkor
Copy link
Owner

konkor commented Jun 10, 2018

@balsoft Great job! You are really like your distro :)
I'll check and test it all. I'm testing the extension from USB stick with Live images of Ubuntu, Fedora, Manjaro Gnome. The extension doesn't work in VM.
I think the installation is still required for fixing of ROOT permissions. It works on your system because you have added the JS rule for it as root. But Debian family (even Ubuntu 18.04 has policykit 0.105) doesn't support it yet and requiring action policy installation.
I'll let you know more when I dig inside it!

@balsoft
Copy link
Author

balsoft commented Jun 10, 2018

@konkor You're right! I've removed the rule from /etc/nixos/configuration.nix, but forgot to nixos-rebuild switch! If I do that, extension hangs the shell. I've had that exact bug before - I think it's because GLib.spawn_sync doesn't like user interactions. Maybe it's just a bug in NixOS - could you try to run GLib.spawn_command_line_sync("notify-send hello") on your system (on my system, gnome-shell hangs on that). That's a really weird behaviour. GLib.spawn_async works in these situations. Maybe we could switch to using that?

@balsoft
Copy link
Author

balsoft commented Jun 10, 2018

Fixed. Works on both gnome-shell 3.26.2 and 3.28. (NixOS 18.03 & NixOS 18.09).

@konkor
Copy link
Owner

konkor commented Jun 10, 2018

@balsoft notify-send works well after installing of ruby-notify package.
it's not convenient to use async calls everywhere. Yeah it's possible to make all async.
I think I'll create the nixos branch bcs master is using a few CI services to build latest stable versions.

@balsoft
Copy link
Author

balsoft commented Jun 10, 2018

@konkor I've switched to using async where possible. I believe it should work, as it works on at least 2 machines now.

@balsoft
Copy link
Author

balsoft commented Jun 10, 2018

@konkor I understand that we have to use sync calls when we need the output. Amazingly though, when we need to be root, we don't need output and can use async calls. It's just a coincidence, but it works.

@konkor
Copy link
Owner

konkor commented Jun 10, 2018

@balsoft OK. Let me some time to check it all today. Don't change nothing more. I'm working on your branch now.
I'll let you know a result soon as possible.
BTW So what about the policykit? Is it asking passwords continuously?

@balsoft
Copy link
Author

balsoft commented Jun 10, 2018

@konkor Yes, it's asking for password every time you change some setting and also for some reason it asks for password a bunch of times on startup. I'm currently thinking of some kind of solution for that. We could have some sort of DBUS service that would elevate once on extension startup and then don't bother us anymore. That wouldn't be easy, though, as it is a potential security hole.

@konkor
Copy link
Owner

konkor commented Jun 10, 2018

@balsoft i think it's possible to add MD5 checksum of cpufreqctl to the PK rule to avoid modifying the script.

@konkor
Copy link
Owner

konkor commented Jun 10, 2018

@balsoft Well, here is what I found:

  1. cpufreqctl require bash interpreter. It's not possible to use just /bin/sh at least on Debian/Ubuntu flavors. We could fix it on NixOS by creating link to the existing /bin/sh in the hope it has link to the bash and it installed or it's compatible with bash.
#I think It's possible to add this and checking inside the extension
pkexec ln -s /bin/sh /bin/bash
  1. I think you should revert the changes about this.installed It can be critical when updating is necessary to normal work of the extension. I can just fix check_install function.

  2. whereis doesn't work there bcs it has multiple paths on ordinary Linux distros. And it doesn't matter bcs '/etc/polkit-1/' is basic path for all systems. I'll use it for new versions of policykit with JS rules and /usr/share/polkit-1/ for old versions.

  3. I made https://github.com/konkor/cpufreq/tree/development branch for this.

  4. Check settings of your editor and set replacing TAB with 4 spaces.

What do you think?

@balsoft
Copy link
Author

balsoft commented Jun 10, 2018

@konkor Ok, thank you for spending the time on this.

  1. That would enrage most NixOS users (to be fair, me included 😆) because Nix tries to keep system as pure as possible, and adding random files to /bin is exactly the opposite. I've made a workaround that should make this work: https://github.com/balsoft/cpufreq/blob/9354391793f1de8c3fd8aa06dcce597668749a23/cpufreqctl#L14 Also, it would be better if you would just point out exact errors that /bin/sh gives in Ubuntu, as it appears to me that the script is sh-compatible (shellcheck cpufreqctl actually gives only some non-important warnings, except in 2 places which seem easy to fix).
  2. I can't see how my changes would stop extension from updating - both https://github.com/balsoft/cpufreq/blob/d5651b4852a075c673d139724ddcf1e844a25ca5/extension.js#L309 and https://github.com/balsoft/cpufreq/blob/d5651b4852a075c673d139724ddcf1e844a25ca5/extension.js#L659 are intact. Installation is not currently working because of directory issues (whereis thing that you've mentioned) - I will roll them back.
  3. See above, rolling back
  4. Great!
  5. I'm sorry for that, I was using nano for a quick second when extension was hanging gnome-shell and forgot that I was using tabs there, sorry.

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

Successfully merging this pull request may close these issues.

2 participants