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

nixos/onedrive: init #77734

Open
wants to merge 1 commit into
base: master
from
Open

nixos/onedrive: init #77734

wants to merge 1 commit into from

Conversation

@SRGOM
Copy link
Member

SRGOM commented Jan 15, 2020

Documentation

#77734 (comment)

Ideal Todo but I fail:

  • Use full path instead of just a dir name.
    Currently this module allows using only a directory name inside ~/.config. Should actually allow full directory paths. I believe that using paths as instance name is not allowed by systemd.

  • Use $XDG_CONFIG_HOME instead of .config. The author of the main package- onedrive, uses .config and not $XDG_CONFIG_HOME in https://github.com/abraunegg/onedrive/blob/master/docs/USAGE.md. I would prefer using XDG but would go with .config because that's what the author seemingly uses, not sure what he's doing in code

Todo that I wouldn't be able to spend time on...

  • Automated testing. I've never written a test and don't have the time or motivation unfortunately.

Manual testing steps:

  1. Check out.
  2. Inside imports section, add a line with absolute path to the file onedrive.nix
  3. Add services.onedrive.enable = true; services.onedrive.package= <SOME RECENT CHANNEL>.onedrive
@doronbehar

This comment has been minimized.

Copy link
Contributor

doronbehar commented Jan 15, 2020

Hey @SRGOM.

First of all, I'm not using onedrive, I just thought it's cool and that we should package it and I don't mind maintaining it much. But, upstream provides a systemd service. Do we package it? If we do, I don't think we should add our own version of it. If we don't package it, let's start by adding it to our package and perhaps make sure everything here:

https://github.com/abraunegg/onedrive/tree/master/contrib

Will get packaged as well. BTW it needs a small version update (see) it could be nice to get on the way..

@SRGOM

This comment has been minimized.

Copy link
Member Author

SRGOM commented Jan 16, 2020

@doronbehar thanks for responding. I have to confess :

  1. I don't know much about systemd in general.

  2. I don't know how to package existing services in nixos (I will find out)

That said, this code allows multiple oned instances for the same user. So at least one of the two services is different.

Also, sorry I don't know what you mean by "adding it to our package". Don't modules live in diff hierarchy? (First time module writer )

I may not be able to spend effort to package other things, sorry. I believe a service is critical hence adding.

@nixos-discourse

This comment has been minimized.

Copy link

nixos-discourse commented Jan 16, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/107

@SRGOM

This comment has been minimized.

Copy link
Member Author

SRGOM commented Jan 16, 2020

@doronbehar I did a find for .service files in the nixpkgs repository, and there's no such file. So I guess you meant just copy paste the service from upstream into a nix style service. Got it.

@SRGOM SRGOM force-pushed the SRGOM:onedrivelll branch from 789cea2 to ae1ef46 Jan 16, 2020
@SRGOM

This comment has been minimized.

Copy link
Member Author

SRGOM commented Jan 16, 2020

@doronbehar I have addressed your comment and incorporated the service from onedrive upstream. Any other comments you'd like to add?

@SRGOM

This comment has been minimized.

Copy link
Member Author

SRGOM commented Jan 16, 2020

@jonringer Since you too reviewed (and approved) #73360, may I further abuse your hospitality by requesting you to review this too? I still have to add documentation but the service part is done (as best as I can- please see the Ideal Todo part in my original post)

@jonringer

This comment has been minimized.

Copy link
Contributor

jonringer commented Jan 16, 2020

you may, however, I'm still largely ignorant of nixos module best practices

cc @Infinisil

@jonringer jonringer requested a review from Infinisil Jan 16, 2020
@aanderse

This comment has been minimized.

Copy link
Contributor

aanderse commented Jan 16, 2020

@SRGOM I'm pretty sure @doronbehar meant you should use systemd.packages instead of writing (or copy+pasting) systemd unit files. Do a quick grep through nixpkgs and you'll see when upstream has a systemd unit file we prefer that.

Also please keep to a consistent 2 spaces per level of indentation.

@doronbehar

This comment has been minimized.

Copy link
Contributor

doronbehar commented Jan 16, 2020

I don't know how to package existing services in nixos (I will find out)

When I said "package files from upstream", I meant that we need to make sure our onedrive package definition should include the files from https://github.com/abraunegg/onedrive/tree/master/contrib . I assumed you are somewhat familiar with the nixpkgs packages' build system (this is where most people start) - sometimes, packages build systems "install" such files to the the prefix which is $out in our case. @SRGOM If almost every word / phrase I'm using here isn't understood to you, please read about this in the manual, as I can't fill for you all of the necessary gaps here and now.

In general, my comment comes from the question: What does this service / module does that the package doesn't provide by it self? And if the package should provide something, I think it'd be better to leave it that way.

@SRGOM

This comment has been minimized.

Copy link
Member Author

SRGOM commented Jan 16, 2020

Alright, I'm going to address your comments as soon as I can (I'll probably need to read up quite a bit). But quickly, @doronbehar- my code allows running the service for an unlimited number of onedrive accounts for the same user with very little effort. Just list all the config dirs in ~/.config/onedrive-launcer and onedrive services will be instantiated for all those accounts.

@SRGOM

This comment has been minimized.

Copy link
Member Author

SRGOM commented Jan 16, 2020

Actually I just re-read both your comments and yeah, I happen to (fortunately) understand nix packaging. I understand (now) how services in Nix are handled, just like bins are moved to $out/bin, services are moved to $out/....servies path.... Fine.

With that understanding I want to say that the module I am submitting is very different and cannot make use of anything but adapted code from upstream.

In my code- I have two things- one is an instantiable "onedrive@" service, which instantiates on config directories. The other is a launcher service which launches each of these instantiated services.

The upstream code cannot be reused for that purpose without adapting.

@aanderse I will fix spacing.

@SRGOM SRGOM mentioned this pull request Jan 17, 2020
1 of 10 tasks complete
nixos/modules/programs/onedrive.nix Outdated Show resolved Hide resolved
nixos/modules/programs/onedrive.nix Outdated Show resolved Hide resolved
nixos/modules/programs/onedrive.nix Outdated Show resolved Hide resolved
@SRGOM SRGOM force-pushed the SRGOM:onedrivelll branch from ae1ef46 to b530c2c Jan 17, 2020
@SRGOM

This comment has been minimized.

Copy link
Member Author

SRGOM commented Jan 17, 2020

Documentation screenshot-
DeepinScreenshot_select-area_20200117204941

@SRGOM

This comment has been minimized.

Copy link
Member Author

SRGOM commented Jan 17, 2020

I am also adding config.services.onedrive.package to environment.systemPackages- this is because first run needs to be manual for setting up oauth by visiting link on the browser.

@SRGOM SRGOM changed the title [WIP]nixos/onedrive: init nixos/onedrive: init Jan 17, 2020
@ianmjones

This comment has been minimized.

Copy link
Member

ianmjones commented Jan 17, 2020

@SRGOM Looks great!

@aanderse

This comment has been minimized.

Copy link
Contributor

aanderse commented Jan 17, 2020

Just out of curiosity... is this module something which might be better suited for home-manager instead?

@SRGOM

This comment has been minimized.

Copy link
Member Author

SRGOM commented Jan 17, 2020

I'm not sure what defines the boundaries and shared regions but in this particular case, if multiple users use a single OneDrive account, no configuration on their part besides the initial oauth is required once the sysadmin enables the service. That's a strong argument in favor of having a "global " service.

That said, I'm always curious why somebody would use home manager at all? Having packages on a multi user system, sure- but it's a bad idea as far as config management is concerned - you cannot edit a file and let things apply on the fly (unlike the dotfile + stow approach). And you don't have a "stateless" config directory anyway- things are very stateful. Also when I open my ~/.ssh/config in vim, ProxyCommand is colored but PoxyCommand isn't. Not something home manager can give yopu.

_
@SRGOM SRGOM force-pushed the SRGOM:onedrivelll branch from b530c2c to dff5f86 Jan 18, 2020
@SRGOM

This comment has been minimized.

Copy link
Member Author

SRGOM commented Jan 18, 2020

DeepinScreenshot_select-area_20200118124150

@nixos-discourse

This comment has been minimized.

Copy link

nixos-discourse commented Jan 18, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/85

@abraunegg

This comment has been minimized.

Copy link

abraunegg commented Jan 18, 2020

  • Use $XDG_CONFIG_HOME instead of .config. The author of the main package- onedrive, uses .config and not $XDG_CONFIG_HOME in https://github.com/abraunegg/onedrive/blob/master/docs/USAGE.md. I would prefer using XDG but would go with .config because that's what the author seemingly uses, not sure what he's doing in code

@SRGOM

Great question. Based on extensive testing and fixing issues / bugs from the original client, many distro's do not set correctly the XDG_CONFIG_HOME value, combined with that if the system is entirely headless (no X11) this value will also not exist.

In the code itself (config.d) there is the following:

// Determine the base directory relative to which user specific configuration files should be stored.
if (environment.get("XDG_CONFIG_HOME") != ""){
	log.vdebug("configDirBase: XDG_CONFIG_HOME environment variable set");
	configDirBase = environment.get("XDG_CONFIG_HOME");
} else {
	// XDG_CONFIG_HOME does not exist on systems where X11 is not present - ie - headless systems / servers
	log.vdebug("configDirBase: WARNING - no XDG_CONFIG_HOME environment variable set");
	configDirBase = homePath ~ "/.config";
}

This allows to check if XDG_CONFIG_HOME is set, but if not use ~/.config

If NixOS is reliably setting XDG_CONFIG_HOME 100% of the time, and this is the norm for the distro from a documentation perspective, I would rather you follow your documentation standards than 'break' that just for this application.

Other than that, the documentation changes / additions look fine to me.

@SRGOM

This comment has been minimized.

Copy link
Member Author

SRGOM commented Jan 19, 2020

@abraunegg Thank you, clearly you've spent time worrying about stability. That if else check makes me want to use XDG_CONFIG_HOME but (and I forgot to update my original post) XDG_CONFIG_HOME isn't available in the systemd service.

Entirely headless => no XDG_CONFIG_HOME

Interesting.

Copy link
Member

Infinisil left a comment

Not sure about this module, having the user write a config file to their home directory to configure something is not really in the spirit of NixOS at all (which is about declarative system config, not stateful user services). Perhaps home-manager is a better place for such a module, since it's focused on single users, allowing you to control home directory paths too.

@SRGOM

This comment has been minimized.

Copy link
Member Author

SRGOM commented Jan 21, 2020

You mention "not sure". Is that a final in which case I will close because I have no motivation to write a home manager module. As I noted above, and I'm not being snarky,

That said, I'm always curious why somebody would use home manager at all? Having packages on a multi user system, sure- but it's a bad idea as far as config management is concerned - you cannot edit a file and let things apply on the fly (unlike the dotfile + stow approach). And you don't have a "stateless" config directory anyway- things are very stateful.

Also when I open my ~/.ssh/config in vim, ProxyCommand is colored but PoxyCommand isn't. Not something home manager can give yopu.

This isn't the occasion to talk about Home Manager but suggesting an unofficial solution here seems a bit off when it fits well in the official solution just fine. Creating a config file in the home directory is like creating a vimrc or emacs .el. . It's optional.

