I have some concerns whether line 13 is correct or not. For context see
https://github.com/ar-/incron/blob/master/usertable.cpp#L47 and
https://github.com/ar-/incron/blob/master/usertable.cpp#L605

Yes, this seems weird. Also, here https://github.com/ar-/incron/blob/1eedfbc9b318372efd119fd17f4abdbde561a53d/usertable.cpp#L605

What's the expected behaviour here? Am I reading it right that it clears PATH and then sets it according to line 13? Is that sensible at all compared to actually inheriting PATH?

@srhb my assumption is that since the user defined command runs as the user it doesn't seem entirely crazy to clear any unwanted extras out of the environment. By adding PATH back in as the standard list of installed software the command can execute in a clean state I guess?

Is the path I used valid to grant the command access to the full list of software available to the user? Even if the path is valid does it make sense in the context of nixos? We want to balance changing the intended behavior of the software and what users would expect vs. what makes sense for nixos.

Yes, this might be a case of conflicting logic especially with NixOS.

The path you added adds in the system profile, but not the user's profile, which is what seems like what incron should really be doing (or possibly both of them.) I'm not really sure how to do this in a sensible way aside from using ~/.nix-profile/bin on PATH as well. I feel like it isn't correct to try to reconstruct the user's PATH by manually adding these things onto it. After all, that's what the normal login process would take care of, so I guess I don't have any terribly good ideas on how to deal with the assumptions in this program.

@srhb I recall the issue now. The process is run as root and therefore it can't simply inherit PATH. So the issue becomes how to best reproduce what PATH would look like for a given user.

@srhb I've modified the PATH to better match what NixOS provides as the user path... which as you mentioned is unfortunately best guess manually reconstructing the users PATH variable. I believe this new code to be most inline with what existing users might expect if using this software, and what is most aligned with the intention of the software developer.

Is it not possible to use makeFlags = [ "PREFIX=$(out)" ];?

Unfortunately this would get overridden:

https://github.com/ar-/incron/blob/master/Makefile#L2

Would it? It usually works.

I just tested and in this case it does not.

Ok, found it: https://www.gnu.org/software/make/manual/html_node/Overriding.html#Overriding makeFlags really have greater priority.

I'm not sure what the issue is then... I pasted your suggested makeFlags in but get this error:

install -m 0755 -d /usr/local/share/man/man1
install: cannot create directory '/usr': Permission denied

It appears to me in this case the value is not being overridden.

Right, the issue is that you are overriding installPhase. The default installPhase does something like make $makeFlags $installFlags $installTargets.

You could switch to something like the following:

  installFlags = [ "PREFIX=$(out)" ];
  installTargets = [ "install-man" ];

  preInstall = ''
    mkdir -p $out/bin

    # make install doesn't work because setuid and permissions
    # just manually install the binaries instead
    cp incrond incrontab $out/bin/
  '';

This can be removed now

Yeah missed that on the last commit. Thanks for the explanation, that makes sense.

Why not use fetchFromGitLab?

Generally speaking, is fetchFromGitLab/fetchFromGitHub preferred over fetchurl?

Yes. There can be some better caching done or something.

Thank you for explaining. I've adjusted as recommended. Thank you.

The profiles are arbitrary. NIX_PROFILES environment variable would contain all of them but that probably will not be available to the systemd service. Instead, it is common to provide extraPackages option in the NixOS module and then add them to the service (example).

How would you recommend doing this on a per user basis? Or are you just suggesting to skip that entirely and have the extraPackages cover all users? If so, would that not restrict users somewhat as they can't modify configuration.nix?

Well would not be able to install packages to global profiles either, would they?

No, but that is why ~/.nix-profile/bin is included in the path as well.

Oh right, pwd->pw_name is probably the user’s name. Maybe this would be good enough until the upstream adds user service support, if a comment that this is does not support other profiles than the common ones is added.

I've added a comment... please suggest any changes to wording.

I've modified this so the user can explicitly set the path for the system incrontab. Each users incrontab will still rely on the packages available to that user, but that can still work out reasonably well considering the users.users..packages option.

On the other hand, the original code also does not try to emulate user’s PATH, so maybe we can just take the config.system.path from the service.

You can re-use name here so that updating is easier.

Use of install for mkdir -p and cp here might be slightly better, but this is a minor nit and should work as is.

This path is likely overbroad, and can enable accidental dependencies - e.g. if incron depends on foo being in path, which it is on your system and a test system, this will work, but on someone's minimal system, that might be missing.

Thanks for the suggestion. I've modified this so the user can explicitly set the path for the system incrontab.

Maybe instead of the patch to add PATH entries, we should instead delete this PATH-setting code and defer to some other PATH loading mechanism. This seems hairy and prone to problems.

I think that sounds like a fairly large change which would really belong upstream.

I went with the logic as I thought it best preserves the intended behaviour of the application.

@grahamc If I modified such that user incrontab files ran with config.system.path as the PATH as mentioned by @jtojnar and left the extraPackages option for the system incrontab would that be acceptable?

Minor nitpick: " is enough here.