Add a settings option for log-format #3961

bqv · 2020-08-26T03:55:39Z

This removes the need to pass --log-format to every nix invocation for your log-format of choice

src/libmain/loggers.cc

edolstra · 2020-08-28T09:16:25Z

This appears to break the --log-format command line option, for example

nix eval --expr '1 + 2'  --json --log-format internal-json -vvvv

no longer prints any JSON messages.

src/libmain/loggers.cc

src/libmain/common-args.cc

bqv · 2020-08-28T23:20:03Z

This appears to break the --log-format command line option, for example

I'm unsure why this is, as of now. I'll get back to this a little later

cole-h · 2020-09-01T07:10:48Z

I tried debugging the issue where --log-format no longer works, but wasn't able to get anywhere due to my lack of familiarity with the nix codebase. All I figured out was that settings.logFormat.get() always returns the default value (an empty string).

Seeing that flags like --extra-substituters work, I imagine settings.<name>.get() is the proper way to get the value, but I don't know why the value of --log-format is not being set in the first place...

(BTW: log-format can be safely removed from MixCommonArgs, as it doesn't actually appear to do anything -- I put an abort(); in the handler and nothing happened when passing --log-format asdf.)

bqv · 2020-09-05T06:05:09Z

It works!

Though I ended up juggling around more things than would have been ideal...

cole-h · 2020-09-06T19:43:16Z

Applying the following patch allows both --option log-format and --log-format to work (by moving the createDefaultLogger() call into the setter), as well as removes --log-format from MixCommonArgs and fixes the build (by fixing the toJSON() function): https://gist.github.com/cole-h/ec8e4334e6595c2848252e9dda54941d.

EDIT: Do note, however, that this means libstore depends on libmain (for access to createDefaultLogger()). I don't know if this is desirable or not (or even extremely undesirable), but I can't see any other way.

Kloenk · 2020-09-10T17:55:05Z

So its ready to be merged? Would need this for one of my prs.
CC @edolstra

thufschmitt

EDIT: Do note, however, that this means libstore depends on libmain (for access to createDefaultLogger()). I don't know if this is desirable
or not (or even extremely undesirable), but I can't see any other way.

That's a bit annoying imho. IIRC there was a good reason to keep the progress bar in libmain, but a good (?) way to break this would be to mak
e the logger pluggable like we do for the stores. Something like

// Somewhere in libutil                                                                                                                       
struct LoggerBuilder {                                                                                                                        
  std::string name;                                                                                                                           
  std::function<(Logger*) void*> builder;                                                                                                     
};                                                                                                                                            
                                                                                                                                              
static std::vector<std::shared_ptr<LoggerBuilder>> registeredLoggers;                                                                         
                                                                                                                                              
void registerLogger(std::string name, std::function<(Logger*) void> builder) {                                                                
  LoggerBuilder lBuilder{ .name = name, .builder = builder };                                                                                 
  registeredLoggers.push_back(std::shared_ptr<LoggerBuilder>(lBuilder));
}

Logger* makeDefaultLogger() {
  for (auto & builder : registeredLoggers) {
    if (builder.name == settings.logFormat) {
      return builder.builder();
    }
  }
  throw Error("Unknown logger %s", settings.logFormat);
}

// Can be called in the file defining the logger
registerLogger("bar", []() -> Logger* { makeProgressBar(false); });
registerLogger("bar-with-logs", []() -> Logger* { makeProgressBar(true); });
registerLogger("json", []() -> Logger* { makeJSONLogger(); });

And then we can recover the list of values for the logger option by introspecting the value of registeredLoggers:

std::string listLogFormats()
{
  std::string res;
  for (auto & builder : registeredLoggers) {
    if (res) res += ", ";
    res += builder.name;
  }
  return res;
}

    Setting<LogFormat> logFormat{this, LogFormat::bar, "log-format",
        format("Default build output logging format; %s.", listLogFormats()};

thufschmitt · 2020-09-17T16:00:07Z

src/libstore/globals.cc

+    else abort();
+}
+
+template<> nlohmann::json BaseSetting<LogFormat>::toJSON()


This isn't correct anymore in master (toJSON() isn't expected to be equal to toString() anymore). You can get the templated definition by including abstractsettingtojson.hh

