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
Make GeoClue2 optional at runtime #656
base: master
Are you sure you want to change the base?
Conversation
If location_geoclue2_init fails, state->thread and state->lock could be uninitialized. Ensure that both are always set to avoid a crash.
Even if support for GeoClue2 was included at compile time, it could be missing at runtime. Be sure to check that the service is really installed. Tested on Arch Linux without GeoClue2 installed (which results in a fallback to manual mode) and with GeoClue2 installed (which results in automatically starting GeoClue2 and using it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion. I don't think this approach will work though, since if Redshift starts at login/boot the GeoClue DBus server may not be running so Redshift would fail to wait for it to become available (please correct me if I'm wrong but this is my understanding of how this would work). I understand that you'd like Redshift to fall back to the manual location if GeoClue for some reason is unresponsive. One solution that's been proposed is to immediate start out with a cached location value when Redshift starts up and then switch to a new location from GeoClue when it becomes available. I think this solution might work better and also solve the issue that you have. What do you think?
@jonls While the solution you mention is great for when GeoClue is used, here the case is about GeoClue not being available on the system. For instance, the distro package can be compiled with GeoClue support, but have it as an optional dependency only. If someone decides they don’t want GeoClue and don’t install it, currently it does not work and they have to recompile without. |
@ArchangeGabriel Maybe in this case the best option for the user is to just set the location provider explicitly in the configuration file to |
Sure. Let’s see if it suits @Lekensteyn. ;) |
If the daemon is not running, but the service files are available, it is supposed to be autostarted. That is what got tested in the above "with GeoClue2" scenario. I'll try to do another test with the GeoClue2 binaries deleted (but the service files installed) and see what happens. Edit: renaming
That sounds like #36 but as @ArchangeGabriel mentioned I am trying to solve a different problem. I am trying to make GeoClue2 runtime optional. I had some issues with lingering GeoClue2 agent processes on logout. While I could try to fix that, I realized that I have no need to the location service at all and for privacy reasons and to reduce the attack vector, removing GeoClue2 is desirable. The original downstream proposal to make it optional is here: https://bugs.archlinux.org/task/59657. This patch tries to address a concern from that issue, namely the default behavior without the GeoClue2 package. Previously the user would not get any error (not even a suggestion to try changing the location provider to "manual"). After this patch it will warn about missing GeoClue2 and fallback to manual. |
ping |
Ping, can this be merged or should something be changed? |
I'm assigned which means I'll take a look when I have time. No need to keep pinging 👍 |
Even if support for GeoClue2 was included at compile time, it could be
missing at runtime. Be sure to check that the service is really
installed.
Tested on Arch Linux without GeoClue2 installed (which results in a
fallback to manual mode) and with GeoClue2 installed (which results in
automatically starting GeoClue2 and using it).
Example output for unpatched redshift 1.12 on a system without GeoClue2:
Example output for this patched version without GeoClue2:
Example output for this patched version with GeoClue2: