Bug 15813

Summary: Incorrect handling of findstr results in *.vcproj
Product: WebKit Reporter: Hiroaki Nakamura <hnakamur>
Component: Tools / TestsAssignee: Adam Roben (:aroben) <aroben>
Status: RESOLVED FIXED    
Severity: Trivial CC: aroben
Priority: P2    
Version: 523.x (Safari 3)   
Hardware: PC   
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+

Description Hiroaki Nakamura 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.
Comment 1 Hiroaki Nakamura 2007-11-03 00:28:20 PDT
Created attachment 17014 [details]
Fix handling results of findstr
Comment 2 David Kilzer (:ddkilzer) 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!
Comment 3 Hiroaki Nakamura 2007-11-03 16:55:27 PDT
Comment on attachment 17014 [details]
Fix handling results of findstr

Usage of "if ERRORLEVEL" was wrong.
Comment 4 Hiroaki Nakamura 2007-11-03 16:58:39 PDT
I edit my attachment and changed review flag to "+". Is this what you mean? Thanks.
Comment 5 Matt Lilek 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 +/-.
Comment 6 Hiroaki Nakamura 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.
Comment 7 Adam Roben (:aroben) 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.
Comment 8 Hiroaki Nakamura 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%&#x0D;&#x0A;touch &quot;$(WebKitOutputDir)\tmp.cpp&quot;&#x0D;&#x0A;cl /analyze /nologo /c &quot;$(WebKitOutputDir)\tmp.cpp&quot; 2&gt;&amp;1 | findstr D9040&#x0D;&#x0A;if ERRORLEVEL 0 set EnablePREfast=&quot;false&quot; else set EnablePREfast=&quot;true&quot;&#x0D;&#x0A;if ERRORLEVEL 0 set AnalyzeWithLargeStack=&quot;&quot; AnalyzeWithLargeStack=&quot;/analyze:65536&quot;&#x0D;&#x0A;&#x0D;&#x0A;mkdir 2&gt;NUL &quot;$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore&quot;&#x0D;&#x0A;xcopy /y /d &quot;$(WebKitLibrariesDir)\include\JavaScriptCore\JavaScriptCore\*&quot; &quot;$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore&quot;&#x0D;&#x0A;&#x0D;&#x0A;bash auto-version.sh &quot;$(InputDir).&quot; &quot;$(IntDir)&quot;&#x0D;&#x0A;&#x0D;&#x0A;"
+				CommandLine="set PATH=%SystemDrive%\cygwin\bin;%PATH%&#x0D;&#x0A;touch &quot;$(WebKitOutputDir)\tmp.cpp&quot;&#x0D;&#x0A;cl /analyze /nologo /c &quot;$(WebKitOutputDir)\tmp.cpp&quot; 2&gt;&amp;1 | findstr D9040&#x0D;&#x0A;if ERRORLEVEL 1 (set EnablePREfast=&quot;true&quot;) else set EnablePREfast=&quot;false&quot;&#x0D;&#x0A;if ERRORLEVEL 1 (set AnalyzeWithLargeStack=&quot;/analyze:65536&quot;) else set AnalyzeWithLargeStack=&quot;&quot;&#x0D;&#x0A;&#x0D;&#x0A;mkdir 2&gt;NUL &quot;$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore&quot;&#x0D;&#x0A;xcopy /y /d &quot;$(WebKitLibrariesDir)\include\JavaScriptCore\JavaScriptCore\*&quot; &quot;$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore&quot;&#x0D;&#x0A;&#x0D;&#x0A;bash auto-version.sh &quot;$(InputDir).&quot; &quot;$(IntDir)&quot;&#x0D;&#x0A;&#x0D;&#x0A;"
 			/>
 			<Tool
 				Name="VCCustomBuildTool"
@@ -119,7 +119,7 @@
 			>
 			<Tool
 				Name="VCPreBuildEventTool"
