xtask: redirect stdout and stderr into log files

[gabriele-0201]

No description provided.

[gabriele-0201]

Current dependencies on/for this PR:

• main
  • PR xtask: redirect stdout and stderr into log files #231 👈

This stack of pull requests is managed by Graphite.

[rphmeier]

Given that these are all flattened into the same Params, how do you differentiate them?

Which one does --log-path X trigger?

[gabriele-0201]

The trick with value_name and id makes the items look like <prefix>-log-path, where the prefix could be zombienet, shim, or sovereign to specify the path where you want to output the logs of the prefix process. This is a workaround, though, and the discussion on how the flags should look like can be found here: #224

[pepyakin]

1. The documentation for create states, quote,

This function will create a file if it does not exist, and will truncate it if it does.
which suggests that set_len may be redundant.

2. It is possible that the log_path points to a file located in a directory that doesn't exist. In that case, I am pretty sure that create can return an error. The intuitive behavior would be to create all the parent directories leading to the specified file path.
3. A more high-level consideration, do you think that failure creating of the log file warrants terminating the process? I would say this is not super important, but if it's easy to fix then we could do that.

[gabriele-0201]

1. Totally right, both are not needed. I will remove set_len
2. Nice edge case. I didn't think about it yesterday. If I'm correct, there is no option in the File struct to create all the directories, but it needs to be done in two steps with std::fs::create_dir_all
3. Probably a fallback into the default log position firstly, and if that fails too, maybe just print stuff to stdout? It is absolutely doable!

[gabriele-0201]

Ok, if the creation of the folder specified in the path or the file itself is a problem, then stdout is the fallback. Implementing this was a little more complex than expected, but I hope to have come up with a versatile and good enough solution

[pepyakin]

FWIW, I was more thinking of ignoring the failure and discarding output (although maybe with a warning). This is not critical data, you can always re-run the code. I think the criteria here should be the simplicity of the implementation.

[gabriele-0201]

Oh, okay, then I added a small modification. However, for how the duct is built, even discarding the output would require calling a method on the expression. It would be something like expression.stdout_null().stderr_null().run() instead of expression.stderr_to_stdout().stdout_file(log_file).run().

If you prefer, it would be really easy to switch to discarding the output instead of printing it to stdout in the current implementation, so let me know which one you think is more appropriate!

[pepyakin]

I guess it's fine for now

[pepyakin]

I don't think it's a good idea to mix logs with the source files.

Recall my suggestion where we could store all the test data files (for zombienet, sovereign demo rollup, and now logs) in a single place. Do you disagree with it?

[gabriele-0201]

Absolutely not! I would really like to have all data in the same place but to be fair, with the current setup, I would not know how to place the zombienet folder, the demo-data folder, and the logs in the same location.

The problem is about the zombienet and logs, but mainly the sovereign rollup as described here: #227