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

Jump to bottom

nixos/geoclue2: restructure module made by worldofpeace #60681

Merged
merged 3 commits into from May 16, 2019
Merged

nixos/geoclue2: restructure module made by worldofpeace #60681

merged 3 commits into from May 16, 2019

Conversation

leotaku
Copy link
Contributor

@leotaku leotaku commented May 1, 2019

Motivation for this change

Changes to pull request #45306, motivations discussed there.

Restructured the module to use the generators.toINI function.
I hope this fits what @michaelpj had in mind, please give me some feedback.

Also, if I understand everything correctly, the still-active review by @infinisil has already been addressed in the initial pr.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@leotaku
Copy link
Contributor Author

leotaku commented May 1, 2019

Also, I would appreciate if somebody could help me with finding a fix for the following problem:
When the service configuration is changed, the /etc/geoclue/geoclue.conf file gets changed, but the geoclue service does not restart. (because nixos does not know that this service depends on the file)

@worldofpeace
Copy link
Contributor

Also, I would appreciate if somebody could help me with finding a fix for the following problem:
When the service configuration is changed, the /etc/geoclue/geoclue.conf file gets changed, but the geoclue service does not restart. (because nixos does not know that this service depends on the file)

maybe

systemd.services.geoclue.restartTriggers = [ "/etc/geoclue/geoclue.conf" ];

Though I don't know how geoclue handles changes to the config file, and just maybe reloads as expected?

@leotaku
Copy link
Contributor Author

leotaku commented May 2, 2019

Though I don't know how geoclue handles changes to the config file, and just maybe reloads as expected?

It does not. I have implemented your suggestion into the module.

michaelpj
michaelpj approved these changes May 14, 2019
View reviewed changes
Copy link
Contributor

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

worldofpeace
worldofpeace suggested changes May 15, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're missing configuration that's upstream default, see what was removed in this commit 807f3f3.

@leotaku
Copy link
Contributor Author

leotaku commented May 15, 2019

@worldofpeace I hope that was what you meant? I hope so, otherwise @ me and I will revert and change what is needed.

@worldofpeace
Copy link
Contributor

That's what I meant @leotaku, but we also need

services.geoclue2.appConfig."io.elementary.desktop.agent-geoclue2" = {
  isAllowed = true;
  isSystem = true;
};

in the pantheon.nix module.

worldofpeace
worldofpeace suggested changes May 15, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above comment

leotaku and others added 3 commits May 16, 2019 17:45
@leotaku @worldofpeace
nixos/geoclue2: make configurable, can whitelist applications
09bb5f0 
All options within geoclue.conf[0] have been made configurable.

Additonally, we can now specify whether or not GeoClue
should ask the agent to authorize an application like so:
```
services.geoclue2.appConfig."redshift" = {
  isAllowed = true;
  isSystem = true;
};
```

[0]: https://gitlab.freedesktop.org/geoclue/geoclue/blob/2.5.2/data/geoclue.conf.in

Co-authored-by: worldofpeace <worldofpeace@protonmail.ch>
@leotaku @worldofpeace
nixos/gnome3: add geoclue application configuration
26a4d6a 
Co-authored-by: worldofpeace <worldofpeace@protonmail.ch>
@leotaku @worldofpeace
nixos/pantheon: add geoclue application configuration
a6830f9 
Co-authored-by: worldofpeace <worldofpeace@protonmail.ch>
@worldofpeace
Copy link
Contributor

Fixed up the commit msg's there and added myself as a coauthor.

Going to test this with GNOME3 and Pantheon and then I think we should be good to merge 👍

@leotaku
Copy link
Contributor Author

leotaku commented May 16, 2019

Great!

Off-topic but is there some explanation on how working on other contributors pull requests/co-authoring works? I don't really know how to use these features which was why I created a separate pull request in the first place.

@worldofpeace
Copy link
Contributor

Off-topic but is there some explanation on how working on other contributors pull requests/co-authoring works? I don't really know how to use these features which was why I created a separate pull request in the first place.

For making a multi author commit this should probably explain that https://help.github.com/en/articles/creating-a-commit-with-multiple-authors.

Usually though what you can do is open a pr against my fork trying to merge your fixes into my branch.
Or I maybe could have added you as a temporary collaborator to my fork. Though that depends if you know the person still cares about x change.

Either way I'm happy to see this continued 👍

worldofpeace
worldofpeace approved these changes May 16, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested that GeoClue functionality in gnome-weather, gnome-maps, and gnome-calendar are unaffected by this change.

@worldofpeace worldofpeace merged commit efcdac6 into NixOS:master May 16, 2019
This was referenced May 16, 2019
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelpj michaelpj michaelpj approved these changes

@worldofpeace worldofpeace worldofpeace approved these changes

@infinisil infinisil Awaiting requested review from infinisil

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@leotaku @worldofpeace @michaelpj