FWIW, abstractsettingtojson.hh is already included by globals.hh, so there was no need to include it in globals.cc as well.

thufschmitt · 2020-09-17T16:01:07Z

src/libstore/globals.cc

+
+template<> void BaseSetting<LogFormat>::convertToArg(Args & args, const std::string & category)
+{
+    args.addFlag({


There's a way to define custom completers for the cli, you could use that so that the autocompletion suggest the different log formats

I've done so (based on the completion function for hashTypes), but I don't know how to test if it really works (nor can I, AFAIK, as I use fish rather than bash or zsh).

cole-h · 2020-09-18T08:26:42Z

Thanks @regnat for the help! Following your advice (both here and on IRC), I refactored the loggers stuff to be pluggable (letting us drop the libmain <- libstore dependency).

There's one thing I'm still unhappy with, however: the fact that man nix.conf shows the default value as "3" (because it's an enum variant), instead of "bar". I don't know if there's a way to make whatever generates these defaults be run through a to_string() of some sort, but that would be swell. I've left a FIXME indicating this lack of satisfaction (input welcome).

One idea I just had would be to create a new log format type that's really just a string (as done with Path in libutil/types.hh), in order to override the default logic and also show the logger's string name in the manpage. Thoughts, anyone?

cole-h · 2020-09-18T08:34:24Z

src/libstore/globals.hh

@@ -12,6 +13,8 @@

 namespace nix {

+std::string listLogFormats();


I don't really like that this is outside the Settings class, but putting it anywhere inside gives undefined references errors (due to its use in the logFormat description). Happy to hear alternatives.

I think you're allowed to make it a static member of the Settings class if you want to

With @LnL7's help, I created a new type for LogFormatSetting, which lets me remove listLogFormats from publicity and instead just call it in the constructor for that setting.

(Your static idea didn't seem to work.)

thufschmitt · 2020-09-18T15:24:07Z

Thanks @cole-h , I like that a lot!

There's one thing I'm still unhappy with, however: the fact that man nix.conf shows the default value as "3" (because it's an enum variant), instead of "bar". I don't know if there's a way to make whatever generates these defaults be run through a to_string() of some sort, but that would be swell. I've left a FIXME indicating this lack of satisfaction (input welcome).

One idea I just had would be to create a new log format type that's really just a string (as done with Path in libutil/types.hh), in order to override the default logic and also show the logger's string name in the manpage. Thoughts, anyone?

That sounds definitely a good thing (The static enum doesn't really make sense now that the thing is pluggable imho).

Regarding your IRC message:

in the generated nix.conf.5 manpage, bar and bar-with-logs won't show up because they're declared outside of libutil

I think I found the reason why: both settings and registeredLoggers (and all the registerLoggers calls) are static variables. And in C++ the order in which they will be initialized isn't defined. This means that it's totally possible that settings will be instantiated while only part of the loggers have been registered, which in turn means that only this subset of loggers will show-up in the help (and the manual).
A way out of that is to replace the toplevel Settings settings; (in globals.hh) by a function Settings* settings() that will lazily initialize a Settings object and return a pointer to it (that way we're guaranteed that it will be instantiated after all the loggers have been registered). I've tried it in tweag@b52f6be, and it definitely works, except that it's quite invasive because it requires rewriting all the accesses to settings.foo into settings()->foo (and there's a lot of these). So it might not be such a good idea.
Another option would be to allow making the description of a setting lazy (so that we only evaluate listLogFormats() once everything is registered), but I don't see how to do that nicely.

cole-h · 2020-09-19T18:49:40Z

Because @LnL7 showed me how to get the json library to specialize enum conversion, the string wrapper is no longer necessary.

I also moved progress-bar.{hh,cc} and names.{hh,cc} to libutil, which allows all the loggers to be registered in libutil (so they all show up in the manpage).

I think this is ready for merge (or at least "final review"). Thanks everyone for your help (especially @regnat)!

thufschmitt

Nice 👍 I don't have the commit rights for the repo, but it looks all good on my side

(I'm not utterly fond of having both a static list of known loggers and a registration mechanism, but it's definitely not a blocker for me).

cole-h · 2020-09-28T18:14:09Z

Ping @edolstra -- would appreciate another review (or, even better, a merge) on this, when you get a chance.

Ericson2314 · 2020-10-09T00:54:48Z

This is really cool! I'm especially excited because I think it might make it easier for hydra alternatives to stream logs per derivation?

Hope to see this land soon :).

bqv · 2020-10-15T18:41:37Z

@edolstra ping, have you had a chance to look over this?

Co-authored-by: Cole Helbling <cole.e.helbling@outlook.com>

Thanks to some precision guidance by regnat, we can remove libstore's dependency on libmain by just making the logging mechanisms pluggable. This: * moves loggers.{hh,cc} from libmain to libutil * removes the now-unnecessary (and incorrect) `toJSON` specialization * moves LogFormat from types.hh to loggers.hh * adds a completer (untested) To add another logger, the following steps should be all that's necessary: 1) Add enum variant in libutil/loggers.hh and name to the `std::set` in loggers.cc 2) Modify the specialized `set` and `to_string` methods to include the new variant 3) Use the `OnStartup` class to `registerLogger` the new logger method Co-authored-by: regnat <rg@regnat.ovh>

It is no longer necessary to add the logger name to the `logFormats` set when adding a new logger (due to its removal). Additionally, the manpage now displays the string "bar" as the default, rather than an unhelpfully opaque integer. Co-authored-by: regnat <rg@regnat.ovh> Co-authored-by: Daiderd Jordan <daiderd@gmail.com>

This fixes manpage generation excluding the `bar` and `bar-with-logs` log formats that were registered outside libutil; now all formats appear. Note that any loggers registered outside of libutil might not appear in the manpage -- probably due to the fact that C++ doesn't define the order in which static variables are initialized (as regnat noted). This also removes the unused store-api.hh include in progress-bar.cc.

bqv · 2020-11-18T12:11:22Z

Seems eelco was ignoring this because he has other plans 490861e

bqv · 2020-12-22T02:53:56Z

Close in favor of #4296

bqv force-pushed the master branch from 8bd6d46 to b3b6a87 Compare August 26, 2020 04:02

thufschmitt reviewed Aug 27, 2020

View reviewed changes

src/libmain/loggers.cc Outdated Show resolved Hide resolved

bqv force-pushed the master branch from b3b6a87 to fc7e060 Compare August 27, 2020 22:19

edolstra reviewed Aug 28, 2020

View reviewed changes

src/libmain/loggers.cc Outdated Show resolved Hide resolved

edolstra reviewed Aug 28, 2020

View reviewed changes

src/libmain/common-args.cc Outdated Show resolved Hide resolved

edolstra mentioned this pull request Aug 31, 2020

progress-bar: multiple output lines #3972

Open

bqv force-pushed the master branch 2 times, most recently from e830b11 to 9dc53a2 Compare September 5, 2020 06:04

bqv force-pushed the master branch 2 times, most recently from 18188ad to fa311d1 Compare September 7, 2020 06:49

bqv requested a review from edolstra September 10, 2020 23:33

bqv force-pushed the master branch 2 times, most recently from b1600b3 to b024a78 Compare September 14, 2020 12:00

thufschmitt requested changes Sep 17, 2020

View reviewed changes

cole-h force-pushed the master branch from b024a78 to 8a945d6 Compare September 18, 2020 08:15

cole-h reviewed Sep 18, 2020

View reviewed changes

cole-h force-pushed the master branch 3 times, most recently from 99f6976 to 1422880 Compare September 19, 2020 18:40

cole-h force-pushed the master branch from 1422880 to bfba368 Compare September 20, 2020 07:21

thufschmitt approved these changes Sep 21, 2020

View reviewed changes

cole-h force-pushed the master branch 3 times, most recently from cbc9f1d to 8f4c71e Compare September 28, 2020 16:37

cole-h force-pushed the master branch from 8f4c71e to 746b4bd Compare October 9, 2020 06:51

cole-h force-pushed the master branch 2 times, most recently from 6cc4cd9 to a67a66f Compare October 24, 2020 03:13

bqv and others added 2 commits October 30, 2020 10:42

Add a settings option for log-format

6822bdb

Co-authored-by: Cole Helbling <cole.e.helbling@outlook.com>

cole-h force-pushed the master branch from a67a66f to 98289e9 Compare October 30, 2020 17:45

cole-h and others added 2 commits October 30, 2020 11:42

cole-h force-pushed the master branch from 98289e9 to 9a38e06 Compare October 30, 2020 18:42

bqv closed this Dec 22, 2020

cole-h mentioned this pull request Nov 14, 2021

allow settings log-format in nix.conf #5561

Open

cole-h mentioned this pull request Feb 20, 2022

Nix.conf for print-build-logs and verbose #5858

Open

cole-h mentioned this pull request Apr 4, 2022

src/libstore/globals.hh: add option log-format #6357

Closed

cole-h mentioned this pull request May 12, 2023

Make print-build-logs a setting #8323

Open

8 tasks

Ericson2314 mentioned this pull request Aug 18, 2023

Progress indicator improvements #4296

Draft

9999years mentioned this pull request Feb 3, 2024

Add setting handlers (side effects when settings are changed) #9922

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a settings option for log-format #3961

Add a settings option for log-format #3961

bqv commented Aug 26, 2020

edolstra commented Aug 28, 2020

bqv commented Aug 28, 2020

cole-h commented Sep 1, 2020

bqv commented Sep 5, 2020 •

edited

cole-h commented Sep 6, 2020 •

edited

Kloenk commented Sep 10, 2020

thufschmitt left a comment

thufschmitt Sep 17, 2020

cole-h Sep 18, 2020 •

edited

thufschmitt Sep 17, 2020

cole-h Sep 18, 2020

cole-h commented Sep 18, 2020

cole-h Sep 18, 2020

thufschmitt Sep 18, 2020

cole-h Sep 18, 2020

thufschmitt commented Sep 18, 2020

cole-h commented Sep 19, 2020

thufschmitt left a comment

cole-h commented Sep 28, 2020

Ericson2314 commented Oct 9, 2020

bqv commented Oct 15, 2020

bqv commented Nov 18, 2020

bqv commented Dec 22, 2020

		@@ -12,6 +13,8 @@

		namespace nix {

		std::string listLogFormats();

Add a settings option for log-format #3961

Add a settings option for log-format #3961

Conversation

bqv commented Aug 26, 2020

edolstra commented Aug 28, 2020

bqv commented Aug 28, 2020

cole-h commented Sep 1, 2020

bqv commented Sep 5, 2020 • edited

cole-h commented Sep 6, 2020 • edited

Kloenk commented Sep 10, 2020

thufschmitt left a comment

Choose a reason for hiding this comment

thufschmitt Sep 17, 2020

Choose a reason for hiding this comment

cole-h Sep 18, 2020 • edited

Choose a reason for hiding this comment

thufschmitt Sep 17, 2020

Choose a reason for hiding this comment

cole-h Sep 18, 2020

Choose a reason for hiding this comment

cole-h commented Sep 18, 2020

cole-h Sep 18, 2020

Choose a reason for hiding this comment

thufschmitt Sep 18, 2020

Choose a reason for hiding this comment

cole-h Sep 18, 2020

Choose a reason for hiding this comment

thufschmitt commented Sep 18, 2020

cole-h commented Sep 19, 2020

thufschmitt left a comment

Choose a reason for hiding this comment

cole-h commented Sep 28, 2020

Ericson2314 commented Oct 9, 2020

bqv commented Oct 15, 2020

bqv commented Nov 18, 2020

bqv commented Dec 22, 2020

bqv commented Sep 5, 2020 •

edited

cole-h commented Sep 6, 2020 •

edited

cole-h Sep 18, 2020 •

edited