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 Makefile and new installation methods supporting multi-user environments #107

Merged
merged 34 commits into from
Apr 5, 2020

Conversation

fin-ger
Copy link
Member

@fin-ger fin-ger commented Mar 30, 2020

This PR is now awaiting feedback and reviews.

TODO

  • Restrict a single-user installation to the user who installed it
  • Do not attempt new installation when the version string in the polkit file changed
  • Reload extension when user issued uninstall via settings window
  • Add github action to create a release zip for github releases when a version tag is pushed
    - this should make it easier for package maintainers to grab a specific version of this extension
  • Add new translation template

@martin31821 @mastercaution Can you test the new installation and review for the security issues?

Changes include:

  • A Makefile with the following targets
    • build for compiling the glib-schema (translation missing for now)
    • package produces a semver-named, extensions.gnome.org compatible, zip file for distribution in the target folder
    • clean cleans the target folder
    • install installs the extension itself to $PREFIX
    • uninstall uninstalls the extension from $PREFIX
    • install-tool installs the polkit rule and the cpufreqctl tool, where the cpufreqctl tool is installed to $PREFIX/local/bin when a TOOL_SUFFIX is provided, and to $PREFIX/bin when no TOOL_SUFFIX is provided
    • uninstall-tool uninstalls the polkit rule and cpufreqctl tool from $PREFIX/bin or $PREFIX/local/bin
    • release increments the major, minor, or patch version, updates the version in several files, and creates a git version tag which is then pushed
  • An uninstall button is added to the settings window
  • polkit rule changes
    • the id is different for each user
    • version annotation is added to rule, to make it easier to identify "old" rules which need updating in the future
  • tool folder
    • Moved cpufreqctl and installer.sh to separate folder to make it easier for package maintainers to spot the files which need executable rights
  • scripts folder contains the release.sh script for the make release target
  • extension code
    • when installer.sh check returns exit code 5, the extension tools need updating
    • src/config.js contains all path related configuration for this extension, so that distro packagers can easily patch paths to the distro's needs
    • src/update.js contains a new menu to ask the user to update the extension tools

@fin-ger
Copy link
Member Author

fin-ger commented Mar 31, 2020

Regarding the first item on my TODO list:
As far as I understand this, polkit cannot restrict access to a specific user. Instead, it manages when an administrator needs to authorize a user to do an administrative task. For restricting access to the cpufreqctl tool for other users, the per-user executables in e.g. /usr/local/bin/cpufreqctl-fin must be +x and +r only for the user fin. This would make the polkit rule useless for any other "normal" user on the system, right?

Edit: Of course the owner must not be fin but be root as this would cause the same issue as noted by @serverwentdown in #102. I'm not sure how to handle this correctly. Essentially we have to achieve that users other than fin have no +x rights on this file while still owned by root. Don't know if that's possible... Please correct me if I'm wrong

@fin-ger
Copy link
Member Author

fin-ger commented Mar 31, 2020

Regarding the 2nd item on my TODO list:
Do we want to "bother" our users with updating/reinstalling the tools with every release we do? Do we want to handle semver here? Do we want to ignore the version string completely for our compatibility check and insert custom tool-update rules like currently for v7 and v8 polkit rules? (tools/installer.sh#L112)

Doing complex checks here might make it hard for distro packagers to keep track of our changes...

Edit: I added a polkit-rule version which can be incremented separately from the cpupower version. This ensures the polkit rule gets updated when needed. The polkit rule version should also be incremented when the cpufreqctl tool receives an update.

@fin-ger
Copy link
Member Author

fin-ger commented Mar 31, 2020

Regarding the 3rd item on my TODO list:
Has anyone successfully reloaded the indicator from the preferences window? I'm struggling with this for a day now...

Edit: solved it in e7468f7

@fin-ger
Copy link
Member Author

fin-ger commented Mar 31, 2020

Regarding the 5th item on my TODO list:
@martin31821 What is the current workflow for adding new translation strings? Anything I could add to the makefile here?

Edit: solved

…ling, and add proper layout for buttons in uninstall dialog
src/preferences.js Outdated Show resolved Hide resolved
@fin-ger
Copy link
Member Author

fin-ger commented Apr 2, 2020

@georgekaz I think I fixed all errors in Ubuntu 18.04 (and 19.10, 20.04)

Can you try again?

@fin-ger fin-ger mentioned this pull request Apr 2, 2020
@georgekaz
Copy link

@fin-ger Sorry for delayed reply. Just tested the "all users" installation on 18.04 and it's working fine now, nice one! I don't have any other versions of ubuntu to hand but I do have manjaro with GNOME Shell 3.36.0 if you need a test there.
Thanks

Makefile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/update.js Outdated Show resolved Hide resolved
@mastercaution
Copy link
Contributor

mastercaution commented Apr 5, 2020

Sorry 😅 but I have this issue on Ubuntu 20.04 since 5fd58c9:

$ pkg-config --variable=glib_compile_schemas gio-2.0
Package gio-2.0 was not found in the pkg-config search path.
Perhaps you should add the directory containing `gio-2.0.pc'
to the PKG_CONFIG_PATH environment variable
No package 'gio-2.0' found

Edit: also 19.10

@fin-ger
Copy link
Member Author

fin-ger commented Apr 5, 2020

Ah, damn. You need the dev packages for glib. For ubuntu it's libglib2.0-dev. I will look up the dev packages for the major distros and add this to the readme. Alternatively, we could skip the pkg-config lookup and let package managers pass the path to the glib-compile-schemas library manually... It's quite an overhead to install the libglib-dev packages. What do you think?

@mastercaution
Copy link
Contributor

Alternatively, we could skip the pkg-config lookup and let package managers pass the path to the glib-compile-schemas library manually

What do you mean with that?

@fin-ger
Copy link
Member Author

fin-ger commented Apr 5, 2020

Alternatively, we could skip the pkg-config lookup and let package managers pass the path to the glib-compile-schemas library manually

What do you mean with that?

Do not read the glib-compile-schemas binary location from the pkg-config, but just set it like this:

GLIB_COMPILE_SCHEMAS = /usr/bin/glib-compile-schemas

This avoids a dependency on pkg-config and glib-dev libs, but makes it more difficult for package maintainers to hook into schemas compilation.

@fin-ger
Copy link
Member Author

fin-ger commented Apr 5, 2020

Sorry sweat_smile but I have this issue on Ubuntu 20.04 since 5fd58c9:

$ pkg-config --variable=glib_compile_schemas gio-2.0
Package gio-2.0 was not found in the pkg-config search path.
Perhaps you should add the directory containing `gio-2.0.pc'
to the PKG_CONFIG_PATH environment variable
No package 'gio-2.0' found

Edit: also 19.10

The file missing is /usr/lib/x86_64-linux-gnu/pkgconfig/gio-2.0.pc (ubuntu), or /usr/lib/pkgconfig/gio-2.0.pc on my machine.

@mastercaution
Copy link
Contributor

Hmm ... I have no experience as package maintainer, but I would go with the alternative to avoid the overhead 😅

@fin-ger
Copy link
Member Author

fin-ger commented Apr 5, 2020

I agree, if this causes trouble packaging this extension for a distro, we can change it.

@mastercaution
Copy link
Contributor

Exactly

@mastercaution
Copy link
Contributor

Fantastic job @fin-ger, I think it's ready to merge

@mastercaution
Copy link
Contributor

mastercaution commented Apr 5, 2020

I tried to give you a shiny green check mark too 😅
Maybe only collaborators have this privilege.

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.

4 participants