Simplify spawn's logfile closing from using globals to other projects

[pevogam]

Arbitrary functions can be used as closing hooks for the spawns and we only need to provide a minimal logfile closing hook and functionality letting the caller design their own hooks of any further complexity.

[ldoktor]

Changing the param-name is always tricky, let's hope nobody calls this with named arguments (given there used to be one I think it's relatively safe).

As for the open_fd, do you actually have a use for this somewhere? IIUC avocado-vt handles the opening/closing of these internally so I don't think we should allow this capability (unless it's required somewhere).

[ldoktor]

I mean this feature (open_fd) was not present previously so why adding it now?

[pevogam]

It was added indeed in order to still have some sensible _close_log_file to be added as a first hook and change less of the previous behavior where we would assume aexpect provides some means to close a log file tracked by the self.log_file attribute. Without this both the self.log_file string as well as the self._close_log_file() and initial close hook setup become meaningless and we change too much externally while some minimal support for an actual standalone log file for aexpect still makes sense.

[ldoktor]

My point of view is that this feature was part of the genio ugly module and was used by avocado-vt. Now things are moving to avocado-vt so I'd prefer leaving the extra bits only there. The interface looks cleaner to me when you hand over an open FD you should close it.

Anyway it's not nack, just something to consider. I won't ask for this the second time so if you think it's ready I'm ready to merge it (probably after the avocado-vt change so the main+main still works)

[pevogam]

The interface looks cleaner to me when you hand over an open FD you should close it.

Perhaps the above is not clear: open_fd is a boolean flag here so for sure we don't pass open fd that aexpect then closes, the fd is both opened and closed only by aexpect. This behavior is now completely separate from Avocado VT and is simply there so that we cut the redundant part but don't cut too much and too deep.

One drawback I can point out myself: we don't read from or write into the open fd just like we don't make much more use of self.log_file beyond the fd itself. This all has to do with the complexity caused by passing to sessions both log files and log functions where the functions are the main means of using any fd and log_file attribute value. What I would recommend therefore is that we perform the current incision minimizing changes in the way I recommended above (not removing too many lines and changing too much of the standard close hooks and API) and simply revisit the "how to log to a file vs via a function with aexpect" as a separate topic that requires us to consider how to simplify the issue with "unused attributes that we still need in some occasions".

[pevogam]

Minimizes changes means that if you take the number added and removed lines, that total will be minimal. Conversely, if you calculate how many lines we didn't have to change in order to achieve the above separation this is maximized. You are always free to try and go ahead with your own suggestion and note down how many other lines will have to be added or dropped if you don't add this tiny feature.

[ldoktor]

You mean you want to keep the same amount of lines even though that means unused (therefore dead) code in? What I have in mind is this:

diff --git a/aexpect/client.py b/aexpect/client.py
index 696f7dc..f846655 100644
--- a/aexpect/client.py
+++ b/aexpect/client.py
@@ -527,7 +527,6 @@ class Tail(Spawn):
         # Add a reader and a close hook
         self._add_reader("tail")
         self._add_close_hook(Tail._join_thread)
