pykms: fix/enhance logging #81366

pvgoran · 2020-02-29T12:33:36Z

Motivation for this change

The current pykms package (version 20190611, introduced at bf9cecf) does logging in a problematic way: it is sed-patched to log to the systemd journal, but at the same time it still creates a log file inside Nix store by default (if the --logfile option is not specified). This prevents this package's binaries from being used without specifying the --logfile option. Also, the journal logs are prefixed with a long full path to the python file in Nix store, which make it quite inconvenient to look at them.

So I replaced the current "sed-patch" with another patch which changes the default location of the log file to be in the current directory, and doesn't involve the systemd journal. This way, the binaries can be used without the --logfile option.

Also, I added the SyslogIdentifier systemd unit option for better identification of pykms' logs.

The result is tested and works as intended.

Things done

flokli · 2020-04-12T10:44:32Z

pkgs/tools/networking/pykms/default.nix

+@@ -48,5 +48,5 @@
+                     'choi' : ["CRITICAL", "ERROR", "WARNING", "INFO", "DEBUG", "MINI"]},
+         'lfile' : {'help' : 'Use this option to set an output log file. The default is \"pykms_logclient.log\" or type \"STDOUT\" to view log info on stdout.',
+-                   'def' : os.path.dirname(os.path.abspath( __file__ )) + "/pykms_logclient.log", 'des' : "logfile"},


Did you approach upstream on that? The help text reads like it should log to pykms_logclient.log in the current working directory, not relative to the location on where pykms is installed.

This should cause problems in other distros as well, if pykms is installed by the distros package manager, and not world-writeable.

Well, creating the log file in the executable's directory seems to be a deliberate decision made upstream. I don't think I'm willing to debate it with them.

I opened SystemRage/py-kms#64 to discuss this upstream.

@flokli Thank you.

Do we really care about this? pykms can be given an absolute path to log to (or STDOUT which is what the the module currently is doing).

@peterhoeg I think this is worth caring for. It's better if the program works out-of-the-box, without requiring to specify this kind of parameters.

At the very least, I firmly disagree with the way the logging behaviour is tuned by the nixpkgs package right now.

flokli · 2020-04-12T10:45:31Z

pkgs/tools/networking/pykms/default.nix

@@ -29,6 +29,29 @@ let
    fi
  '');

+  loggingPatch = 


We usually don't inline patches inside nix.

We prefer to fetch them from a PR to upstream, or if that's not feasible, ship them as a patchfile next to the default.nix.

Fetching patches from a PR seems fragile. If I make a PR upstream and reference it in .nix, what happens if this PR is deleted for some reason?

I can extract the patch into a separate file, though.

Usually, referring to the patch via something like https://github.com/$upstreamOwner/$upstreamRepo/pull/$pr/commits/$commitID.patch should be stable, at least as long as the fork still has this commit on some branch. But I agree, moving to a separate file might also be okay.

You can reference the commit instead of the PR. The commit should be immutable even if you delete the PR.

@Mic92 And if I delete the branch?

@Mic92 And if I delete the branch?

Also in that case. This commit is no longer accessible by any branch but the link still works: Mic92/dotfiles@18869fa

@Mic92 Thanks.

peterhoeg · 2020-04-16T09:21:50Z

👍 on the changes. The easiest is just to stick the patch into a separate file and be done with it if upstream doesn't want to take your patch.

peterhoeg · 2020-04-29T00:55:24Z

@peterhoeg I think this is worth caring for. It's better if the program works out-of-the-box, without requiring to specify this kind of parameters.

But it does though. If you use the module it will log to stdout. Many of the services we expose on nixos do not run when invoked *manually* but require the nixos module to set up the proper environment.

At the very least, I firmly disagree with the way the logging behaviour is tuned by the nixpkgs package right now.

I'm with you 100% on this. It's ugly AF and your way is much better. In principle, there is nothing wrong with deviating from upstream if the deviation makes more sense. And I am also in agreement with you on this - writing to the binary path is not the way to do it. If you want to maintain the deviation that's not a problem while we wait for upstream to (maybe) fix things. If not, it's better just to stick to upstream and work around what we need to in the module as that will be the entry point for the vast majority of users. But inlining the patch is not right - it should be separated out.

pvgoran · 2020-04-29T03:32:08Z

