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

logging should only go to STDERR #183

Merged
merged 1 commit into from
Sep 20, 2021
Merged

logging should only go to STDERR #183

merged 1 commit into from
Sep 20, 2021

Conversation

kba
Copy link
Member

@kba kba commented Sep 2, 2021

Having output to be parsed on STDOUT and logging on STDERR consistenly will ensure that logging never interferes with parseable output, e.g. from ocrd-name-of-processor --dump-json or in bashlib-based calls.

@bertsky
Copy link
Collaborator

bertsky commented Sep 2, 2021

I'm in on that, but I'm also afraid this is still no guarantee against interference on stdout: processors might call legacy code which simply cannot be silenced or made to adhere to our logging conventions. (We could perhaps redirect all stdout to stderr, but then the question becomes: what is allowed on stdout actually?)

@kba
Copy link
Member Author

kba commented Sep 14, 2021

what is allowed on stdout actually?

I would say anything that is to be parsed by software, such as the --dump-json ocrd-tool.json or "one item per line" CSV output of ocrd workspace find.

@bertsky
Copy link
Collaborator

bertsky commented Sep 14, 2021

I would say anything that is to be parsed by software, such as the --dump-json ocrd-tool.json or "one item per line" CSV output of ocrd workspace find.

Agreed! So we could really redirect stdout to stderr, but prior to that, duplicate stdout to some dedicated FD, which we then use for printing/dumping the things which the command is actually made for, right?

So we still need to go through all output of the ocrd (sub)commands and the processor decorator, and decide which actually belongs to stdout...

@kba
Copy link
Member Author

kba commented Sep 20, 2021

So we still need to go through all output of the ocrd (sub)commands and the processor decorator, and decide which actually belongs to stdout...

Agreed, but AFAICS this should not prevent merging this and OCR-D/core#713 in the meantime, right?

@kba kba merged commit d8e2c62 into master Sep 20, 2021
@bertsky
Copy link
Collaborator

bertsky commented Sep 20, 2021

Agreed, but AFAICS this should not prevent merging this and OCR-D/core#713 in the meantime, right?

Not at all – just trying to figure out what this would mean for our implementation.

(I do find it adequate for a spec like this to underspecify what should or should not actually appear on stdout. It's not needed for interoperability, so why be too restrictive. But in our implementation we can try to be rigorous, and thus fix its potential stdout parsing problem once and for all.)

@kba kba deleted the log-to-stderr branch September 6, 2022 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants