Refactor attempt while providing upstream downstream #262

Kreyren · 2020-07-05T03:16:01Z

DO_NOT_MERGE: Work in progress

Currently it is painful to build this project due to the lack of code quality assurance in bootstrap.sh and seemingly hard dependency on systemd, this is an attempt to resolve it while providing upstream downstream for paludis made for Exherbo Linux and Exheredrey.

Kreyren · 2020-07-05T03:17:11Z

@FeralInteractive Please provide upstream-way of building (ideally with explanation) for this binary so that i can make a proper logic, thank you ^-^

EDIT: I know how to build it, but having more informations for in-code docs and logic for known issues would be appreciated

mdiluz · 2020-07-05T09:22:29Z

I'm not sure I understand the goals here or what's missing in the current repo - could you explain a little more?

Kreyren · 2020-07-05T09:49:08Z

@mdiluz I want to change following

bootstrap.sh, because it's not passing shellcheck and is seemingly designed to work only on specified systems with lack of sanitization
Add CI to capture these code quality issues
openrc support so that i can use this on my system (which doesn't seem to be supported atm, but seems to be easily implemented?)

I would also like to add exherbo and devuan downstream to upstream if you want it otherwise i will keep it in my fork, because keeping this on upstream repos seems to make it easier to work with. ^-^

Assuming it being welcomed?

stephanlachnit · 2020-07-05T21:37:17Z

dist/README.md

@@ -0,0 +1,3 @@
+# Downstream management
+
+Directory dedicated for various package managers to recognize this repository into a package


WTF no please don't do something like this upstream. Packaging files should be downstream, always. It only bloats the repo and you can't even directly change something in there since you don't have write rights.

stephanlachnit · 2020-07-05T21:37:46Z

Makefile

+all:
+	# FIXME: Add nice message
+	@ exit 2
+
+build:
+	@ bootstrap.sh build
+
+exherbo-docker-test:
+	@ sudo docker run \
+		--interactive \
+		--tty \
+		--volume "$$(pwd)/dist/paludis/exherbo:/var/repositories/gamemode" \
+		exherbo/exherbo-x86_64-pc-linux-gnu-base \
+			bash -c "true \
+				&& chown root:tty /dev/tty \
+				&& usermod -a -G tty paludisbuild \
+				&& printf '%s\n' \
+					\"format = e\" \
+					\"location = \"\$$\{root\}/var/db/paludis/repositories/gamemode\"\" \
+					\"sync = file:///var/repositories/gamemode\" \
+				> /etc/paludis/repositories/gamemode.conf \
+				&& cat /etc/paludis/repositories/gamemode.conf \
+				&& cave sync \
+				&& cave resolve games-utils/gamemode \
+			"


Adding a second build system (Make) just for a test that could be run in a script is a bad idea, it ruins automatic build systems like debhelper on Debian.

stephanlachnit · 2020-07-05T21:39:14Z

bootstrap.sh

-# Simple bootstrap script to build and run the daemon
+#!/bin/sh
+# Originally created by FeralInteractive <https://www.feralinteractive.com/en/> under BSD 3-Clause License <https://github.com/FeralInteractive/gamemode/blob/master/LICENSE.txt> in 2017 bootstrap.sh
+# Performed full refact by Jacob Hrbek <[email protected]> under GPLv3 license <https://www.gnu.org/licenses/gpl-3.0.en.html> in 05.07.2020 04:28:55 CET


Ignoring the need for this, please don't put license clutter in here. The file was already licensed under a BSD license, so no need to change that to a GPL license.

stephanlachnit · 2020-07-05T21:40:20Z

bootstrap.sh

+
+# Maintainer info
+UPSTREAM="https://github.com/FeralInteractive"
+MAINTAINER="https://github.com/Kreyren"


It's nice that you want to maintain this file, but I think that's something you should ask beforehand since you don't have write access to this repo.

stephanlachnit · 2020-07-05T21:42:44Z

bootstrap.sh

+if command -v "$UNAME" 1>/dev/null; then
+	unameKernel="$("$UNAME" -s)"
+	edebug "Identified the kernel as '$unameKernel"
+	case "$unameKernel" in
+		Linux)
+			KERNEL="$unameKernel"

