-
Notifications
You must be signed in to change notification settings - Fork 0
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
Initial steps to modernizing task_schedule #15
Conversation
if (not $_->{cmd} =~ m|\A \s* /|x and $opt{bin_dir}) { | ||
$_->{cmd} = "$opt{bin_dir}/$_->{cmd}"; | ||
} | ||
my ($bin_dir_cmd, @cmd_args) = split(" ", "$opt{bin_dir}/$_->{cmd}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are @cmd_args, where do they go? I don't see that handled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... cripes. I just came to the same conclusion the hard way by testing and looking at logs in debug mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, sorry I missed that in my review. Though I note it seems to be working fine for arc which I thought was a stressing case (though maybe it doesn't use this bin_dir section).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clearly the test coverage is inadequate here to have caught this. And of course in Python my editor would have complained that the new variable is never used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arc3 uses a better idiom, calling out the path instead of magically finding it in share.
exec 10 : $ENV{SKA_SHARE}/arc3/get_iFOT_events.pl
exec 5 : $ENV{SKA_SHARE}/arc3/get_web_content.pl
Zen of Python no. 2: Explicit is better than implicit!
} | ||
my ($bin_dir_cmd, @cmd_args) = split(" ", "$opt{bin_dir}/$_->{cmd}"); | ||
if (-e $bin_dir_cmd and -x $bin_dir_cmd) { | ||
$_->{cmd} = $bin_dir_cmd; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So should this just be
$_->{cmd} = $bin_dir_cmd + join(" ", @cmd_args);
Or does that need different error checking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be
$_->{cmd} = $bin_dir_cmd . " " . join(" ", @cmd_args);
Cuts the cruft that no longer applies and makes this work on a standalone MacOS platform.
Substantive code changes are:
SKA_ARCH_OS
env var is always defined, even in a (non-production) conda env.bin_dir
. ONLY if it exists and is executable then prependbin_dir
to the executable path. This was previously done regardless of whether the executable was found inbin_dir
. This new logic allows specifying an executable that is in the global searchPATH
. This is more common now with the use of entry point scripts, e.g.cheta_sync
orkadi_update_cmds
.This takes a different approach to unit testing and just installs locally in a fixed directory, then overrides
SKA
. For these testsSKA
is the root for bothdata
andbin
.Testing
Created a testing Ska3 environment on linux and MacOS
task_schedule self-test
Then from the task_schedule git repo, each of the following gave expected results
Note that
make test
had problems because it seemed to start theexception
test before thebasic
test was done since this forks off processes.Test with kadi 4.19 (
py3-cmds-task
branch) on linux and MacOSExpected results:
loud=1
)${CONDA_PREFIX}/data/kadi/logs/kadi_cmds.log