Add COB_SET_RUNTIME_STDOUT_FILE and COB_SET_RUNTIME_STDERR_FILE runtime options to enable redirection of stdout/stderr output - #296
Conversation
|
@GitMensch Here is the promised PR for stdout/stderr redirection. I opened it in draft for now to clarify a small issue in I also feel like implementing the redirection in some places seemed unnecessary to me, with the thought that they were meant for direct usage over the command line, rather than a C module interacting with GnuCOBOL. I would appreciate if you could check in general where it makes sense, or whether there are still places missing, where we need to the redirection. As always, always glad and available for further questions and discussions. Thanks for the support! |
|
I can confirm that the failing tests in CI (should be seven in total) are related to the PR, will investigate. |
|
All tests should be fixed now, the issue was |
|
Not sure what is going on with Windows CI tests, and the vbisam one, but I believe it is unrelated? |
GitMensch
left a comment
There was a problem hiding this comment.
Please also add those two as "normal" runtime settings COB_STDOUT_FILE and COB_STDERR_FILE with the common file handling (like + opening the file as append), this allows users to both set the filename within a COBOL thread via SET ENVIRONMENT as well as easily having one per PID (which would be your example).
Not sure, but... would it be reasonable to redirect stdin the same way?
| bdb_env->set_errfile (bdb_env, cobsetptr->cob_stderr); | ||
| #if (DB_VERSION_MAJOR > 4) || ((DB_VERSION_MAJOR == 4) && (DB_VERSION_MINOR > 2)) | ||
| bdb_env->set_msgfile (bdb_env, stderr); | ||
| bdb_env->set_msgfile (bdb_env, cobsetptr->cob_stderr); |
There was a problem hiding this comment.
Should this be re-set if the current thread changes the file* ?
There was a problem hiding this comment.
How would I trigger the refresh/re-set in cob_set_runtime_option in that case, by simply adding a non-static getter for bdb_env?
There was a problem hiding this comment.
the right way would be to do the same as in screenio - see cob_settings_screenio and its callers... but at least add one exported function into fileio.c that you can call when this specific runtime setting changed
There was a problem hiding this comment.
Done, let me know if it is missing something.
There was a problem hiding this comment.
the only question here is shouldn't the code be moved to the setting function (and that function either be called in the caller or inside this function)?
There was a problem hiding this comment.
That makes sense, I guess I was trying to be as uninvasive as possible, hence redundant stuff.
There was a problem hiding this comment.
I have moved the unused (probably deprecated) part of the ifdef to settings as well. My gut feeling was telling me they should stay together.
For stdin, I don't have a clue whether it would be useful, so you can decide. I think it wouldn't be too much of an effort. For adding them as normal runtime settings, how does "SET ENVIRONMENT" work right now for IO files? I thought GnuCOBOL already implements changing the stdout/stderr via |
|
If stdin is not a big effort, then please add it the same way, covering all internally used streams. We may even use it later for other tests. For the |
GitMensch
left a comment
There was a problem hiding this comment.
there seems to be one missing element used when calling libcob from cobc, but only on Win32 - I guess that's the place where the binary mode is set (unix_lf), as that will only happen this way on win32
That and the stderr_f assignment inlined should fix CI; adding stdin redirection + reassignment for BDB would then finalize the changes.
If you feel that you can't get the part done soon that works on those changes and are likely more often used by "normal" users (three matching runtime settings to assign the streams to a filename) then we can get this in with a follow-up PR or work by someone else.
Apart from TODOs I have commented on, some stylings and docs, all parts should be there now. I have changed the test slightly to make it Windows compatible, let's see if CI passes. |
| { | ||
| if (ambig_fallback) | ||
| { | ||
| /* TODO: Decide how to do stdout/stderr redirection here |
There was a problem hiding this comment.
What do you think about here? Solve it similarly by adding an exported init function?
There was a problem hiding this comment.
if that's the only place here: call into cob_get_runtime_option once to get the fp - if it is NULL use stderr
There was a problem hiding this comment.
There are two functions, but I think adding it to both doesn't harm too much.
There was a problem hiding this comment.
By the way, cob_get_runtime_option actually assumes settings is initialized, I have added an if check to bypass it for stdin/stdout/stderr. I can add the same logic to others to make it uniform among all runtime options.
|
Remaining failing CI tests should be hopefully unrelated. |
|
Updated docs/changelogs, will mark the PR non-draft, once all the todos are cleared. The one extra failed CI with Ubuntu+MSYS1 should actually pass (i.e. I haven't touched anything other than docs)? Instead, it got broken pipe in SIGPIPE test for some reason. |
GitMensch
left a comment
There was a problem hiding this comment.
ci failures unrelated, most important adjustment is the handling of the setting of the runtime option - the current version won't work if those come from SET ENVIRONMENT (explicit setting in the COBOL main instead of runtime config file / environment)
| bdb_env->set_errfile (bdb_env, cobsetptr->cob_stderr); | ||
| #if (DB_VERSION_MAJOR > 4) || ((DB_VERSION_MAJOR == 4) && (DB_VERSION_MINOR > 2)) | ||
| bdb_env->set_msgfile (bdb_env, stderr); | ||
| bdb_env->set_msgfile (bdb_env, cobsetptr->cob_stderr); |
There was a problem hiding this comment.
the only question here is shouldn't the code be moved to the setting function (and that function either be called in the caller or inside this function)?
| if (cobsetptr->cob_stdin_filename) { | ||
| if (!cobsetptr->cob_unix_lf) { | ||
| cobsetptr->cob_stdin = | ||
| fopen (cobsetptr->cob_stdin_filename, "r"); | ||
| } else { | ||
| cobsetptr->cob_stdin = | ||
| fopen (cobsetptr->cob_stdin_filename, "rb"); | ||
| } | ||
| if (!cobsetptr->cob_stdin) { | ||
| cobsetptr->cob_stdin_filename = NULL; | ||
| cobsetptr->cob_stdin = stdin; | ||
| } | ||
| } | ||
|
|
||
| if (cobsetptr->cob_stdout_filename) { | ||
| cobsetptr->cob_stdout = | ||
| cob_open_logfile (cobsetptr->cob_stdout_filename); | ||
| if (!cobsetptr->cob_stdout) { | ||
| cobsetptr->cob_stdout_filename = NULL; | ||
| cobsetptr->cob_stdout = stdout; | ||
| } | ||
| } | ||
|
|
||
| if (cobsetptr->cob_stderr_filename) { | ||
| cobsetptr->cob_stderr = | ||
| cob_open_logfile (cobsetptr->cob_stderr_filename); | ||
| if (!cobsetptr->cob_stderr) { | ||
| cobsetptr->cob_stderr_filename = NULL; | ||
| cobsetptr->cob_stderr = stderr; | ||
| } | ||
| } |
There was a problem hiding this comment.
that part needs to be moved (maybe to termio.c?) and called when one of those runtime settings are changed - and in that case close the old pointer, before opening it again (if the pointer changed and the content differs)
There was a problem hiding this comment.
Moved it, see comment at the bottom.
Makes sense, I will take a look. |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## gitside-gnucobol-3.x #296 +/- ##
=======================================================
Coverage ? 67.42%
=======================================================
Files ? 34
Lines ? 61700
Branches ? 16091
=======================================================
Hits ? 41599
Misses ? 14112
Partials ? 5989 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I think SET ENVIRONMENT should work now, added a test for it. It is kind of messy how I do it in cobsetptr, i.e. having There is also the Otherwise, from my side all that is missing is what we should with |
|
@oguzcankirmemis please rebase, which should improve the CI results |
e407b35 to
d573e9c
Compare
|
@GitMensch rebase done, I think the failing two tests in Windows are due to runtime mismatch between libcob and the running tests (similar to CBL_GC_HOSTED). The error doesn't make sense to me really otherwise. Should we try to fix it, or ignore the test in windows debug builds? You probably already looked into it, but do you know a fast way to experiment/verify with Windows builds, i.e. some ready to use image or machine I can connect to? Then, I can take a look to make the libraries use the same c runtime maybe. It should be helpful for future tests. |
|
@GitMensch I fixed the Windows CI issues with a little help of AI, i.e. it found the A minor consequence for now is that the debug builds via cobc always uses Let me know if there are other consequences we need to think about for the fix. |
|
For curiosity, I also asked AI to decouple CRT selection from the -g flag, effectively adding another variable on top of cb_source_debugging. Using cb_source_debugging adds more than the CRT flag as mentioned, hence the reason. I can push it if you find this idea cleaner. |
|
Yes, CRT selection should be independent from the optimization flags. To not mix/match your MSVC CI fixes (and - very nice - remove some removement of failing tests) with the rest of this PR: please create a separate one (which then @ddeclerck can also merge here + upstream, while I inspect the "real PR = new feature"). |
|
As the MSVC debug builds break in the single test for this PR.. there seems something is actually broken in the implementation, no? |
I am confused, the CI fix also fixes this issue, no? |
Gotcha, I will revert the fix here once I opened the other PR. |
hm, now I see all passing... not sure what I've seen before, guess that was an error from "it is a file pointer, but not from this runtime" |
GitMensch
left a comment
There was a problem hiding this comment.
We're nearly done on this PR. Thanks for your efforts!
Some notes and mostly minor things to adjust in the review below.
| * common.h (cob_runtime_option_switch): add COB_SET_RUNTIME_STDIN_FILE, | ||
| COB_SET_RUNTIME_STDOUT_FILE and COB_SET_RUNTIME_STDERR_FILE enums | ||
| * common.c (cob_set_runtime_option, cob_get_runtime_option): extend for | ||
| COB_SET_RUNTIME_STDIN_FILE, COB_SET_RUNTIME_STDOUT_FILE and | ||
| COB_SET_RUNTIME_STDERR_FILE options |
There was a problem hiding this comment.
you can combine those two into a single one
| void * | ||
| cob_get_runtime_option (enum cob_runtime_option_switch opt) | ||
| { | ||
| switch (opt) { |
There was a problem hiding this comment.
please add a check to directly return NULL if cobsetptr is not available (= no init done yet)
| { | ||
| if (ambig_fallback) | ||
| { | ||
| /* TODO: Decide how to do stdout/stderr redirection here |
There was a problem hiding this comment.
if that's the only place here: call into cob_get_runtime_option once to get the fp - if it is NULL use stderr
|
|
||
| (void)_setmode (_fileno (stdin_f), _O_BINARY); | ||
| (void)_setmode (_fileno (stdout_f), _O_BINARY); | ||
| (void)_setmode (_fileno (stderr_f), _O_BINARY); |
There was a problem hiding this comment.
Not sure: wouldn't we need that in the termio inits after opening the file?
Maybe move that whole part there?
There was a problem hiding this comment.
Yes, I think we need it also in termio. I would suggest that aside from setting stdout/stderr/stdin in cob_init, no further initialization should be done at common, and the rest should happen in termio.
I find even setting the initials in cobsetptr in common is uglyish, but I guess it is an ok compromise unless we we want to introduce a different init routine in termio just for the sake of redirection.
There was a problem hiding this comment.
This ties back to your review in termio with what happens if the string is null/empty etc.: It should ignore and simply work with whatever set in the cobsetptr, which would be the process stdin/stdout/stderr, and do the configuration for it.
There was a problem hiding this comment.
Ok, moving the whole part there sounds problematic, because cob_init_termio expects that settings is initialized, whereas the whole point of cob_common_init is to leave settings unintialized. But as you said, we also need this logic in the termio. Since calling _setmode twice with the same parameter shouldn't hurt, for now, I would just duplicate the relevant part and also put a comment in cob_common_init hinting at the issue.
| void | ||
| cob_set_runtime_option (enum cob_runtime_option_switch opt, void *p) | ||
| { | ||
| switch (opt) { |
There was a problem hiding this comment.
Please add an explicit check for cobsetptr here and call cob_fatal_error (COB_FERROR_INITIALIZED) if it isn't set.
| AT_CLEANUP | ||
|
|
||
|
|
||
| AT_SETUP([stdin/stdout/stderr redirection]) |
There was a problem hiding this comment.
Just seen that the first time after you've added stdin.
That test is super clever and I like it - the only possible issue: if the stdin part does not work (what we want to ensure with this test) this test would hang forever, no?
But that can possibly apply to every test, especially file related... so leaving it in is a good way to catch if that does not work :-)
There was a problem hiding this comment.
How about running the test with a timeout?, simply like AT_CHECK([timeout 10s $COBCRUN_DIRECT ./prog], [0], [], [])
There was a problem hiding this comment.
I would even suggest adding timeout to both, the other one can also hang.
There was a problem hiding this comment.
I had to revert timeout, apparently it does not come pre-installed in MacOS. Disappointing really, it should be part of the standard posix tools if I am not misunderstanding. There is a perl alternative we can probably use, but it looks ugly to me. Or one can install coreutils in Mac and alias gtimeout with timeout, if we want to go there.
There was a problem hiding this comment.
not sure about the portability of that... but at least on GNU/Linux and one checked Solaris that's available.
| ** new runtime configurations: COB_STDIN_FILE, COB_STDOUT_FILE, | ||
| COB_STDERR_FILE. They allow redirection of stdin/stdout/stderr input/output | ||
| to a file. Alternatively, these configurations are also available as runtime | ||
| options which can be used to set them programmatically: |
There was a problem hiding this comment.
to set them via configuration file, environment variables, or programatically
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
- introduces following runtime configurations: COB_STDIN_FILENAME, COB_STDOUT_FILENAME, COB_STDERR_FILENAME Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Introduce COB_STDIN, COB_STDOUT, COB_STDERR, COB_STDERR_OR_DEFAULT Add timeout to redirection tests Solve cobgetopt.c with cob_get_runtime_option Refactor termio for better edge case handling Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
Signed-off-by: Oguzcan Kirmemis <oguzcan.kirmemis@gmail.com>
26c80d9 to
d7c9dc5
Compare
|
Rebased after the merge of #300 |
GitMensch
left a comment
There was a problem hiding this comment.
LGTM - thanks for your work on this, ready for upstream when the CI passes
If neither @lefessan nor @ddeclerck get to commit that upstream, then I'll do so not later than this weekend.
| if (data_loc == offsetof (cob_settings, cob_stdin_filename) | ||
| || data_loc == offsetof (cob_settings, cob_stdout_filename) | ||
| || data_loc == offsetof (cob_settings, cob_stderr_filename)) { |
There was a problem hiding this comment.
| if (data_loc == offsetof (cob_settings, cob_stdin_filename) | |
| || data_loc == offsetof (cob_settings, cob_stdout_filename) | |
| || data_loc == offsetof (cob_settings, cob_stderr_filename)) { | |
| if (data_loc == offsetof (cob_settings, cob_stdin_filename) | |
| || data_loc == offsetof (cob_settings, cob_stdout_filename) | |
| || data_loc == offsetof (cob_settings, cob_stderr_filename)) { |
minor formatting, can also be applied during upstream commit
| if (cobsetptr->cob_stdin_filename_set && | ||
| strcmp (cobsetptr->cob_stdin_filename, | ||
| cobsetptr->cob_stdin_filename_set) == 0) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
| if (cobsetptr->cob_stdin_filename_set && | |
| strcmp (cobsetptr->cob_stdin_filename, | |
| cobsetptr->cob_stdin_filename_set) == 0) { | |
| return; | |
| } | |
| if (cobsetptr->cob_stdin_filename_set | |
| && strcmp (cobsetptr->cob_stdin_filename, | |
| cobsetptr->cob_stdin_filename_set) == 0) { | |
| return; | |
| } |
minor formatting here and similar below in the new code, can also be done during usptream commit
This PR adds runtime options
COB_SET_RUNTIME_STDOUT_FILEandCOB_SET_RUNTIME_STDERR_FILEwhich allows control over the location where the stdout/stderr output is written.Motivation
For modules written in C that interact with libcob, it is currently difficult to change the location of stdout/stderr output without disturbing the stdout/stderr of the whole process (i.e., other threads). This is especially useful for thread applications, where it is desired that each thread gets its own stdout/stderr without disturbing the one another.
With these options, the main program can easily redirect the output of the COBOL program (including the messages from the runtime itself) and decide what to do with it. For that reason, the main program is also responsible for managing the lifetime of the passed stdout/stderr files, should they be used.
Implementation
COB_SET_RUNTIME_STDOUT_FILEandCOB_SET_RUNTIME_STDERR_FILEoptions tocob_runtime_option_switchincommon.h.cob_settingsincoblocal.hto store the location of configuredstdout/stderr. By default, they are initialized to stdout/stderr of the process.cob_set_runtime_optionandcob_get_runtime_optionincommon.c../libcob, so that they behave accordingly.doc/gnucobol.texi.