gregorio-project / gregorio

The Gregorio Project
http://gregorio-project.github.io
Other
164 stars 43 forks source link

Kpsewhich error not appearing in log #1541

Open rpspringuel opened 3 years ago

rpspringuel commented 3 years ago

The following kpsewhich based error is sent to stderr but does not get added to the glog file that Gregorio itself generates:

154: in function `gregorio_out_name_ok': error:kpse prohibits write to file <gtex location>

This can make debugging difficult when auto compile (as this error doesn't end up in any log file). Is it possible for the executable to copy stderr messages to glog or should we add a redirect for stderr to the command call in the Lua code which executes the autocompile?

henryso commented 3 years ago

glog is currently being written by the lua code. The executable should not be writing to the same file as it will cause buffering to be messed up and the file to be corrupted.. If anything, the lua code should append stderr from the executable to the glog.

rpspringuel commented 3 years ago

I can make that change, but I’ll point out that we’re already passing the glow file to the executable using the -l argument. If we don’t want the executable to write to glog directly, then we’ll need to change that too.

✝️✝️✝️✝️✝️✝️✝️✝️✝️✝️✝️✝️✝️✝️✝️✝️✝️✝️✝️✝️✝️✝️✝️✝️ Fr. Samuel, OSB (R. Padraic Springuel) St. Anselm’s Abbey 4501 South Dakota Ave, NE Washington, DC, 20017 202-269-2300 (c) 202-853-7036

PAX ☧ ΧΡΙΣΤΟΣ

On May 23, 2021, at 10:19 PM, Henry So @.***> wrote:

 glog is currently being written by the lua code. The executable should not be writing to the same file as it will cause buffering to be messed up and the file to be corrupted.. If anything, the lua code should append stderr from the executable to the glog.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

henryso commented 3 years ago

Yeah, either that should change or the lua code needs to close the file before calling the executable.

rpspringuel commented 3 years ago

Is the glog file still needed after the executable is called (I haven't had a chance to check yet myself)? If it is, is it reasonable for the Lua code to close the file, call the executable, and then immediately reopen the file?

henryso commented 3 years ago

Yes, that's reasonable. The executable currently overwrites the -l file, so this would need to be changed for the behavior you want.

henryso commented 3 years ago

The executable should also be audited to ensure that it writes to said file instead of stderr once option parsing is completed.

rpspringuel commented 3 years ago

My computer has decided to go on the fritz again, so I can’t complete my analysis, but I think that support.c may have the offending line. It’s the only place where gregorio_set_error_file is called with stderr as hard coded argument.

Sent with GitHawk

henryso commented 3 years ago

It's set to that by default and then overridden by the -l argument. However, the -l argument causes it to open the specified in "wb" mode (write, binary), which truncates any existing file before writing. It needs to open in "ab" mode (append, binary; or perhaps just "a" mode). The y.c files write directly to stderr, so these need to change to use the gregorio_message stuff instead.

rpspringuel commented 3 years ago

What do you mean by “y.c”?

Sent with GitHawk

henryso commented 3 years ago

There are source files that end in y.c.

rpspringuel commented 3 years ago

I'll admit that C code is not my strong suit, but in looking through the code, it looks like the error message which I quoted earlier is already using the internal messaging commands which should direct output to the file specified by -l. The fact that it's going to stderr implies either that option processing isn't complete yet, or that switching from the default stderr to the -l file is happening well after option processing is done.

henryso commented 3 years ago

Like I said before, the -l flag is destroying the old contents of the file. I can take this issue on, but I'd like to clear up the open pull request (one way or the other) to minimize the merge conflicts of multiple branches.

henryso commented 3 years ago

@rpspringuel Please see my stderr-fix branch and let me know if that works for you.

rpspringuel commented 3 years ago

I can't find the instructions for building the executable with the kpsewhich restrictions in place, so the modified executables that I build aren't triggering the error (and thus make it impossible for me to test the log-file changes). @henryso do you have those instructions somewhere so that I can do some proper testing?

henryso commented 3 years ago

I think you would add --with-kpathsea when running configure.

rpspringuel commented 3 years ago

Heading on vacation today. I’ll give it a go when I get back (Friday).

Sent with GitHawk

rpspringuel commented 3 years ago

Working on this. Seems I need to do the testing on Ubuntu as there doesn't appear to be a simple way to get the Kpathsea library in a shared form on Mac.

henryso commented 2 years ago

Any movement on this?

rpspringuel commented 2 years ago

Totally forgot about this. Let me see if I can't get my test system updated and running these tests before the end of today.

rpspringuel commented 2 years ago

I'm working on this, but my virtual machine is being rather stubborn at the moment. Compilation steps are taking really long.

rpspringuel commented 2 years ago

Okay, here's what I get:

After successfully building Gregorio on your branch, I get an executable that returns the following:

$  src/gregorio-6_0_0 --version
Gregorio 6.0.0-stderr-fix-8ec1d64f-4524 (kpathsea version 6.3.1).
Copyright (C) 2006-2021 Gregorio Project authors (see CONTRIBUTORS.md)
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Which seems to confirm a correct build. However, the error is still present when I attempt to use it:

$ ~/Downloads/gregorio/src/gregorio-6_0_0  -o ../test.gtex gabc-score.gabc 
error:kpse prohibits write to file ../test.gtex

Should I do anything else to try and verify the executable further?

henryso commented 2 years ago

Well kpse is preventing that path correctly. So that build includes kpse. The thing now is to see if the logging works correctly with this version of the executable.

rpspringuel commented 2 years ago

Right. I’d completely forgotten what I was supposed to be testing. Now I need to put the score in a test document and look at the log. I’ll try that out in the morning.

Sent with GitHawk

rpspringuel commented 2 years ago

Okay, finally got the VM working at reasonable speed and there doesn't appear to be any change. The error shows up in stderr, but not the glog file.

henryso commented 2 years ago

Then I don't think there's a way to get that error into the glog file besides redirecting stderr from lua rather than using gregorio's error file feature. Looks like kpse is forcing it to print to stderr.