Summary: | [Mac] Add Build Phase to Check Headers for Inappropriate Macros (Platform.h macros) | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ddkilzer, joepeck, mitz, mrowe | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Joseph Pecoraro
2012-12-06 11:26:26 PST
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
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? (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. Comment on attachment 178049 [details]
[PATCH] Part 2: Run the Script as an Xcode Build Phase
r=me
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. 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). 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 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. (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. |