-# Restart polkit so we don't get pop-ups whenever we pkexec
-if systemctl list-unit-files |grep -q polkit.service; then
-    sudo systemctl try-restart polkit
+			# Assume Linux Distro and release
+			# NOTICE(Krey): We are expecting this to return a lowercase value
+			if command -v "$LSB_RELEASE" 1>/dev/null; then


I don't think it is necessary to identify either the kernel or the distro

stephanlachnit · 2020-07-05T21:47:48Z

bootstrap.sh

+# Used to prefix logs with timestemps, uses ISO 8601 by default
+logPrefix="[ $(date -u +"%Y-%m-%dT%H:%M:%SZ") ] "
+# Path to which we will save logs
+# NOTICE(Krey): To avoid storing file '$HOME/.some-name.sh.log' we are stripping the '.sh' here
+logPath="${XDG_DATA_HOME:-$HOME/.local/share}/${myName%%.sh}.log"
+
+# inicialize the script in logs
+"$PRINTF" '%s\n' "Started $myName on $("$UNAME" -s) at $(date -u +"%Y-%m-%dT%H:%M:%SZ")" >> "$logPath"


Honestly this is so small there is no need to log this. Users can simply copy the output (this script isn't intended for packaging).

stephanlachnit · 2020-07-05T21:49:19Z

.github/workflows/shell.yml

+name: Shell
+
+# Relevant to events - https://help.github.com/en/actions/automating-your-workflow-with-github-actions/events-that-trigger-workflows
+on:
+  pull_request:
+    types: [synchronize, opened, reopened, ready_for_review]
+    paths:
+    - '**.sh'
+
+jobs:
+  # Linting
+  lint:
+    runs-on: ubuntu-latest
+    container: debian:stable
+    steps:
+      - name: Installing dependencies..
+        run: |
+          # Sync repos
+          # Check for git
+          if ! apt list --installed 2>/dev/null | grep -qP "^git\/stable.*"; then
+            # Check if we can install git
+            if ! apt list | grep -qP "^git\/stable.*"; then
+              apt update
+            elif apt list | grep -qP "^git\/stable.*"; then
+              true
+            else
+              exit 255
+            fi
+            # Install git
+            apt install -y git
+          elif apt list --installed 2>/dev/null | grep -qP "^git\/stable.*"; then
+            true
+          else
+            exit 255
+          fi
+          # Check for shellcheck
+          if ! apt list --installed 2>/dev/null | grep -qP "^shellcheck\s{1}-.*"; then
+            # Check if we can install shellcheck
+            if ! apt list | grep -qP "^shellcheck\s{1}-.*"; then
+              apt update
+            elif apt list | grep -qP "^shellcheck\s{1}-.*"; then
+              true
+            else
+              exit 255
+            fi
+            # Install shellcheck
+            apt install -y shellcheck
+          elif apt list --installed 2>/dev/null | grep -qP "^shellcheck\s{1}-.*"; then
+            true
+          else
+            exit 255
+          fi
+      - name: Pulling git dir..
+        uses: actions/checkout@v2
+      - name: Processing files..
+        # Make sure that bash is used
+        shell: bash
+        run: |
+          cd "$GITHUB_WORKSPACE"
+
+          # Process files
+          ## NOTICE: Do not use for loop to avoid pitfall https://mywiki.wooledge.org/BashPitfalls#pf1
+          git --git-dir="$GITHUB_WORKSPACE/.git" ls-files -z -- '*.sh' | while IFS= read -rd '' file; do
+              printf 'linting shell file %s\n' "$file"
+              shellcheck --external-sources --shell=sh "$file"
+          done


Besides a spellcheck being pretty much useless wasted computation time, this can be easily integrated into travis in like two lines.

stephanlachnit · 2020-07-05T21:52:48Z

.github/workflows/exherbo-build.yml

+name: Exherbo
+
+# Relevant to events - https://help.github.com/en/actions/automating-your-workflow-with-github-actions/events-that-trigger-workflows
+on:
+  pull_request:
+    types: [synchronize, opened, reopened, ready_for_review]
+    paths:
+    - 'dist/paludis/exherbo'
+
+jobs:
+  # Linting
+  lint:
+    runs-on: ubuntu-latest
+    container: exherbo:exherbo-x86_64-pc-linux-gnu-base
+    steps:
+      - name: FIXME
+        # Make sure that bash is used
+        shell: bash
+        run: |
+          printf '%s\n' "FIXME"


I don't want to downplay Exherbo - but before a CI for Exherbo is added, one might add Fedora or Arch first since they have a much bigger user and developer base, and thus are more representative if the CI would actually fail.

stephanlachnit · 2020-07-05T21:53:10Z

.github/workflows/bootstrap-test.yaml

+name: bootstrap-test
+
+# Relevant to events - https://help.github.com/en/actions/automating-your-workflow-with-github-actions/events-that-trigger-workflows
+on:
+  pull_request:
+    types: [synchronize, opened, reopened, ready_for_review]
+    paths:
+    - 'bootstrap.sh'
+
+jobs:
+  # Linting
+  lint:
+    runs-on: ubuntu-latest
+    container: debian:stable
+    steps:
+      - name: FIXME
+        # Make sure that bash is used
+        shell: bash
+        run: |
+          printf '%s\n' "FIXME"


Already included into travis, no need for this.

stephanlachnit · 2020-07-05T21:53:56Z

.github/workflows/bash.yml

+          else
+            exit 255
+          fi
+          # Check for shellcheck


No need to run the spellcheck twice.

stephanlachnit · 2020-07-05T22:13:07Z

Currently it is painful to build this project due to the lack of code quality assurance in bootstrap.sh and seemingly hard dependency on systemd

First of all, I don't think that there is a lack of "QA" for this tiny bootstrap script. And gamemode it doesn't have a hard dependency on systemd either. It does require libsystemd, aka sd-bus, which btw doesn't require to have systemd as an init system.
Anyway, you probably want to use elogind for this, and there is a Meson build option for this.

openrc support so that i can use this on my system (which doesn't seem to be supported atm, but seems to be easily implemented?)

So in this case, you might not want to run bootstrap.sh but rather write a custom script just for that purpose. I guess it doesn't make much sense to write a build script covering every possible option, so it only works for the majority of people. Distributions like Debian for example don't even use that script since they simply build with Meson, and set up the rest themselves.
Basically you want to do

meson builddir -Dwith-sd-bus-provider=elogind --prefix=/usr
meson compile -C builddir
sudo meson install -C builddir

And restart polkit somehow. That's it, no need for 400+ loc. Put that in your build files and distribute the package.

mdiluz · 2020-07-07T11:28:03Z

I largely agree with @stephanlachnit here, there's no reason to put downstream stuff into the upstream repo. I also want to comment a little:

bootstrap.sh, because it's not passing shellcheck and is seemingly designed to work only on specified systems with lack of sanitization

This is a reason to delete it, not add 500 lines of code.

And maybe this is simply a reminder to move the travis stuff over to github actions now that those are allowed for free.

kakra · 2020-09-24T22:32:13Z

I guess it should simply be pointed out that bootstrap.sh is not to be used for packaging. I usually avoid such scripts even for a simple one-shot installation directly from source, because such scripts often make wrong assumptions about my system, i.e. they use sudo to clutter and clobber my system, overwriting files in /usr/lib or simply putting files not tracked by the package manager all over the system folders.

In this instance, it will install into /usr prefix by default. That should be /opt/gamemode or /usr/local probably. I'd prefer one-shot installations to live in their own namespace below /opt so I could uninstall by just deleting it. An unmanaged installer should never install to /usr directly.

As such, the script should probably be removed, or the defaults should change. In short term, the install instruction should put a clear message that bootstrap.sh is not to be used by packagers, maybe by introducing a new "Packaging" section in the README file showing how to use meson/ninja and how to manage the systemd/elogind dependencies (these are not properly documented yet, this is the opportunity).

stephanlachnit · 2020-09-26T13:04:20Z

I guess it should simply be pointed out that bootstrap.sh is not to be used for packaging. I usually avoid such scripts even for a simple one-shot installation directly from source, because such scripts often make wrong assumptions about my system, i.e. they use sudo to clutter and clobber my system, overwriting files in /usr/lib or simply putting files not tracked by the package manager all over the system folders.

In this instance, it will install into /usr prefix by default. That should be /opt/gamemode or /usr/local probably. I'd prefer one-shot installations to live in their own namespace below /opt so I could uninstall by just deleting it. An unmanaged installer should never install to /usr directly.

As such, the script should probably be removed, or the defaults should change. In short term, the install instruction should put a clear message that bootstrap.sh is not to be used by packagers, maybe by introducing a new "Packaging" section in the README file showing how to use meson/ninja and how to manage the systemd/elogind dependencies (these are not properly documented yet, this is the opportunity).

Yeah I guess it's a good idea to make it more clear that the scripts is only intended if you install from source. Switching to /usr/local is probably a good idea, but it will break gamemoded --test because that path is hardcoded, something that should be fixed for that.

Stub

138a1e0

Kreyren mentioned this pull request Jul 5, 2020

pkg: game-util/gamemode Exheredrey/Exheredrey#8

Closed

stephanlachnit reviewed Jul 5, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor attempt while providing upstream downstream #262

Refactor attempt while providing upstream downstream #262

Kreyren commented Jul 5, 2020 •

edited

Loading

Kreyren commented Jul 5, 2020 •

edited

Loading

mdiluz commented Jul 5, 2020

Kreyren commented Jul 5, 2020

stephanlachnit Jul 5, 2020 •

edited

Loading

stephanlachnit Jul 5, 2020

stephanlachnit Jul 5, 2020

stephanlachnit Jul 5, 2020

stephanlachnit Jul 5, 2020

stephanlachnit Jul 5, 2020 •

edited

Loading

stephanlachnit Jul 5, 2020

stephanlachnit Jul 5, 2020

stephanlachnit Jul 5, 2020

stephanlachnit Jul 5, 2020

stephanlachnit commented Jul 5, 2020 •

edited

Loading

mdiluz commented Jul 7, 2020

kakra commented Sep 24, 2020

stephanlachnit commented Sep 26, 2020

		@@ -0,0 +1,3 @@
		# Downstream management

		Directory dedicated for various package managers to recognize this repository into a package

Refactor attempt while providing upstream downstream #262

Are you sure you want to change the base?

Refactor attempt while providing upstream downstream #262

Conversation

Kreyren commented Jul 5, 2020 • edited Loading

Kreyren commented Jul 5, 2020 • edited Loading

mdiluz commented Jul 5, 2020

Kreyren commented Jul 5, 2020

stephanlachnit Jul 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanlachnit Jul 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanlachnit commented Jul 5, 2020 • edited Loading

mdiluz commented Jul 7, 2020

kakra commented Sep 24, 2020

stephanlachnit commented Sep 26, 2020

Kreyren commented Jul 5, 2020 •

edited

Loading

Kreyren commented Jul 5, 2020 •

edited

Loading

stephanlachnit Jul 5, 2020 •

edited

Loading

stephanlachnit Jul 5, 2020 •

edited

Loading

stephanlachnit commented Jul 5, 2020 •

edited

Loading