Bug 104279 – [Mac] Add Build Phase to Check Headers for Inappropriate Macros (Platform.h macros)

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Joseph Pecoraro]

We want to ensure the headers (public and sometimes private) in the mac frameworks don't
contain <wtf/Platform.h> macros (PLATFORM(), ENABLE(), USE(), etc).

There are already a number of build phase scripts run in each of the projects to verify
the build products output. This would be another that checks for common macros.

[Joseph Pecoraro]

<rdar://problem/12692988>

[Joseph Pecoraro]

Created attachment 178047 [details]
[PATCH] Part 1: Add script check-for-inappropriate-macros-in-external-headers

Adds another ruby script and tests. The existing check-* scripts are mostly ruby. None had tests so I added a webkitruby dir and tests to mirror webkitperl and webkitpython.

Running tests looks like:

    shell> ./Tools/Scripts/test-webkitruby 
    Testing: Tools/Scripts/check-for-inappropriate-macros-in-external-headers
    PASS - test_good_fake_data
    PASS - test_bad_fake_data

[Joseph Pecoraro]

Created attachment 178049 [details]
[PATCH] Part 2: Run the Script as an Xcode Build Phase

Build phase in each looks like: (note we are just testing public headers right now)

>    if [ "${ACTION}" = "installhdrs" ]; then
>        exit 0;
>    fi
>
>    if [ -f ../../Tools/Scripts/check-for-inappropriate-macros-in-external-headers ]; then
>        ../../Tools/Scripts/check-for-inappropriate-macros-in-external-headers Headers || exit $?
>    fi

Comment on attachment 178047 [details]
[PATCH] Part 1: Add script check-for-inappropriate-macros-in-external-headers

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

> Tools/Scripts/check-for-inappropriate-macros-in-external-headers:43
> +  full_path = File.join Dir.pwd, framework, $is_shallow_bundle ? "" : "Versions/A/", path

Can you use CONTENTS_FOLDER_PATH instead of the framework parameter and this?

[Joseph Pecoraro]

(In reply to comment #4)
> (From update of attachment 178047 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178047&action=review
> 
> > Tools/Scripts/check-for-inappropriate-macros-in-external-headers:43
> > +  full_path = File.join Dir.pwd, framework, $is_shallow_bundle ? "" : "Versions/A/", path
> 
> Can you use CONTENTS_FOLDER_PATH instead of the framework parameter and this?

Good idea. I'll take a look. The other Scripts/Tools/check-* scripts used this SHALLOW_BUNDLE approach, so if we can convert to a cleaner approach I'll update those as well.

[David Kilzer (:ddkilzer)]

Comment on attachment 178049 [details]
[PATCH] Part 2: Run the Script as an Xcode Build Phase

r=me

[David Kilzer (:ddkilzer)]

Comment on attachment 178047 [details]
[PATCH] Part 1: Add script check-for-inappropriate-macros-in-external-headers

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

r=me, but please address Dan's comment and making sure to use the same ruby binary in test-webkitruby when running tests.

Also, I think Tools/Scripts/webkitruby/check-for-inappropriate-macros-in-external-headers-tests/passing-expected.txt was probably renamed to pass-expected.txt, so it should be removed from the commit.

> Tools/Scripts/test-webkit-scripts:31
> +"""Run unit tests of WebKit's Perl, Ruby, and Python scripts."""

Nit:  Should it be "Perl, Python and Ruby scripts" to keep them alphabetized?  :)

> Tools/Scripts/test-webkit-scripts:80
> +        print('Note: Perl, Ruby, and Python results appear separately above.')

Ditto.

> Tools/Scripts/test-webkitruby:30
> +  puts %x{ ruby '#{test}' }

Does using "ruby" here imply that you're using the same ruby executable that was used to launch this script?  If not, we should use the path the ruby binary that was used to run this script (however you grab that in ruby; maybe "$0"?) to make the behavior more predictable.

[David Kilzer (:ddkilzer)]

Comment on attachment 178047 [details]
[PATCH] Part 1: Add script check-for-inappropriate-macros-in-external-headers

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

> Tools/Scripts/webkitruby/check-for-inappropriate-macros-in-external-headers-tests/run-test.rb:44
> +  output = sanitized_output %x{ #{$tool} #{config[:paths].join(' ')} 2>&1 }
> +  expected_output = File.read File.join($test_directory, config[:expected])
> +  pass = output == expected_output

Also, if the test passes, I think the *-expected.txt file should have the word "PASS" with a newline in it (rather than just being blank).

[Joseph Pecoraro]

Looks like there are a few paths I could probably take advantage of:

    (taken from a WebCore check-* build phase)
    setenv CONTENTS_FOLDER_PATH WebCore.framework/Versions/A
    setenv PRIVATE_HEADERS_FOLDER_PATH WebCore.framework/Versions/A/PrivateHeaders
    setenv PUBLIC_HEADERS_FOLDER_PATH WebCore.framework/Versions/A/Headers

[Joseph Pecoraro]

Comment on attachment 178047 [details]
[PATCH] Part 1: Add script check-for-inappropriate-macros-in-external-headers

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

>> Tools/Scripts/test-webkit-scripts:31
>> +"""Run unit tests of WebKit's Perl, Ruby, and Python scripts."""
> 
> Nit:  Should it be "Perl, Python and Ruby scripts" to keep them alphabetized?  :)

Heh, sure I can do that.

>> Tools/Scripts/test-webkitruby:30
>> +  puts %x{ ruby '#{test}' }
> 
> Does using "ruby" here imply that you're using the same ruby executable that was used to launch this script?  If not, we should use the path the ruby binary that was used to run this script (however you grab that in ruby; maybe "$0"?) to make the behavior more predictable.

Good call. I have yet to find a way to determine the current ruby executable running the script, so instead I'll just drop this "ruby" and run the test (which will use the env ruby). This line becomes:

    puts %x{ '#{test}' }

>> Tools/Scripts/webkitruby/check-for-inappropriate-macros-in-external-headers-tests/run-test.rb:44
>> +  pass = output == expected_output
> 
> Also, if the test passes, I think the *-expected.txt file should have the word "PASS" with a newline in it (rather than just being blank).

These expected files are the expected results of running the check-* script. I wouldn't expect the script to ever output PASS.

Probably a better test would be, if the exit code is 0, ensure the output is empty. If the exit code is 1, ensure the output matches some error-expected.txt file. I'll try making that change.

[Joseph Pecoraro]

(In reply to comment #9)
> Looks like there are a few paths I could probably take advantage of:
> 
>     (taken from a WebCore check-* build phase)
>     setenv CONTENTS_FOLDER_PATH WebCore.framework/Versions/A
>     setenv PRIVATE_HEADERS_FOLDER_PATH WebCore.framework/Versions/A/PrivateHeaders
>     setenv PUBLIC_HEADERS_FOLDER_PATH WebCore.framework/Versions/A/Headers

To address this comment I filed:
<http://webkit.org/b/105336> [Mac] Build Phase check-* scripts should use CONTENTS_FOLDER_PATH instead of SHALLOW_BUNDLE

Kilzer reviewed (offline) follow up fixes to the test and his earlier comments. I'm doing a final test build now and then I'll land.

[Joseph Pecoraro]

Committed r138064 and r138065:
<http://trac.webkit.org/changeset/138064>
<http://trac.webkit.org/changeset/138065>