Hello Magnus,
This looks really nice! The only thing I would wish for would be that
you took some of the documentation you provided in this review mail and
added it to Execute.gmk.
Concerning the future "chmod" post commands, I think those should just
be add as something like " ; chmod $$@ " to COMMAND. The way the post
command is defined now, it's a separate rule. A simple chmod really
should be done in the same recipe as is creating the file.
/Erik
On 2019-03-01 05:24, Magnus Ihse Bursie wrote:
The most important operations performed by the build system is
generalized into various Setup functionality, which forms the API
on top of which the rest of the build system is erected.
However, we have a long tail with various small operations, calling a
specific build tool, etc, that are done on a more ad-hoc basis. Over
the years, a more and more refined set of "best practice" rules has
emerged for such operations, but they tend to be implemented only for
new operations, and not retrospectively updated on old code.
To make sure we always keep all such operations up to date with modern
robustness solutions, I propose to create a new, more general,
function, SetupExecute. Basically, this takes a command line to
execute, and make sure this is done The Right Way.
In this patch, I'm introducing the SetupExecute function, and I've
installed it for all locations that are currently using
ExecuteWithLog. (Those were easy to find.) As the next step,
operations that do not even use ExecuteWithLog (which is part of the
"best practice" since some time ago, but was not from the start)
should be located and converted, too.
I have verified using COMPARE_BUILD that the resulting output does not
change. (The solaris_sparcv9 confirmation build is still running.)
The SetupExecute API works like this: You need to specify a COMMAND,
the actual command line to execute. You are strongly recommended to
provide a INFO with the text to display for LOG=info on what operation
is performed. You can use DEPS to provide additional dependencies for
your command to run. You can optionally include a PRE_COMMAND and a
POST_COMMAND, intended for simple pre- and post-processing. The latter
might be e.g. a mv from a temporary file to the final destination, the
former e.g. a simple sed replacement on the input file. If the
operations are unrelated to the main COMMAND, this is not a suitable
solution.
If your command outputs a variety of files, or if it's really a single
file but you don't really care about the output from the perspective,
you can just supply a OUTPUT_DIR. You are supposed to make sure the
command creates files in this directory (which will be created for you
if it does not exist), but this can't be enforced by SetupExecute.
Additional support files (like logs and markers) are created in this
directory. If you want support files in a separate directory (e.g. if
you're targeting an OUTPUT_DIR in the image directly), you can specify
a SUPPORT_DIR. If your command outputs only a single file, you can get
rid of the marker files (but not the log files) by specifying
OUTPUT_FILE. Note that if you specify an OUTPUT_FILE, support log
files will be placed in the same directory as the OUTPUT_FILE. If you
do not want that, use SUPPORT_DIR as well.
After the call to SetupExecute, $1 will contain references to all
generated files (that make knows about), and $1_TARGET will contain a
reference to the final target (that is OUTPUT_FILE if it exists, or
the $1_exec.marker file otherwise).
All the above keep functioning as expected even if PRE_COMMAND and
POST_COMMAND are given. One special case worth noting is that if
OUTPUT_FILE and POST_COMMAND is both given, the actual OUTPUT_FILE is
considered to be a result of running the POST_COMMAND. (This will
probably interact badly with a chmod as POST_COMMAND. I believe we
still have a few such places, so this behavior might need to get
revisited when I get around to converting those.)
While this might sound a bit convoluted, I find that it matches our
actual usage very well. The guiding idea here, as in the other
Setup functions, is to make the usage of the function as natural
and simple as possible, even if at the expense of a slightly more
complicated implementation.
Bug: https://bugs.openjdk.java.net/browse/JDK-8219971
WebRev:
http://cr.openjdk.java.net/~ihse/JDK-8219971-introduce-SetupExecute/webrev.01
/Magnus