Thank you for spending the time to look and for maintaining NixOS. (I hope I didn't sound ungrateful with my above comments, it's just that I feel a bit strongly about home manager I feel that HM is the nail to the (amazing) Nix hammer. I am 100% against entitlement, this is your repo and you guys are free to do whatever and I am free to fork NixOS (and get nowhere). But I think most of what I'm doing is inline with other projects.

@ianmjones

This comment has been minimized.

Copy link
Member

ianmjones commented Jan 21, 2020

What if there was a compromise that meant the system admin could declare which users OneDrive is enabled for?

{
  services.onedrive.package = pkgs.onedrive;

  services.onedrive.users."ian" = {
    enable = true;
    configDirs = [ ".config/onedrive" ".config/office365" ];
  };
}

This means the systemd services are controlled by the system administrator rather than users.

The module would only set up the services where declared users have a config file in one of the prescribed config directories.

services.onedrive.users.<name?>.configDirs would be optional, defaults to value of services.onedrive.configDirs.

services.onedrive.configDirs would be optional, defaults to [ ".config/onedrive" ].

configDirs entries are always evaluated relative to the user's home dir.

@Infinisil

This comment has been minimized.

Copy link
Member

Infinisil commented Jan 21, 2020

A general problem with NixOS modules that control user-level things is that users can't control it unless they're a super-user: If you want to enable such a service, you need to be able to call nixos-rebuild switch.

In contrast, home-manager allows you to do that, because it runs on a per-user basis, with its config file in ~/.config/nixpkgs/home.nix. And for the people who have single-user systems, they can also run home-manager as part of NixOS, allowing you to configure home-manager from your configuration.nix:

{
  home-manager.users.alice = {
    services.onedrive.enable = true;
  };
}

home-manager is nowadays pretty much the standard way to configure user-level things. Yes it's a third-party tool, but I think we should embrace this, because nixpkgs is really quite big already. Once NixOS/rfcs#42 is finished it will also allow easier usage of third-party Nix code and libraries.

@abraunegg

This comment has been minimized.

Copy link

abraunegg commented Jan 21, 2020

Given I have absolutely no knowledge of NixOS, please can you keep this in mind when reading below & making your decision as how best to run this under your OS / distribution:

  1. If you run 'onedrive' as a 'super user' (eg root) either via cron or systemd, then all the files downloaded from OneDrive will have a UID/GID = this user. The issue here is that 'many' folk run this as a system service (see https://github.com/abraunegg/onedrive/blob/master/docs/USAGE.md#onedrive-service-running-as-a-non-root-user-via-systemd-without-notifications-or-gui). Example - you run as systemd, but configure the sync_dir to be /path/to/my/files - essentially you will get access / permission issues and problems when attempting to read / write the files.
  2. Whilst the binary is installed with root privileges, standard users can run & configure the application as to how they want to 'use it'. Essentially the application 'has' to run as the user that intends to access / use the files. This allows then 'many' users on the same system, at the same time can use the same binary, but with their unique configuration to access OneDrive & sync in the manner that they want.
  3. The ~/.config/onedrive user path serves the following purposes:
  4. The config path is configurable via the CLI (or by modifying the systemd / initd scripts) by using --confdir - if ~/.config/onedrive if not applicable, then store elsewhere, but ensure that the --confdir is used at each application runtime.

@ianmjones

This means the systemd services are controlled by the system administrator rather than users.

See above - unless you enable the right user service, then it will cause issues when downloading files as a non super / system admin user.

@ianmjones

This comment has been minimized.

Copy link
Member

ianmjones commented Jan 21, 2020

@abraunegg said...

unless you enable the right user service, then it will cause issues when downloading files as a non super / system admin user.

Yup, this module for NixOS only creates user level systemd units, and only starts the service for the user if they have a recognised config dir for onedrive.

@SRGOM

This comment has been minimized.

Copy link
Member Author

SRGOM commented Jan 22, 2020

Thank you, I appreciate the good faith discussion. I can see how home manager shines here. I want to add one thing though (I hope I'm not answering a non-question)-

Unlike other distros, NixOS allows users to install their own packages. So I can see how it might be a concern that admins are required for a service. But that's true for a lot of other services, (redshift comes to mind). So this service isn't much different from others really.

Also, just to give an idea of how this interacts with config files.

Assume disabled service. Nothing happens.

Assume enabled service. If I as a user have .config/OneDrive, then I likely want the service to run anyway. But the module gives you an option anyway to not run the individual service if you do not wish.

Basically once the admin enables the service, only the oneshot onederivelauncher service starts mandatorily . Everything else is trivially manageable by the user.

In fact I'd argue that redshift (and probably others) don't belong in nix because there is no way to configure different color temperature preferred by different people. I like 1800k candle flame, most are used to 2700k tungsten filament bulb.

@SRGOM

This comment has been minimized.

Copy link
Member Author

SRGOM commented Jan 22, 2020

@ianmjones Sorry I missed your previous comment- I believe the concern infinisil's concern here is the lack of flexibility (please correct me if not). As such my previ0us comment tries to address that.

Was your concern in response to that. If not, does my previous comment seem to address what your motivation might have been? Happy to answer more

@ianmjones

This comment has been minimized.

Copy link
Member

ianmjones commented Jan 22, 2020

@SRGOM No worries, my previous comment was indeed trying to address @Infinisil's original comment about not liking the requirement of users adding config files to their home dir that are specific to this module (that's how I read their comment). As you know, I too raised concerns about the ~/.config/onedrive-launcher file, it doesn't feel right.

It also doesn't feel right to me that we're currently needing to install a third party channel (home-manager) in order to get per user declarative config, hence my suggested compromise.

However, I do like the format that @Infinisil's follow-up home-manager example used for enabling the onedrive service on a per user basis, so I'm going to give home-manager a try.

It is a shame though that we can't have a OneDrive service module that enables users to optionally use it simply by having the standard config in place, and doing the one-time manual authentication to confirm access.

@SRGOM

This comment has been minimized.

Copy link
Member Author

SRGOM commented Jan 22, 2020

I actually think that allowing the user the flexibility of turning on and off sync without a system rebuild is a positive. I believed that your concern arose from it not being a package defined standard but something my module created ( I share that but I don't think it's a deal breaker).

@nixos-discourse

This comment has been minimized.

Copy link

nixos-discourse commented Feb 4, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/how-to-setup-user-ssh-keys/5745/4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.