Hummer12007 / brightnessctl

A program to read and control device brightness
Other
900 stars 48 forks source link

[RFC] configure script + Logind providers #69

Closed xdbob closed 2 years ago

xdbob commented 3 years ago

As I suggested in #67 I converted the build-system to the traditional ./configure && make && sudo make install (without the whole autotools nonsense).

What do you think ?

And btw, neither basu nor elogind are packaged on my distribution someone need to test if they work (and are configured) as expected.

Closes #67

PS: @abcdw I credited you in dbus: support elogind and basu at build time if you want a more explicit reference I can credit you in the code or not at all if you wish

xdbob commented 3 years ago

I would also like to change 2 of the defaults:

Are you OK with that @Hummer12007 ?

Hummer12007 commented 3 years ago

Thanks! Mind pinging me if I don't get to the PR in the next few days?

abcdw commented 3 years ago

It will require some changes to package recipe in downstream package managers and distros, but overall looks good to me and such approach seems a little cleaner.

xdbob commented 3 years ago

Thanks! Mind pinging me if I don't get to the PR in the next few days?

@Hummer12007 ping ? :)

btw, you did not address my other question:

I would also like to change 2 of the defaults:

* enable logind -> most of the distributions should have systemd v243 by now

* prefix=/usr/local -> let's keep `/usr` for distribution packages

Are you OK with that @Hummer12007 ?

Hummer12007 commented 2 years ago

@xdbob looks good?

xdbob commented 2 years ago

@xdbob looks good?

I would reverse the bus provider checks and break when the first one is found, eg:

for prov in libsystemd libelogind basu; do
    if pkg-config --exists "$prov" >/dev/null 2>&1; then
        bus_provider="$prov"
        sdbus="${prov#lib}"
        break
    fi
done

Otherwise, LGTM :+1:

Hummer12007 commented 2 years ago

Nice catch! The order was intended. With regards to enabling logind by default, I'd rather not (it is much slower than the ordinary sysfs setter, which won't play nice with the gradual brightness transition). It is up to the distros to decide anyway. UPD: Oh, it is used as a fallback anyway. With regards to the default prefix, /usr/local is the better option, as the packagers should explicitely specify the prefix anyway, while end users are better off with /usr/local

Hummer12007 commented 2 years ago

Oh, you suggested /usr/local as the new default.

Hummer12007 commented 2 years ago

Thanks!