Bug 195975 - Update the way generate-xcfilelists returns strings from functions
Summary: Update the way generate-xcfilelists returns strings from functions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Rollin
URL:
Keywords: InRadar
Depends on: 195977
Blocks:
  Show dependency treegraph
 
Reported: 2019-03-19 16:24 PDT by Keith Rollin
Modified: 2019-03-26 16:51 PDT (History)
5 users (show)

See Also:


Attachments
Patch (8.15 KB, patch)
2019-03-19 16:36 PDT, Keith Rollin
no flags Details | Formatted Diff | Diff
Patch (2.64 KB, patch)
2019-03-19 18:31 PDT, Keith Rollin
no flags Details | Formatted Diff | Diff
Patch (8.59 KB, patch)
2019-03-19 22:12 PDT, Keith Rollin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Rollin 2019-03-19 16:24:10 PDT
There are places where generate-xcfilelists executes assignments with statements like:

FOO=$(some_function)

where "some_function" return a string by echoing it. E.g.

some_function()
{
    echo "Hello, World"
}

This is a common idiom, but it has a problem if "some_function" needs to call "exit" in an attempt to halt the entire script right then and there. Since "some_function" is called inside of $(), it's being executed in a sub-shell. Calling exit in that sub-shell simply exits that shell; it doesn't not exit the outer shell in which the main part of the script is still running. As such, the main script keeps executing when the intent was for the script to halt.

The solution to this is to use a different idiom for returning strings. The one we now is to pass in the name of the variable to receive the string result:

some_function()
{
    variable_name=$1
    eval $variable_name ="Hello, World"
}

The call site now looks like

some_function FOO

Because there's no invocation of a sub-shell, some_function can now call "exit" if it wants to, and the entire script will exit at that point.

(This work is part of addressing Radar 48616075, which notes that building for iphonesimulator not only reports that iphonesimulator is an invalid platform, but that the build does not halt at that error as is expected. This patch addresses the second part where the build is not stopping.)
Comment 1 Radar WebKit Bug Importer 2019-03-19 16:24:54 PDT
<rdar://problem/49040807>
Comment 2 Keith Rollin 2019-03-19 16:36:10 PDT
Created attachment 365258 [details]
Patch
Comment 3 Keith Rollin 2019-03-19 18:31:29 PDT
Created attachment 365282 [details]
Patch
Comment 4 Keith Rollin 2019-03-19 18:32:41 PDT
Comment on attachment 365282 [details]
Patch

Patch got borked.
Comment 5 Keith Rollin 2019-03-19 22:12:31 PDT
Created attachment 365307 [details]
Patch
Comment 6 Alexey Proskuryakov 2019-03-20 10:20:06 PDT
Comment on attachment 365307 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365307&action=review

> Tools/ChangeLog:31
> +        The solution to this is to use a different idiom for returning
> +        strings. The one we now is to pass in the name of the variable to
> +        receive the string result:

I probably shouldn't review this patch because I don't even see where exactly we spawn shells to call a function, but I can't help myself asking whether executing sub-shells is unavoidable in the first place.

> Tools/Scripts/generate-xcfilelists:612
> +    [[ "${GX_PROVISIONAL_CONFIGURATION}" == "" ]]           && { eval $GX_RESULT=""; return; }

I can't find any such rules in WebKit coding style, but I remember seeing reviewer feedback about how we don't do formatting of this kind in WebKit as a matter of coding style. The rationale is that it makes refactoring harder, and I agree with that.
Comment 7 Keith Rollin 2019-03-20 12:20:16 PDT
(In reply to Alexey Proskuryakov from comment #6)
> Comment on attachment 365307 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365307&action=review
> 
> > Tools/ChangeLog:31
> > +        The solution to this is to use a different idiom for returning
> > +        strings. The one we now is to pass in the name of the variable to
> > +        receive the string result:
> 
> I probably shouldn't review this patch because I don't even see where
> exactly we spawn shells to call a function,

I don't know who's good for reviewing bash scripts either, but I'll add Dean, who looked at this script before and understood it enough to hack it up.

> but I can't help myself asking
> whether executing sub-shells is unavoidable in the first place.

A fair question.

The normal idiom for having a shell function return a string and have that string be assigned to a variable is:

FOO=$(some_function)

This syntax executes "some_function" in a sub-shell and replaces the whole $(...) with the textual output of that function, allowing that text to be assigned to the variable. You can see this in the actual script in places like:

local GX_SDK=$(get_canonical_platform_name "$1")

There's no replaceable construct that can replace that, AFAIK. For instance, we can't say: 

local GX_SDK=get_canonical_platform_name "$1"

because that won't treat "get_canonical_platform_name $1" as a function call. We can't use as something that allows for functions to be called and the output gathered in a single location:

local GX_SDK={get_canonical_platform_name "$1";}

because that's not valid syntax.

Searching the web for alternatives to $(...), I didn't find any solution that I liked better than what I ended up using here.

> > Tools/Scripts/generate-xcfilelists:612
> > +    [[ "${GX_PROVISIONAL_CONFIGURATION}" == "" ]]           && { eval $GX_RESULT=""; return; }
> 
> I can't find any such rules in WebKit coding style, but I remember seeing
> reviewer feedback about how we don't do formatting of this kind in WebKit as
> a matter of coding style. The rationale is that it makes refactoring harder,
> and I agree with that.

I like the spacing because it makes it really clear what the function is returning. All of the strings I'm returning are lined up, making it clear what's happening in what situations. Without that spacing, I had a hard time seeing what was happening. So, from that aspect of readability and maintainability, I like it.

What refactoring do you see it interfering with? About all I can think if is if I add some new cases that require the amount of spacing to be increased, meaning that I'd be touching lines for no reason other than to increase that spacing. For me, the maintainability of seeing what the code is doing outweighs that.
Comment 8 Alexey Proskuryakov 2019-03-20 14:39:14 PDT
> What refactoring do you see it interfering with?

The general example is find and replace.
Comment 9 WebKit Commit Bot 2019-03-26 16:51:16 PDT
Comment on attachment 365307 [details]
Patch

Clearing flags on attachment: 365307

Committed r243527: <https://trac.webkit.org/changeset/243527>
Comment 10 WebKit Commit Bot 2019-03-26 16:51:18 PDT
All reviewed patches have been landed.  Closing bug.