-        self._add_close_hook(Tail._close_log_file)
 
         # Init the superclass
         super().__init__(command, a_id, auto_close, echo, linesep,
@@ -610,20 +609,13 @@ class Tail(Spawn):
         """
         self.output_prefix = output_prefix
 
-    def set_log_file(self, filepath, open_fd=False):
+    def set_log_file(self, filepath):
         """
         Set a log file name for this tail instance.
 
         :param filepath: complete file name and path of the log.
-        :param open_fd: whether to also open the log file
         """
         self.log_file = filepath
-        if open_fd:
-            self.log_file_fd = open(filepath, encoding="utf-8")  # pylint: disable=R1732
-
-    def _close_log_file(self):
-        if self.log_file_fd is not None:
-            self.log_file_fd.close()
 
     def _tail(self):  # speed optimization pylint: disable=too-many-branches,too-many-statements

which, unless I'm mistaken, should not lead to any troubles on avocado-vt side, results in less lines of code in aexpect and less branching/code-complexity.

[pevogam]

I results in fewer lines of code but more changed lines overall (as I mentioned above), it changes more of the original behavior increasing the probability of problems down the line, and finally it renders the use of self.log_file mute, meaning that his is just some string attribute we set and never use anywhere within the aexpect API.

[ldoktor]

Keeping the same amount of code doesn't mean less changes. And here it actually introduces new concept (opening and closing log_file_fd) that was not previously present and it is not even used (that is my main concern).

Anyway you are right that the self.log_file is not actually used anywhere so looking at it now I think we should get rid of the set_log_file as well as self.log_file completely. In avocado-vt I can see 2 usages and we are always manually extending the close_hooks, so we can simply pass a generated function by using:

self.logsessions[key].close_hooks += [lambda self: utils_logfile.close_log_file(outfile)]

instead of the original:

self.logsessions[key].set_log_file(outfile)
self.logsessions[key].close_hooks += [utils_logfile.close_own_log_file]

I mean we do have the output_func that is properly handled inside aexpect, having a log_file is redundant and apparently not used (at least not handled inside there).

[pevogam]

Keeping the same amount of code doesn't mean less changes. And here it actually introduces new concept (opening and closing log_file_fd) that was not previously present and it is not even used (that is my main concern).

This sounds ambiguous while I prefer to be precise here: what I mean by amount of changes is number of added and removed lines. It is easier to quantify and I mentioned it first here. Everything else is subjective and relative to the point of view. It still checks out in terms of your measure regarding "introducing new concepts" where removing the extra lines you suggested will cause us to consider the self.log_file and other additional functionality and potentially change more concepts (=dropping such still implies changing them which still implies changing long established behavior leading to potential breaking of client code).

Anyway you are right that the self.log_file is not actually used anywhere so looking at it now I think we should get rid of the set_log_file as well as self.log_file completely. In avocado-vt I can see 2 usages and we are always manually extending the close_hooks, so we can simply pass a generated function by using:

self.logsessions[key].close_hooks += [lambda self: utils_logfile.close_log_file(outfile)]

instead of the original:

self.logsessions[key].set_log_file(outfile)
self.logsessions[key].close_hooks += [utils_logfile.close_own_log_file]

I mean we do have the output_func that is properly handled inside aexpect, having a log_file is redundant and apparently not used (at least not handled inside there).

Exactly, I suggested something along these lines here with the idea of aexpect only exposing a single output_func as interface on any kind of logging. My worry however is not about Avocado VT where we can track number of use cases and correct them right away but any other potential objects that make use of logging and expect certain behavior that we deviate more from with more changes (measured as changed lines).

[ldoktor]

Thank you, @pevogam, this cleans the aexpect. Apart of the new open_fd interface this makes sense to me. Anyway if you have a good usecase for it, let me know.

[ldoktor]

Hello @pevogam, there were few ping-poings, how do you intend to approach this PR in order to reach conclusion? In general I do like it, the only issue for me is the addition of the open_fd. I haven't seen any usage of it therefore I don't think it should be included. If someone opens an issue, we can revisit it but unless there is a need of it, I'd like if you could skip it.

[pevogam]

Hi @ldoktor right now I am waiting for the closure of the pull request on the VT side and then can revisit all these topics. As @luckyh also added his opinion about the way to go forward here, I think it will be best to at least add a separate commit to drop the self.logfile functionality entirely and offer a single self.log_func as I wrote in #124 (comment). This would then wrap up everybody's concern and we will have improved aexpect API in addition to improved avocado-vt usage.

[ldoktor]

OK, I was afraid I was blocking you somehow. Note I have nothing against the unified log_func approach.

[pevogam]

OK, I was afraid I was blocking you somehow. Note I have nothing against the unified log_func approach.

No that's fine, I just hope it is also ok to sometimes contribute a change that is not complete in the views of maintainers but still a step forward passing all available tests and fully usable within the library since this is all the resource I could spare at the time.

Further improvements are always welcome on both sides but best taken a separate steps that can be easily tracked with GH issues. I would likely quantize changes in a similar manner like you if I was the aexpect maintainer (it is normal for maintainers to want the most and the best for their projects) but as a contributor I am often limited to the smallest set of changes and effort to achieve something (but of course not increase the technical debt whenever possible). As the current set of changes is already self-contained and results in net decrease of complexity (less coupling with other projects like this) and thus maintenance costs, I would prefer to take the shorter step here.

Nevertheless, as we are already more or less clear on the step forward to unify output functionality let's just go that route here once the VT pull request gets merged.

[pevogam]

Not sure if the CI is functioning right now so perhaps I will rebase this once we merged the CI branches and rerun.

[ldoktor]

The CI works well now. While on rebase would you please also remove the unused open_fd code or reference here where it's being needed?

[pevogam]

The CI works well now. While on rebase would you please also remove the unused open_fd code or reference here where it's being needed?

Do you still see such code? Because I have already pushed today with the final changes and only wrote about why I believe the CI fails.

[pevogam]

The CI passes now and this is the final version of the proposed branch, let me know if you have any further comments.

[ldoktor]

Thank you for this simplification (especially the avocado-vt part)

[ldoktor]

The CI passes now and this is the final version of the proposed branch, let me know if you have any further comments.

Ahh, you are right. It gets introduced and removed immediately. Let me merge this and in case some projects needs this we can always bring it back in a cleaner form.