I force-pushed another version, with the patch separated into a file. Also, I made a PR upstream: SystemRage/py-kms#66 . I couldn't use this PR here, though, because the patch is different (the affected lines changed in upstream's master).

@peterhoeg What you write makes perfect sense. However, what is the rationale for not inlining patches? (Just curious.)

I don't mind maintaining the change/the package, although I'm not familiar with the maintenance processes.

peterhoeg · 2020-04-29T03:46:29Z

I force-pushed another version, with the patch separated into a file. Also, I made a PR upstream: SystemRage/py-kms#66 . I couldn't use this PR here, though, because the patch is different (the affected lines changed in upstream's master).

Please add a comment to default.nix above the patch where you reference the upstream issue. It makes it nicer for the next person who looks at this.

@peterhoeg What you write makes perfect sense. However, what is the rationale for not inlining patches? (Just curious.)

I don't know if there is an official set of reasons, but my reasoning has always been as follows: 1. we stick to upstream as much as possible so patches are meant to be short-lived and ideally reside completely outside of nixpkgs because they are already PRs/MRs tracking an upstream issue. 2. if we need to carry the patch(es) ourselves, leave it in a separate file to minimize the noise in the main nix expression. A change that simply drops a line from default.nix and removes an external file makes for a nicer commit history compared to one that adds and removes 50 lines inside the main nix expression. 3. a shorter file is just easier to grok and when reading the main nix expression the patch doesn't add anything to the understanding whereas a single line that says `patches = [ ./log-to-current-directory-by-default.patch ];` very clearly conveys what is going on here.

The logging "sed-patch" that was introduced for version 20190611 worked poorly: it was too intrusive (breaking the --logfile option), and it didn't prevent using in-store file for logging by default. The new logging patch (an actual "diff-patch") is less intrusive: it just changes the default log file's location to be the current directory instead of the executable's directory.

pvgoran · 2020-04-29T03:55:28Z

Please add a comment to default.nix above the patch where you reference the upstream issue. It makes it nicer for the next person who looks at this.

@peterhoeg Done, and thanks for explaining.

pvgoran changed the title ~~Pykms fix logging~~ pykms: fix/enhance logging Feb 29, 2020

pvgoran mentioned this pull request Feb 29, 2020

Update pykms to 20190611, adapt pykms service #80363

Closed

10 tasks

ofborg bot added 6.topic: nixos 8.has: module (update) labels Feb 29, 2020

ofborg bot requested a review from peterhoeg February 29, 2020 12:44

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Feb 29, 2020

flokli reviewed Apr 12, 2020

View reviewed changes

pvgoran force-pushed the pykms-fix-logging branch from e0a0376 to 11ec14a Compare April 29, 2020 03:13

ofborg bot added 10.rebuild-darwin: 1 10.rebuild-linux: 1 labels Apr 29, 2020

pvgoran added 2 commits April 29, 2020 03:53

nixos/pykms: add SyslogIdentifier

c678d68

pvgoran force-pushed the pykms-fix-logging branch from 11ec14a to c678d68 Compare April 29, 2020 03:53

peterhoeg merged commit 83a57cf into NixOS:master Apr 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pykms: fix/enhance logging #81366

pykms: fix/enhance logging #81366

pvgoran commented Feb 29, 2020

flokli Apr 12, 2020

pvgoran Apr 12, 2020

flokli Apr 14, 2020

pvgoran Apr 14, 2020

peterhoeg Apr 24, 2020 •

edited

pvgoran Apr 28, 2020

flokli Apr 12, 2020

pvgoran Apr 12, 2020

flokli Apr 14, 2020

Mic92 Apr 14, 2020

pvgoran Apr 14, 2020

Mic92 Apr 15, 2020

pvgoran Apr 19, 2020

peterhoeg commented Apr 16, 2020

peterhoeg commented Apr 29, 2020 via email

pvgoran commented Apr 29, 2020

peterhoeg commented Apr 29, 2020 via email

pvgoran commented Apr 29, 2020

pykms: fix/enhance logging #81366

pykms: fix/enhance logging #81366

Conversation

pvgoran commented Feb 29, 2020

Motivation for this change

Things done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterhoeg Apr 24, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterhoeg commented Apr 16, 2020

peterhoeg commented Apr 29, 2020 via email

pvgoran commented Apr 29, 2020

peterhoeg commented Apr 29, 2020 via email

pvgoran commented Apr 29, 2020

peterhoeg Apr 24, 2020 •

edited