During the sprint last week, some discussions on how to implement this feature
were held. The main complication of this issue lies in how to execute the
system call so that it works on all supported platforms. One way to achieve
this would be to forward this responsibility to an external library, for
example Boost. The discussion on Discord ended with the conclusion that a less
robust but more lightweight solution is sufficient, especially because this fix
should only exist until the Python library is functional and should then be
removed.
Malte and I discussed what the implementation could look like in more detail.
The idea is to first check the value of the environment variable
DOWNWARD_PLAN_CALLBACK. If the variable is *not* set, we do nothing. Otherwise,
we call the executable the variable points to and provide the name of the newly
generated plan as an argument. For Windows, we use the system() to execute the
callback, and for Linux/MacOS we use the appropriate versions of the
fork/exec/wait functions. In particular we want to use execvp(), which takes
the arguments as an array instead of as a variadic argument. Additionally we
want that both platforms behave the same way with regards to whether they scan
the PATH to look for the executable, execvp() and system() both do so.
I tried to put this plan into practice in the following pull request:
https://github.com/aibasel/downward/pull/153
In the current implementation, DOWNWARD_PLAN_CALLBACK has to contain a relative
or absolute path to an executable. This means that setting the variable to
something like "python my_script.py" does not work. To achieve this, we can
instead just set it to "./my_script.py" on Unix (because the shebang is
interpreted), or write a batch script that calls the python program on Windows.
Regarding PATH, while it is searched on both platforms, on Windows the content
of DOWNWARD_PLAN_CALLBACK is double-quoted to allow whitespace in the path.
This prevents PATH commands from being recognized (the unquoted command type is
recognized by the command interpreter, but "type" is not).
I tested the functionality manually for Linux and Windows, but I did not verify
that things work the same way for Mac. I am not sure how thorough we want/need
to be given that this is only a temporary solution. Automated tests through
GitHub actions were also on the table, but I have not done anything in this
direction.
As a next step I would suggest that somebody takes a look at the pull request,
and to decide whether more thorough testing is needed. Another question would
be if we need to verify that the no-hook operation of the planner is not
affected by this change. The last task would be to write the merge commit
message, I will write a suggestion when the code is reviewed and ready to be
merged.
|