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

makefiles/tools/serial.inc.mk: make use of pyterm session names #20121

Merged
merged 3 commits into from
Jul 11, 2024

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Nov 29, 2023

Contribution description

pyterm loggs all terminal output but the feature is very under-utilized.
Especially when multiple terminals are opened, this is very unreliable as all output goes to the default log file, so it is rather random which output gets logged.

Instead, create a session name based on the current time, the application and the board.
This helps recovering output later on.

Testing procedure

Run make term as usual, but now you will find multiple log files in ~/.pyterm:

/home/benpicco/.pyterm/T430s:
2023-11-29_20.13.51-gnrc_networking-same54-xpro.log
2023-11-29_20.17.53-hello-world-saml21-xpro.log

Issues/PRs references

@github-actions github-actions bot added Area: build system Area: Build system Area: tools Area: Supplementary tools labels Nov 29, 2023
@benpicco benpicco added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs labels Nov 29, 2023
@maribu maribu enabled auto-merge November 30, 2023 10:09
@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 30, 2023
@riot-ci
Copy link

riot-ci commented Nov 30, 2023

Murdock results

✔️ PASSED

59bb957 makefiles/tools/serial.inc.mk: pyterm: log to /tmp by default

Success Failures Total Runtime
1 0 1 01m:12s

Artifacts

@maribu maribu added this pull request to the merge queue Nov 30, 2023
@OlegHahm OlegHahm removed this pull request from the merge queue due to a manual request Nov 30, 2023
@OlegHahm
Copy link
Member

OlegHahm commented Nov 30, 2023

Wait a second before merging: I think I encoded the timestamp in the pyterm logfile name in a previous version but got annoyed of logfiles piling up. I any case I wonder whether this setting should be make in the build system or rather in pyterm itself.

Edit: At least the timestamp configuration. If we want to have board and application in there, it has to come from the environment.

@maribu
Copy link
Member

maribu commented Nov 30, 2023

Looks like this did stop the Murdock build, I'll cancel this now

@benpicco
Copy link
Contributor Author

Well the logfiles are piling up either way, with this they are at least somewhat useful.

@OlegHahm
Copy link
Member

I guess the question is whether one consider a one huge log file or an insane number of log files as more disturbing. How about creating subdirectories per board and creating/updating a tarball upon start for old log files?

@benpicco
Copy link
Contributor Author

The huge file is worse for sure because it's almost impossible to find anything in it.

How about moving the log files to /tmp/pyterm-logs by default. That way, if someone is interested in them, they can be saved. If not, they are removed with the next boot.

@OlegHahm
Copy link
Member

The huge file is worse for sure because it's almost impossible to find anything in it.

Why do you think that is easier to find anything in a plethora of files?

How about moving the log files to /tmp/pyterm-logs by default. That way, if someone is interested in them, they can be saved. If not, they are removed with the next boot.

In that case I would rather vote for disabling logging by default at all.

In the end, it's a matter of personal preferences. Pyterm used to have a config file, but I just realized that the functionality is broken in newer python versions. But I think having a personal config file would be probably the best way to cater all different needs.

@benpicco benpicco requested a review from fabian18 January 22, 2024 12:39
@benpicco
Copy link
Contributor Author

Why do you think that is easier to find anything in a plethora of files?

Because now it's basically random which session gets written if you have multiple instances of pyterm open. This makes the automatic logging rather useless (you can still find some information in there, but you can't rely on it).

I would rather vote for disabling logging by default at all.

I'm also fine with that, I guess 99% of the users don't even know that the logging exists and we are just silently wasting disk space there.

@benpicco
Copy link
Contributor Author

How about saving the logs to /tmp by default.
That way, if they are interesting they can easily be moved to a persistent location while not cluttering up the file system in the common case. (This is also a good way to handle the Downloads folder)

@OlegHahm
Copy link
Member

Well, my originally thinking was that enabling logging per default to a non-volatile location may be helpful in many cases where you only realize that you would have needed the log afterwards - potentially weeks later.

But probably the most reasonable option would be to re-enable the config file again, disable logging per default, provide documentation, and leave everything up to the user.

@benpicco
Copy link
Contributor Author

my originally thinking was that enabling logging per default to a non-volatile location may be helpful in many cases where you only realize that you would have needed the log afterwards - potentially weeks later.

But in that case individual log files per session would make more sense, otherwise you just have one gargantuan file that might contain the log you need if you are lucky.

@OlegHahm
Copy link
Member

That's true. I don't know if the behavior of Python's logging module has changed over the years or if I just thought about the use case of having to concurrent pyterms.

@benpicco
Copy link
Contributor Author

So how should we proceed?
with native now also using pyterm the default-run.log just keeps piling up faster.

@OlegHahm
Copy link
Member

I would say: disable logging per default in pyterm itself.

@benpicco benpicco added this pull request to the merge queue Jul 11, 2024
@benpicco
Copy link
Contributor Author

I think this is still useful. I'll do a follow-up to make logging off by default, but when it's enabled we should have separate files per session.

Merged via the queue into RIOT-OS:master with commit db21bd1 Jul 11, 2024
25 checks passed
@benpicco benpicco deleted the pyterm-session branch July 12, 2024 09:38
@mguetschow mguetschow added this to the Release 2024.07 milestone Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants