Bug 15813

Summary:

Incorrect handling of findstr results in *.vcproj

Product:

WebKit

Reporter:

Hiroaki Nakamura <hnakamur>

Component:

Tools / Tests

Assignee:

Adam Roben (:aroben) <aroben>

Status:

RESOLVED FIXED

Severity:

Trivial

CC:

aroben

Priority:

Version:

523.x (Safari 3)

Hardware:

OS:

Windows Vista

Attachments:

Description	Flags
Fix handling results of findstr	aroben: review-
Fix handling results of findstr	aroben: review+
patch to revision 27716	aroben: review+
Revised to apply against ToT (@r40493)	aroben: review+

Hiroaki Nakamura

Reported 2007-11-03 00:27:33 PDT

Although I was able to build the WebKit successfully, I found a trivial bug in WebKit/win/WebKit.vcproj/WebKit.vcproj and WebCore/WebCore.vcproj/WebCore.vcproj. Exit code of findstr is 0 when found and 1 when not found. So an correct handling of the result is: echo foo | findstr foo if errorlevel 1 (echo not found) else echo found or echo foo | findstr foo if %errorlevel% equ 0 (echo found) else echo not found I made a patch and will attach it.

Attachments
Fix handling results of findstr (18.63 KB, patch) 2007-11-03 00:28 PDT, Hiroaki Nakamura	aroben: review-	Details Formatted Diff Diff
Fix handling results of findstr (9.32 KB, patch) 2007-11-10 16:40 PST, Hiroaki Nakamura	aroben: review+	Details Formatted Diff Diff
patch to revision 27716 (9.75 KB, patch) 2007-11-12 10:21 PST, Hiroaki Nakamura	aroben: review+	Details Formatted Diff Diff
Revised to apply against ToT (@r40493) (27.01 KB, patch) 2009-02-02 15:03 PST, Brent Fulgham	aroben: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Hiroaki Nakamura

Comment 1 2007-11-03 00:28:20 PDT

Created attachment 17014 [details] Fix handling results of findstr

David Kilzer (:ddkilzer)

Comment 2 2007-11-03 08:54:01 PDT

Comment on attachment 17014 [details] Fix handling results of findstr Please set the "review?" flag on patches in the future to make sure they're reviewed. Thanks!

Hiroaki Nakamura

Comment 3 2007-11-03 16:55:27 PDT

Comment on attachment 17014 [details] Fix handling results of findstr Usage of "if ERRORLEVEL" was wrong.

Hiroaki Nakamura

Comment 4 2007-11-03 16:58:39 PDT

I edit my attachment and changed review flag to "+". Is this what you mean? Thanks.

Matt Lilek

Comment 5 2007-11-03 17:28:37 PDT

(In reply to comment #4) > I edit my attachment and changed review flag to "+". Is this what you mean? > Thanks. > No, set it to ? so a WebKit reviewer knows to take a look at it. They'll change it to +/-.

Hiroaki Nakamura

Comment 6 2007-11-03 21:01:09 PDT

Oh, now I understand, you had set the review flag which I should have set it. Sorry to bother you and thank you.

Adam Roben (:aroben)

Comment 7 2007-11-10 09:11:20 PST

Comment on attachment 17014 [details] Fix handling results of findstr I'm not sure how the new behavior is different from the old. Before we had: if ERRORLEVEL 0 set EnablePREfast false else set EnablePREfast true Now we have if ERRORLEVEL 1 set EnablePREfast true else set EnablePREfast false What is the difference? I also don't think we want "echo on" in our pre-build step.

Hiroaki Nakamura

Comment 8 2007-11-10 16:29:25 PST

Comment on attachment 17014 [details] Fix handling results of findstr Index: WebKit/win/WebKit.vcproj/WebKit.vcproj =================================================================== --- WebKit/win/WebKit.vcproj/WebKit.vcproj (revision 27388) +++ WebKit/win/WebKit.vcproj/WebKit.vcproj (working copy) @@ -25,7 +25,7 @@ > <Tool Name="VCPreBuildEventTool" - CommandLine="set PATH=%SystemDrive%\cygwin\bin;%PATH%
touch "$(WebKitOutputDir)\tmp.cpp"
cl /analyze /nologo /c "$(WebKitOutputDir)\tmp.cpp" 2>&1 | findstr D9040
if ERRORLEVEL 0 set EnablePREfast="false" else set EnablePREfast="true"
if ERRORLEVEL 0 set AnalyzeWithLargeStack="" AnalyzeWithLargeStack="/analyze:65536"

mkdir 2>NUL "$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore"
xcopy /y /d "$(WebKitLibrariesDir)\include\JavaScriptCore\JavaScriptCore\*" "$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore"

bash auto-version.sh "$(InputDir)." "$(IntDir)"

" + CommandLine="set PATH=%SystemDrive%\cygwin\bin;%PATH%
touch "$(WebKitOutputDir)\tmp.cpp"
cl /analyze /nologo /c "$(WebKitOutputDir)\tmp.cpp" 2>&1 | findstr D9040
if ERRORLEVEL 1 (set EnablePREfast="true") else set EnablePREfast="false"
if ERRORLEVEL 1 (set AnalyzeWithLargeStack="/analyze:65536") else set AnalyzeWithLargeStack=""

mkdir 2>NUL "$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore"
xcopy /y /d "$(WebKitLibrariesDir)\include\JavaScriptCore\JavaScriptCore\*" "$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore"

bash auto-version.sh "$(InputDir)." "$(IntDir)"

" /> <Tool Name="VCCustomBuildTool" @@ -119,7 +119,7 @@ > <Tool Name="VCPreBuildEventTool" - CommandLine="set PATH=%SystemDrive%\cygwin\bin;%PATH%
touch "$(WebKitOutputDir)\tmp.cpp"
cl /analyze /nologo /c "$(WebKitOutputDir)\tmp.cpp" 2>&1 | findstr D9040
if ERRORLEVEL 0 set EnablePREfast="false" else set EnablePREfast="true"
if ERRORLEVEL 0 set AnalyzeWithLargeStack="" AnalyzeWithLargeStack="/analyze:65536"

mkdir 2>NUL "$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore"
xcopy /y /d "$(WebKitLibrariesDir)\include\JavaScriptCore\JavaScriptCore\*" "$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore"

bash auto-version.sh "$(InputDir)." "$(IntDir)"

" + CommandLine="set PATH=%SystemDrive%\cygwin\bin;%PATH%
touch "$(WebKitOutputDir)\tmp.cpp"
cl /analyze /nologo /c "$(WebKitOutputDir)\tmp.cpp" 2>&1 | findstr D9040
if ERRORLEVEL 1 (set EnablePREfast="true") else set EnablePREfast="false"
if ERRORLEVEL 1 (set AnalyzeWithLargeStack="/analyze:65536") else set AnalyzeWithLargeStack=""

mkdir 2>NUL "$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore"
xcopy /y /d "$(WebKitLibrariesDir)\include\JavaScriptCore\JavaScriptCore\*" "$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore"

bash auto-version.sh "$(InputDir)." "$(IntDir)"

" /> <Tool Name="VCCustomBuildTool" @@ -211,7 +211,7 @@ > <Tool Name="VCPreBuildEventTool" - CommandLine="set PATH=%SystemDrive%\cygwin\bin;%PATH%
touch "$(WebKitOutputDir)\tmp.cpp"
cl /analyze /nologo /c "$(WebKitOutputDir)\tmp.cpp" 2>&1 | findstr D9040
if ERRORLEVEL 0 set EnablePREfast="false" else set EnablePREfast="true"
if ERRORLEVEL 0 set AnalyzeWithLargeStack="" AnalyzeWithLargeStack="/analyze:65536"

mkdir 2>NUL "$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore"
xcopy /y /d "$(WebKitLibrariesDir)\include\JavaScriptCore\JavaScriptCore\*" "$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore"

bash auto-version.sh "$(InputDir)." "$(IntDir)"

" + CommandLine="set PATH=%SystemDrive%\cygwin\bin;%PATH%
touch "$(WebKitOutputDir)\tmp.cpp"
cl /analyze /nologo /c "$(WebKitOutputDir)\tmp.cpp" 2>&1 | findstr D9040
if ERRORLEVEL 1 (set EnablePREfast="true") else set EnablePREfast="false"
if ERRORLEVEL 1 (set AnalyzeWithLargeStack="/analyze:65536") else set AnalyzeWithLargeStack=""

mkdir 2>NUL "$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore"
xcopy /y /d "$(WebKitLibrariesDir)\include\JavaScriptCore\JavaScriptCore\*" "$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore"

bash auto-version.sh "$(InputDir)." "$(IntDir)"

" /> <Tool Name="VCCustomBuildTool" Index: WebCore/WebCore.vcproj/WebCore.vcproj =================================================================== --- WebCore/WebCore.vcproj/WebCore.vcproj (revision 27388) +++ WebCore/WebCore.vcproj/WebCore.vcproj (working copy) @@ -26,7 +26,7 @@ > <Tool Name="VCPreBuildEventTool" - CommandLine="set PATH=%SystemDrive%\cygwin\bin;%PATH%
touch "$(WebKitOutputDir)\tmp.cpp"
cl /analyze /nologo /c "$(WebKitOutputDir)\tmp.cpp" 2>&1 | findstr D9040
if ERRORLEVEL 0 set EnablePREfast="false" else set EnablePREfast="true"
if ERRORLEVEL 0 set AnalyzeWithLargeStack="" AnalyzeWithLargeStack="/analyze:65536"
bash build-generated-files.sh "$(WebKitOutputDir)" "$(WebKitLibrariesDir)"
" + CommandLine="set PATH=%SystemDrive%\cygwin\bin;%PATH%
touch "$(WebKitOutputDir)\tmp.cpp"
cl /analyze /nologo /c "$(WebKitOutputDir)\tmp.cpp" 2>&1 | findstr D9040
if ERRORLEVEL 1 (set EnablePREfast="true") else set EnablePREfast="false"
if ERRORLEVEL 1 (set AnalyzeWithLargeStack="/analyze:65536") else set AnalyzeWithLargeStack=""
bash build-generated-files.sh "$(WebKitOutputDir)" "$(WebKitLibrariesDir)"
" /> <Tool Name="VCCustomBuildTool" @@ -101,7 +101,7 @@ > <Tool Name="VCPreBuildEventTool" - CommandLine="set PATH=%SystemDrive%\cygwin\bin;%PATH%
touch "$(WebKitOutputDir)\tmp.cpp"
cl /analyze /nologo /c "$(WebKitOutputDir)\tmp.cpp" 2>&1 | findstr D9040
if ERRORLEVEL 0 set EnablePREfast="false" else set EnablePREfast="true"
if ERRORLEVEL 0 set AnalyzeWithLargeStack="" AnalyzeWithLargeStack="/analyze:65536"
bash build-generated-files.sh "$(WebKitOutputDir)" "$(WebKitLibrariesDir)"
" + CommandLine="set PATH=%SystemDrive%\cygwin\bin;%PATH%
touch "$(WebKitOutputDir)\tmp.cpp"
cl /analyze /nologo /c "$(WebKitOutputDir)\tmp.cpp" 2>&1 | findstr D9040
if ERRORLEVEL 1 (set EnablePREfast="true") else set EnablePREfast="false"
if ERRORLEVEL 1 (set AnalyzeWithLargeStack="/analyze:65536") else set AnalyzeWithLargeStack=""
bash build-generated-files.sh "$(WebKitOutputDir)" "$(WebKitLibrariesDir)"
" /> <Tool Name="VCCustomBuildTool" @@ -172,7 +172,7 @@ > <Tool Name="VCPreBuildEventTool" - CommandLine="set PATH=%SystemDrive%\cygwin\bin;%PATH%
touch "$(WebKitOutputDir)\tmp.cpp"
cl /analyze /nologo /c "$(WebKitOutputDir)\tmp.cpp" 2>&1 | findstr D9040
if ERRORLEVEL 0 set EnablePREfast="false" else set EnablePREfast="true"
if ERRORLEVEL 0 set AnalyzeWithLargeStack="" AnalyzeWithLargeStack="/analyze:65536"
bash build-generated-files.sh "$(WebKitOutputDir)" "$(WebKitLibrariesDir)"
" + CommandLine="set PATH=%SystemDrive%\cygwin\bin;%PATH%
touch "$(WebKitOutputDir)\tmp.cpp"
cl /analyze /nologo /c "$(WebKitOutputDir)\tmp.cpp" 2>&1 | findstr D9040
if ERRORLEVEL 1 (set EnablePREfast="true") else set EnablePREfast="false"
if ERRORLEVEL 1 (set AnalyzeWithLargeStack="/analyze:65536") else set AnalyzeWithLargeStack=""
bash build-generated-files.sh "$(WebKitOutputDir)" "$(WebKitLibrariesDir)"
" /> <Tool Name="VCCustomBuildTool"

Hiroaki Nakamura

Comment 9 2007-11-10 16:40:55 PST

Created attachment 17176 [details] Fix handling results of findstr First, "echo on" was my mistake. I forgot removing them. As for ERRORLEVEL, I read the help of cmd.exe and understand that "if ERRORLEVEL n stmt1 else stmt2" means if errorlevel is greater than or equal to n then execute stmt1 else execute stmt2. And findstr returns 0 when found, 1 when not found. So if you write "if ERRORLEVEL 0", stmt1 is always executed. Also you need parenthesis if stmt1 is not one word. I tried without parenthesis and it didn't work correctly. I tried on Japanese Windows Vista Home Edition. And original project file was missing "else" for setting AnalyzeWithLargeStack.

Adam Roben (:aroben)

Comment 10 2007-11-10 23:54:43 PST

Comment on attachment 17176 [details] Fix handling results of findstr Thanks for the explanation. It seems to me we could combine the two if/else statements into one. But r=me even if you don't do that. Thanks!

Mark Rowe (bdash)

Comment 11 2007-11-12 09:41:27 PST

This doesn't apply any more :(

Hiroaki Nakamura

Comment 12 2007-11-12 10:21:06 PST

Created attachment 17208 [details] patch to revision 27716 Hi, Mark. I recreated the patch for revision 27716. Please try again.

Adam Roben (:aroben)

Comment 13 2007-11-19 00:29:04 PST

Comment on attachment 17208 [details] patch to revision 27716 r=me again

Darin Adler

Comment 14 2008-10-14 11:19:12 PDT

Adam, could you land this patch, or clear the review flag if the patch is one again impossible to apply. I'd like this out of our "reviewed by not committed" list.

Brent Fulgham

Comment 15 2009-02-02 15:03:01 PST

Created attachment 27261 [details] Revised to apply against ToT (@r40493)

Adam Roben (:aroben)

Comment 16 2009-02-02 20:22:56 PST

Comment on attachment 27261 [details] Revised to apply against ToT (@r40493) r=me

Brent Fulgham

Comment 17 2009-02-02 20:50:12 PST

Committed in revision 40520.

Note You need to log in before you can comment on or make changes to this bug.