RFR: JDK-8219971 Introduce SetupExecute in build system

[Erik Joelsson]

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<Foo> 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<Foo> 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
>