-				CommandLine="set PATH=%SystemDrive%\cygwin\bin;%PATH%&#x0D;&#x0A;touch &quot;$(WebKitOutputDir)\tmp.cpp&quot;&#x0D;&#x0A;cl /analyze /nologo /c &quot;$(WebKitOutputDir)\tmp.cpp&quot; 2&gt;&amp;1 | findstr D9040&#x0D;&#x0A;if ERRORLEVEL 0 set EnablePREfast=&quot;false&quot; else set EnablePREfast=&quot;true&quot;&#x0D;&#x0A;if ERRORLEVEL 0 set AnalyzeWithLargeStack=&quot;&quot; AnalyzeWithLargeStack=&quot;/analyze:65536&quot;&#x0D;&#x0A;&#x0D;&#x0A;mkdir 2&gt;NUL &quot;$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore&quot;&#x0D;&#x0A;xcopy /y /d &quot;$(WebKitLibrariesDir)\include\JavaScriptCore\JavaScriptCore\*&quot; &quot;$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore&quot;&#x0D;&#x0A;&#x0D;&#x0A;bash auto-version.sh &quot;$(InputDir).&quot; &quot;$(IntDir)&quot;&#x0D;&#x0A;&#x0D;&#x0A;"
+				CommandLine="set PATH=%SystemDrive%\cygwin\bin;%PATH%&#x0D;&#x0A;touch &quot;$(WebKitOutputDir)\tmp.cpp&quot;&#x0D;&#x0A;cl /analyze /nologo /c &quot;$(WebKitOutputDir)\tmp.cpp&quot; 2&gt;&amp;1 | findstr D9040&#x0D;&#x0A;if ERRORLEVEL 1 (set EnablePREfast=&quot;true&quot;) else set EnablePREfast=&quot;false&quot;&#x0D;&#x0A;if ERRORLEVEL 1 (set AnalyzeWithLargeStack=&quot;/analyze:65536&quot;) else set AnalyzeWithLargeStack=&quot;&quot;&#x0D;&#x0A;&#x0D;&#x0A;mkdir 2&gt;NUL &quot;$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore&quot;&#x0D;&#x0A;xcopy /y /d &quot;$(WebKitLibrariesDir)\include\JavaScriptCore\JavaScriptCore\*&quot; &quot;$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore&quot;&#x0D;&#x0A;&#x0D;&#x0A;bash auto-version.sh &quot;$(InputDir).&quot; &quot;$(IntDir)&quot;&#x0D;&#x0A;&#x0D;&#x0A;"
 			/>
 			<Tool
 				Name="VCCustomBuildTool"
@@ -211,7 +211,7 @@
 			>
 			<Tool
 				Name="VCPreBuildEventTool"
