GNOME Bugzilla – Bug 742973
validate: launcher: Fix log file creation when redirecting to stdout
Last modified: 2015-02-17 16:26:01 UTC
Currently gst-validate-launcher misuses the --logsdir parameter to redirect log files to stdout/stderr. This causes gst-validate-launcher to incorrectly create an stdout/stderr directory to write the xunit.xml file, and also involves creating temporary directories for extra log files. The attached patches partially revert commit 20c28de, introduce a new parameter to specify log file redirection, and always create the normal logs directory, no longer using temporary directories.
Created attachment 294599 [details] [review] [PATCH 1/2] validate: launcher: Introduce new parameter for log file redirecting
Created attachment 294600 [details] [review] [PATCH 2/2] validate: launcher: Always create log files
Review of attachment 294599 [details] [review]: Looks generally good, just a few simple comments. ::: validate/launcher/baseclasses.py @@ +349,3 @@ def clean(self, full=True): Test.clean(self, full=full) + if hasattr(self, 'validatelogs') and not self.options.redirect_logs: You should make sure the attribute is set instead of checking of the attribute exists. ::: validate/launcher/main.py @@ +251,3 @@ if not os.path.exists(self.dest): os.makedirs(self.dest) + if not os.path.exists(self.logsdir): Where was it done before? @@ +408,3 @@ help="Directory where to store logs and rendered files. Default is MAIN_DIR") dir_group.add_argument("-l", "--logs-dir", dest="logsdir", + help="Directory where to store logs, default is OUTPUT_DIR/logs.") I agree it is better to have a dedicated parametter but I think we should still accept -l stdout/stderr here, so we keep the same user interface, removing it from the doc is still good.
Review of attachment 294600 [details] [review]: OK, that cleans up the code and is better :) But we should really start a testsuite for the launcher! :) ::: validate/launcher/baseclasses.py @@ +340,3 @@ + def clean(self): + Test.clean(self) + self._sent_eos_pos = None Why don't we need the full parameter anymore?
Review of attachment 294599 [details] [review]: ::: validate/launcher/baseclasses.py @@ +349,3 @@ def clean(self, full=True): Test.clean(self, full=full) + if hasattr(self, 'validatelogs') and not self.options.redirect_logs: The previous check worked by chance, because at the first time this function is called, the log file hadn't been opened in the reporter, so the check for uses_standard_output() would return false. validatelogs is set in an unrelated part of the code. The check with hasattr() is not elegant, but it is removed by the next patch, which no longer creates temporary directories (and therefore no longer needs to remove temporary files). I don't think it's worth fixing the other issue just to have it removed by the next patch. ::: validate/launcher/main.py @@ +251,3 @@ if not os.path.exists(self.dest): os.makedirs(self.dest) + if not os.path.exists(self.logsdir): In _TestsLauncher:set_settings(). The check there can be removed, thanks for spotting. When using -l stdout, a directory named stdout would be created to output some log files. @@ +408,3 @@ help="Directory where to store logs and rendered files. Default is MAIN_DIR") dir_group.add_argument("-l", "--logs-dir", dest="logsdir", + help="Directory where to store logs, default is OUTPUT_DIR/logs.") Ok, so what about keeping -l stdout/stderr, but just using that information to set redirect_logs internally, and using the default logsdir again?
Review of attachment 294600 [details] [review]: ::: validate/launcher/baseclasses.py @@ +340,3 @@ + def clean(self): + Test.clean(self) + self._sent_eos_pos = None When the full parameter was added, the behaviour of clean() changed from from just setting _sent_eos_pos to setting _sent_eos_pos and removing the temporary file. full=True would do both, full=False would just remove the file. Now that the file is no longer needed, we can go back to the previous behaviour.
Review of attachment 294599 [details] [review]: ::: validate/launcher/baseclasses.py @@ +349,3 @@ def clean(self, full=True): Test.clean(self, full=full) + if hasattr(self, 'validatelogs') and not self.options.redirect_logs: Ah ok, nevermind then. ::: validate/launcher/main.py @@ +408,3 @@ help="Directory where to store logs and rendered files. Default is MAIN_DIR") dir_group.add_argument("-l", "--logs-dir", dest="logsdir", + help="Directory where to store logs, default is OUTPUT_DIR/logs.") Yes, that is what I had in mind.
Review of attachment 294600 [details] [review]: ::: validate/launcher/baseclasses.py @@ +340,3 @@ + def clean(self): + Test.clean(self) + self._sent_eos_pos = None OK
Created attachment 294609 [details] [review] [PATCH 1/2] validate: launcher: Introduce new parameter for log file redirecting
Created attachment 294610 [details] [review] [PATCH 2/2] validate: launcher: Always create log files
commit 9c3606a86787ebe9c69a284d87c4d38d055008fc Author: Ramiro Polla <ramiro.polla@collabora.co.uk> Date: Thu Jan 15 15:32:12 2015 +0100 validate: launcher: Always create log files Create log files even when stdout redirection is enabled. This commit partially reverts 20c28de. https://bugzilla.gnome.org/show_bug.cgi?id=742973 commit 4e0388c6313d412f48e01f6c917e54e080c306c1 Author: Ramiro Polla <ramiro.polla@collabora.co.uk> Date: Thu Jan 15 15:26:14 2015 +0100 validate: launcher: Introduce new parameter for log file redirecting Allow log file redirection through the new --redirect-logs parameter. Keep the old --logs-dir stdout/stderr parameter, but reset to the default logs directory in that case, and set redirect_logs internally. This also prevents the creation of an stdout/stderr directory for writing xunit.xml. https://bugzilla.gnome.org/show_bug.cgi?id=742973