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.)
<rdar://problem/49040807>
Created attachment 365258 [details] Patch
Created attachment 365282 [details] Patch
Comment on attachment 365282 [details] Patch Patch got borked.
Created attachment 365307 [details] Patch
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.
(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.
> What refactoring do you see it interfering with? The general example is find and replace.
Comment on attachment 365307 [details] Patch Clearing flags on attachment: 365307 Committed r243527: <https://trac.webkit.org/changeset/243527>
All reviewed patches have been landed. Closing bug.