-				CommandLine="set PATH=%SystemDrive%\cygwin\bin;%PATH%&#x0D;&#x0A;touch &quot;$(WebKitOutputDir)\tmp.cpp&quot;&#x0D;&#x0A;cl /analyze /nologo /c &quot;$(WebKitOutputDir)\tmp.cpp&quot; 2&gt;&amp;1 | findstr D9040&#x0D;&#x0A;if ERRORLEVEL 0 set EnablePREfast=&quot;false&quot; else set EnablePREfast=&quot;true&quot;&#x0D;&#x0A;if ERRORLEVEL 0 set AnalyzeWithLargeStack=&quot;&quot; AnalyzeWithLargeStack=&quot;/analyze:65536&quot;&#x0D;&#x0A;&#x0D;&#x0A;mkdir 2&gt;NUL &quot;$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore&quot;&#x0D;&#x0A;xcopy /y /d &quot;$(WebKitLibrariesDir)\include\JavaScriptCore\JavaScriptCore\*&quot; &quot;$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore&quot;&#x0D;&#x0A;&#x0D;&#x0A;bash auto-version.sh &quot;$(InputDir).&quot; &quot;$(IntDir)&quot;&#x0D;&#x0A;&#x0D;&#x0A;"
+				CommandLine="set PATH=%SystemDrive%\cygwin\bin;%PATH%&#x0D;&#x0A;touch &quot;$(WebKitOutputDir)\tmp.cpp&quot;&#x0D;&#x0A;cl /analyze /nologo /c &quot;$(WebKitOutputDir)\tmp.cpp&quot; 2&gt;&amp;1 | findstr D9040&#x0D;&#x0A;if ERRORLEVEL 1 (set EnablePREfast=&quot;true&quot;) else set EnablePREfast=&quot;false&quot;&#x0D;&#x0A;if ERRORLEVEL 1 (set AnalyzeWithLargeStack=&quot;/analyze:65536&quot;) else set AnalyzeWithLargeStack=&quot;&quot;&#x0D;&#x0A;&#x0D;&#x0A;mkdir 2&gt;NUL &quot;$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore&quot;&#x0D;&#x0A;xcopy /y /d &quot;$(WebKitLibrariesDir)\include\JavaScriptCore\JavaScriptCore\*&quot; &quot;$(WebKitOutputDir)\include\JavaScriptCore\JavaScriptCore&quot;&#x0D;&#x0A;&#x0D;&#x0A;bash auto-version.sh &quot;$(InputDir).&quot; &quot;$(IntDir)&quot;&#x0D;&#x0A;&#x0D;&#x0A;"
 			/>
 			<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%&#x0D;&#x0A;touch &quot;$(WebKitOutputDir)\tmp.cpp&quot;&#x0D;&#x0A;cl /analyze /nologo /c &quot;$(WebKitOutputDir)\tmp.cpp&quot; 2&gt;&amp;1 | findstr D9040&#x0D;&#x0A;if ERRORLEVEL 0 set EnablePREfast=&quot;false&quot; else set EnablePREfast=&quot;true&quot;&#x0D;&#x0A;if ERRORLEVEL 0 set AnalyzeWithLargeStack=&quot;&quot; AnalyzeWithLargeStack=&quot;/analyze:65536&quot;&#x0D;&#x0A;bash build-generated-files.sh &quot;$(WebKitOutputDir)&quot; &quot;$(WebKitLibrariesDir)&quot;&#x0D;&#x0A;"
+				CommandLine="set PATH=%SystemDrive%\cygwin\bin;%PATH%&#x0D;&#x0A;touch &quot;$(WebKitOutputDir)\tmp.cpp&quot;&#x0D;&#x0A;cl /analyze /nologo /c &quot;$(WebKitOutputDir)\tmp.cpp&quot; 2&gt;&amp;1 | findstr D9040&#x0D;&#x0A;if ERRORLEVEL 1 (set EnablePREfast=&quot;true&quot;) else set EnablePREfast=&quot;false&quot;&#x0D;&#x0A;if ERRORLEVEL 1 (set AnalyzeWithLargeStack=&quot;/analyze:65536&quot;) else set AnalyzeWithLargeStack=&quot;&quot;&#x0D;&#x0A;bash build-generated-files.sh &quot;$(WebKitOutputDir)&quot; &quot;$(WebKitLibrariesDir)&quot;&#x0D;&#x0A;"
 			/>
 			<Tool
 				Name="VCCustomBuildTool"
@@ -101,7 +101,7 @@
 			>
 			<Tool
 				Name="VCPreBuildEventTool"
