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

Merge stdout and stderr to see errors easier #8

Merged

Conversation

PROger4ever
Copy link
Contributor

@PROger4ever PROger4ever commented Jul 31, 2020

Problem

When exiftool reports about errors, it prints them to stderr. But go-exiftool pipes only stdout, so there's no way to find out what happened.

Solution

Let's merge stdout and stderr:

cmd := exec.Command(binary, initArgs...)
r, w := io.Pipe()
e.stdMergedOut = r

cmd.Stdout = w
cmd.Stderr = w

var err error
if e.stdin, err = cmd.StdinPipe(); err != nil {
  return nil, fmt.Errorf("error when piping stdin: %w", err)
}

e.scanMergedOut = bufio.NewScanner(r)
e.scanMergedOut.Split(splitReadyToken)

It allows us to see error output when unmarshalling json:

if err := json.Unmarshal(e.scanMergedOut.Bytes(), &m); err != nil {
  fms[i].Err = fmt.Errorf("error during unmarshaling (%v): %w)", e.scanMergedOut.Bytes(), err)
  continue
}

Warning

Closing and freeing resources statements should be reviewed after me...

@barasher
Copy link
Owner

Could you please provide me a picture that makes ExifTool (CLI) raise an error on stderr that is not a "file not found" ?

I've edited a JPG file (altering random bytes):

barasher@linux:/tmp$ exiftool -stay_open True -@ - -common_args > /tmp/blastdout 2> /tmp/blastderr
-j
/tmp/20190404_131804.jpg
-execute

Nothing is goes out on stderr, bu the following appears on stdout:

barasher@linux:/tmp$ tail -f /tmp/blastdout 
ExifTool Version Number         : 10.80
File Name                       : 20190404_131804.jpg
Directory                       : /tmp
File Size                       : 47 kB
File Modification Date/Time     : 2020:08:11 22:33:31+02:00
File Access Date/Time           : 2020:08:11 22:33:31+02:00
File Inode Change Date/Time     : 2020:08:11 22:33:31+02:00
File Permissions                : rw-rw-r--
Error                           : File format error

@barasher
Copy link
Owner

The only error that I've been able to pop on stderr is a non existing file:

barasher@linux:/tmp$ tail -f /tmp/blastderr 
File not found: /tmp/nonexisting.jpg

This is checked in the go code (before invoking exiftool)

@PROger4ever
Copy link
Contributor Author

PROger4ever commented Aug 13, 2020

Yes, the only one error I got from exiftool is "file not found" too.

But when I got a stuck on Windows because of #7 for the first time, I couldn't understand the reason of it.
When I run exiftool on Windows directly, it displays filename with encoding problems in any terminal (cmd, PowerShell, GoLand's), but works:

C:\Users\PROger4ever>exiftool "E:\temp\Русское название\existing file.jpg"
ExifTool Version Number         : 12.03
File Name                       : existing file.jpg
Directory                       : E:/temp/╨єёёъюх эрчтрэшх
Warning                         : FileName encoding not specified
...

I'm newbie in exiftool, so I suggested that the reason of the stuck is the encoding problem in Windows and tried to tune the parameter -charset filename=CHARSET. But I couldn't even see the possible error from exiftool in my program, when it can't find the file because of wrong encoding...

Of course, after debugging go-exiftool line-to-line for few ... eternities, I found the reason in function splitReadyToken that couldn't find the token even if it was there.

The ability to see the full output from exiftool could save a lot of time on debugging.

I think it's a must have feature.

@barasher
Copy link
Owner

I totally agree with #7 : I already merged your pull request and I thank you once more.

Let's just discuss this pull request: the stdout and stderr merge.

go-exiftool does not check what happens on stderr, that's right. It would be cleaner to check it instead of considering that the only thing that could appear on stderr is a "file not found" that it is already check in the go code.

To do it properly, I don't think that merging stdout and stderr is the right way to do it.
With your implementation, what would happen if exiftool writes a warning on stderr and then, for the same extraction, a correct JSON on stdout ? There would be an error (unparsable JSON).
I think that stderr should be interpreted "alone", independently from stdout.

@PROger4ever
Copy link
Contributor Author

I agree that we should analyze stderr and stdout separately (a perfect case). But It takes much more effort to change the code...
I think, my solution is a golden mean, that at least doesn't ignore stderr and makes us to know that something is send to stderr.

go-exiftool runs exiftool in -stay_open mode with json-format. So warning is just a field of json:

C:\Users\PROger4ever>exiftool "E:\temp\Русское название\existing file.jpg" -j
[{
  "SourceFile": "E:/temp/╨єёёъюх эрчтрэшх/existing file.jpg",
  "ExifToolVersion": 12.03,
  "FileName": "existing file.jpg",
  "Directory": "E:/temp/╨єёёъюх эрчтрэшх",
  "Warning": "FileName encoding not specified"
  // ...
}]

It should be safe.

If I still haven't been able to convince you this moment, I agree to close the PR :)

@barasher
Copy link
Owner

You've convinced me that stderr should be taken care of.

I unfortunately do not have time to do it right now the way I'd like it to be done: I'll make some tests with your implementation and if everything's fine, I'll merge it. I'll add an issue to adapt this later.

So, do not close the PR.

@barasher barasher merged commit 4e36c0f into barasher:master Aug 16, 2020
@barasher barasher modified the milestones: v1.2.0, v1.1.3 Aug 16, 2020
@PROger4ever PROger4ever deleted the merge_stdout_and_stderr branch September 16, 2020 02:21
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