hexive / sunpaper

Dynamic wallpaper changes based on the sun.
https://github.com/hexive/sunpaper
Apache License 2.0
197 stars 7 forks source link

Separate configuration from script #10

Closed nmiculinic closed 3 years ago

nmiculinic commented 3 years ago

I was writing PKGBUILD to install this on Arch-based systems:

# This is an example PKGBUILD file. Use this as a start to creating your own,
# and remove these comments. For more information, see 'man PKGBUILD'.
# NOTE: Please fill out the license field for your package! If it is unknown,
# then please put 'unknown'.

# The following guidelines are specific to BZR, GIT, HG and SVN packages.
# Other VCS sources are not natively supported by makepkg yet.

# Maintainer: Your Name <youremail@domain.com>

pkgname=sunpaper-git
pkgver=r87.b2d4048
pkgrel=1
pkgdesc=""
arch=("any")
url="https://github.com/hexive/sunpaper"
license=('GPL')
groups=()
depends=('sunwait' 'wallutils')
makedepends=('git') 
provides=("${pkgname%-git}")
conflicts=("${pkgname%-git}")
replaces=()
backup=()
options=()
install=
source=('git+https://github.com/hexive/sunpaper.git')
noextract=()
md5sums=('SKIP')

# Please refer to the 'USING VCS SOURCES' section of the PKGBUILD man page for
# a description of each element in the source array.

pkgver() {
    cd "$srcdir/${pkgname%-git}"

# The examples below are not absolute and need to be adapted to each repo. The
# primary goal is to generate version numbers that will increase according to
# pacman's version comparisons with later commits to the repo. The format
# VERSION='VER_NUM.rREV_NUM.HASH', or a relevant subset in case VER_NUM or HASH
# are not available, is recommended.

# Git, tags available
    # printf "%s" "$(git describe --long | sed 's/\([^-]*-\)g/r\1/;s/-/./g')"

# Git, no tags available
    printf "r%s.%s" "$(git rev-list --count HEAD)" "$(git rev-parse --short HEAD)"
}

package() {
    cd "$srcdir/${pkgname%-git}"
    install -D -m755 sunpaper.sh "$pkgdir/usr/bin/sunpaper"
    mkdir -p "$pkgdir/usr/share/sunpaper"
    cp -dr images "$pkgdir/usr/share/sunpaper/images"
    find "$pkgdir/usr/share/sunpaper/images" -type d -exec chmod +x {} \;
}

And got it installed...only to find out the configuration is intertwined with the execution itself. Please, have the script accept parameters either via JSON config file or via environment variables ( and running in bash "strict" mode, http://redsymbol.net/articles/unofficial-bash-strict-mode/ )

That way it would be easier to package into the package manager, run & maintain it.

hexive commented 3 years ago

The script optionally accepts an external config file at ~/.config/sunpaper/config ... is that sufficient for your needs?

I'm pretty new to bash but will look into and test the script in "strict" mode and convert it if there aren't any major issues.

nmiculinic commented 3 years ago

Actually, probably yes if I can use the main shell file without changes. Where can I find documentation for this config?

hexive commented 3 years ago

There is no documentation, as I didn't want to support an external config by default: https://github.com/hexive/sunpaper/issues/3

But, optionally, if a file exists at ~/.config/sunpaper/config, script will read config values from there.

nmiculinic commented 3 years ago

There is no documentation, as I didn't want to support an external config by default:

What made you chose not to support external config by default? Not sure how experienced are you with software engineering, though it's a good practice separating binaries (in this case script) from the config. Though I understand this is your own project, which is amazing by the way!, and I'm glad you shared it freely online :).

nmiculinic commented 3 years ago

My idea was to configure this as systemd timer, and well...systemd is realistically everywhere and I can probably use user's systemd session. This was my deployment idea, and it's easily shareable and reusable across projects & desktop environments

hexive commented 3 years ago

I have no software engineering experience at all! I'm excited to have sunpaper in AUR so please let me know what I can do to support your build.

hexive commented 3 years ago

I added an external configuration file with default settings to /extra/config. If you move that file to ~/.config/sunpaper/config on install everything should work as expected.

If new configuration options are added on future updates, will they need to be handled by me in a special way in order to update existing users external configs?

I also started looking at the script using the bash "strict" method you suggested. Except for a couple unbound variables I think that change will be straight forward. I want to do some more testing to make sure all the optional features are still working as they should.

Thank you again for your time putting this all together.

nmiculinic commented 3 years ago

Anyway, I'm having further issues. sunpaper.sh is rather..unverbose to me.

I start it (without any config files in my system) and it just silently exits without an error code or anything. Could you increase the logging verbosity?

nmiculinic commented 3 years ago

In #12 I got installation working, more or less (gotta hash out a few things), using systemd's unit & services on the user level...that should work.

nmiculinic commented 3 years ago

IMO this product should require explicit config file, and fail without one (with appropriate error).

hexive commented 3 years ago

I'm using this project as an excuse to learn bash and git so I'll pretty much follow your suggestions as it's pretty clear you have more experience than I do.

hexive commented 3 years ago

Here is an issue #11 where the setwallpaper utility is not setting wallpaper properly from within systemd. It may possibly related to environmental variables not available? Were you able to get changing paper with a systemd timer on your personal setup?

Barbaross93 commented 3 years ago

I can make this a separate issue if need be, but what you may want to consider is to do something like adding a --daemon|-d flag where it sets daemonMode=True. Then, instead of the block of code at the end of your script, wrap it in a function called main and place this at the end of the script"

if [ "$daemonMode" == "True" ]; then
    while :; do
        main &
        sleep 60
    done &
else
    main
fi

You'd then probably want to add a stop flag that would kill any running sunpaper.sh instances, but having this functionality internally would probably be the better option than having to rely on cron, systemd, or w/e

hexive commented 3 years ago

Is it really that simple?! I literally have a todo list item over here that says "learn how to make a bash daemon" haha. I'll get this in on the next update.

Barbaross93 commented 3 years ago

It might be a little trickier than that. The only thing I can see being problematic with that block is that executing main &, although forked to the background, does take a few microseconds to run before starting the next sleep timer. After a looooooong time things might start to get skewed. It would probably take months if not years before it becomes a problem though