-				CommandLine="set PATH=%SystemDrive%\cygwin\bin;%PATH%&#x0D;&#x0A;touch &quot;$(WebKitOutputDir)\tmp.cpp&quot;&#x0D;&#x0A;cl /analyze /nologo /c &quot;$(WebKitOutputDir)\tmp.cpp&quot; 2&gt;&amp;1 | findstr D9040&#x0D;&#x0A;if ERRORLEVEL 0 set EnablePREfast=&quot;false&quot; else set EnablePREfast=&quot;true&quot;&#x0D;&#x0A;if ERRORLEVEL 0 set AnalyzeWithLargeStack=&quot;&quot; AnalyzeWithLargeStack=&quot;/analyze:65536&quot;&#x0D;&#x0A;bash build-generated-files.sh &quot;$(WebKitOutputDir)&quot; &quot;$(WebKitLibrariesDir)&quot;&#x0D;&#x0A;"
+				CommandLine="set PATH=%SystemDrive%\cygwin\bin;%PATH%&#x0D;&#x0A;touch &quot;$(WebKitOutputDir)\tmp.cpp&quot;&#x0D;&#x0A;cl /analyze /nologo /c &quot;$(WebKitOutputDir)\tmp.cpp&quot; 2&gt;&amp;1 | findstr D9040&#x0D;&#x0A;if ERRORLEVEL 1 (set EnablePREfast=&quot;true&quot;) else set EnablePREfast=&quot;false&quot;&#x0D;&#x0A;if ERRORLEVEL 1 (set AnalyzeWithLargeStack=&quot;/analyze:65536&quot;) else set AnalyzeWithLargeStack=&quot;&quot;&#x0D;&#x0A;bash build-generated-files.sh &quot;$(WebKitOutputDir)&quot; &quot;$(WebKitLibrariesDir)&quot;&#x0D;&#x0A;"
 			/>
 			<Tool
 				Name="VCCustomBuildTool"
@@ -172,7 +172,7 @@
 			>
 			<Tool
 				Name="VCPreBuildEventTool"
-				CommandLine="set PATH=%SystemDrive%\cygwin\bin;%PATH%&#x0D;&#x0A;touch &quot;$(WebKitOutputDir)\tmp.cpp&quot;&#x0D;&#x0A;cl /analyze /nologo /c &quot;$(WebKitOutputDir)\tmp.cpp&quot; 2&gt;&amp;1 | findstr D9040&#x0D;&#x0A;if ERRORLEVEL 0 set EnablePREfast=&quot;false&quot; else set EnablePREfast=&quot;true&quot;&#x0D;&#x0A;if ERRORLEVEL 0 set AnalyzeWithLargeStack=&quot;&quot; AnalyzeWithLargeStack=&quot;/analyze:65536&quot;&#x0D;&#x0A;bash build-generated-files.sh &quot;$(WebKitOutputDir)&quot; &quot;$(WebKitLibrariesDir)&quot;&#x0D;&#x0A;"
+				CommandLine="set PATH=%SystemDrive%\cygwin\bin;%PATH%&#x0D;&#x0A;touch &quot;$(WebKitOutputDir)\tmp.cpp&quot;&#x0D;&#x0A;cl /analyze /nologo /c &quot;$(WebKitOutputDir)\tmp.cpp&quot; 2&gt;&amp;1 | findstr D9040&#x0D;&#x0A;if ERRORLEVEL 1 (set EnablePREfast=&quot;true&quot;) else set EnablePREfast=&quot;false&quot;&#x0D;&#x0A;if ERRORLEVEL 1 (set AnalyzeWithLargeStack=&quot;/analyze:65536&quot;) else set AnalyzeWithLargeStack=&quot;&quot;&#x0D;&#x0A;bash build-generated-files.sh &quot;$(WebKitOutputDir)&quot; &quot;$(WebKitLibrariesDir)&quot;&#x0D;&#x0A;"
 			/>
 			<Tool
 				Name="VCCustomBuildTool"
Comment 9 Hiroaki Nakamura 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.
Comment 10 Adam Roben (:aroben) 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!
Comment 11 Mark Rowe (bdash) 2007-11-12 09:41:27 PST
This doesn't apply any more :(
Comment 12 Hiroaki Nakamura 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.
Comment 13 Adam Roben (:aroben) 2007-11-19 00:29:04 PST
Comment on attachment 17208 [details]
patch to revision 27716

r=me again
Comment 14 Darin Adler 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.
Comment 15 Brent Fulgham 2009-02-02 15:03:01 PST
Created attachment 27261 [details]
Revised to apply against ToT (@r40493)
Comment 16 Adam Roben (:aroben) 2009-02-02 20:22:56 PST
Comment on attachment 27261 [details]
Revised to apply against ToT (@r40493)

r=me
Comment 17 Brent Fulgham 2009-02-02 20:50:12 PST
Committed in revision 40520.