691292 - Random modifications to start-msvcN.bat.

Status: RESOLVED FIXED

(Firefox Build System :: MozillaBuild, task)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Modified batch file

Joel Rocha sent me some modifications to the start-msvc9 batch file.

Our irc conversation:

[04:20:05] <up_up_and|away> hi
[04:20:31] <up_up_and|away> Fallen told me to talk to you
[04:20:42] <khuey> ok
[04:20:44] <khuey> about what?
[04:21:06] <up_up_and|away> i changed the file start-msvc9.bat
[04:21:56] <up_up_and|away> i think someone forgot to update this:
[04:21:56] <up_up_and|away>  rem The Win7 SDK (or newer) should automatically
                            integrate itself into vcvars32.bat
[04:21:57] <up_up_and|away>     if "%SDKVER%" == "6" (
[04:22:22] <up_up_and|away> if it can be newer then it should be GEQ
[04:22:24] <up_up_and|away> ?
[04:22:34] <up_up_and|away> i don't know for sure
[04:23:13] <up_up_and|away> all i know is i have the Win7 SDK and it gives 7
                            for the SDKVER
[04:23:34] <up_up_and|away> so, i'm guessing it should use that if
[04:24:08] <up_up_and|away> by the way, my name is Joel Rocha
[04:24:14] <khuey> which win7 sdk do you have?
[04:24:22] <khuey> there's more than one :-D
[04:24:35] <up_up_and|away> i don't know and i dont care
[04:24:42] <up_up_and|away> :D
[04:24:44] <khuey> heh
[04:24:46] <khuey> ok
[04:24:55] <khuey> so did the build fail for you the first time you used it?
[04:24:55] <up_up_and|away> focus
[04:25:01] <up_up_and|away> no
[04:25:16] <up_up_and|away> i was just bored because the build was so long
[04:25:27] <khuey> ah
[04:25:32] <up_up_and|away> i started popping up some other files
[04:25:36] <khuey> so, it's not obvious to me why it's == and not GEQ
[04:25:47] <up_up_and|away> that's when i saw this
[04:26:34] <khuey> and there's nothing in the bug that added this that explains
                   that
[04:26:43] <up_up_and|away> ok
[04:24:35] <up_up_and|away> i don't know and i dont care
[04:24:42] <up_up_and|away> :D
[04:24:44] <khuey> heh
[04:24:46] <khuey> ok
[04:24:55] <khuey> so did the build fail for you the first time you used it?
[04:24:55] <up_up_and|away> focus
[04:25:01] <up_up_and|away> no
[04:25:16] <up_up_and|away> i was just bored because the build was so long
[04:25:27] <khuey> ah
[04:25:32] <up_up_and|away> i started popping up some other files
[04:25:36] <khuey> so, it's not obvious to me why it's == and not GEQ
[04:25:47] <up_up_and|away> that's when i saw this
[04:26:34] <khuey> and there's nothing in the bug that added this that explains
                   that
[04:26:43] <up_up_and|away> ok
[04:26:51] <up_up_and|away> let's leave it that way then
[04:26:57] <up_up_and|away> anyway
[04:27:15] <up_up_and|away> the following code will never work anyway
[04:27:33] <up_up_and|away> if "%SDKVER%" == "6" (
[04:27:33] <up_up_and|away>         if "%SDKMINORVER%" == "1" (
[04:27:33] <up_up_and|away>           SET USESDK=1
[04:27:33] <up_up_and|away>         )
[04:27:33] <up_up_and|away>     )
[04:27:33] <up_up_and|away>     if "%USESDK%" == "0" (
[04:27:33] <up_up_and|away>         ECHO Using VC 2008 built-in SDK
[04:27:33] <up_up_and|away>     )
[04:28:13] <up_up_and|away> what happens is that the %usesSDK% does not get
                            re-evaluated after the set usesSDK
[04:28:52] <up_up_and|away> i know that this is not important but someone
                            messed up this one
[04:28:55] <khuey> orly?
[04:29:06] <khuey> does %foo% get evaluated at parse time?
[04:30:09] <up_up_and|away> when the batch file gets loaded the vars get all
                            evaluated and stored in a cache
[04:30:56] <up_up_and|away> as far as i can tell: in the beggining of an if
                            block at least, i know that this happens
[04:31:08] <up_up_and|away> (happens on FOR blocks also)
[04:31:17] <khuey> interesting
[04:31:23]  * khuey wonders how this works at all then
[04:31:23] <up_up_and|away> the vars in it get evaluated and cached
[04:31:41] <khuey> since the lines further down about USEPSDKATL/IDL do the
                   same thing
[04:32:11] <up_up_and|away> if you set a var within an if block you cannot use
                            the var in that same if block
[04:32:33] <up_up_and|away> that is why USEPSDKATL/IDL use a different if block

We should figure out which of these modifications we need.

Comment 1

[Joel Rocha]

Attachment: previous file was lacking some lines included on the original start-msvc9.bat

The Delayed Expansion flag enables us to create a more readable file.

Comment 2

[Joel Rocha]

I removed:
if "%SDKVER%" == "6" (
   if "%SDKMINORVER%" == "1" (
		SET USESDK=1
   ) else (
		ECHO Using VC 2008 built-in SDK
	)
)

From my initial proposal because: if %USESDK% is set externally, and the developer knows what he is doing, then after this block:
if "%SDKVER%" == "6" (
    if "%SDKMINORVER%" == "1" (
        SET USESDK=1
    )
)
if "%USESDK%" == "0" (
    ECHO Using VC 2008 built-in SDK
)
The echo can be made, if %USESDK% has been previously set to 0, and %USESDK%=1 due to the lack of the delayed evaluation of variables. Since i don't know if this is the desired behavior, i decided to remove my proposal.

Also, some comments in the file refer to SDKVER=6 as:
rem SDK Ver.6.0 (Windows Vista SDK) and newer
and
rem If the SDK is Win2k8, we want to use it.

Comment 3

[Joel Rocha]

Attachment: Changes on guess-msvc.bat

Comment 4

[Joel Rocha]

Changed the way Errorlevel is dealed in guess-msvc.bat file.
Might make the file run faster by not making so much REG QUERY's

Last statement was sligtly useless as it tried to:
setlocal disableextensions enabledelayedexpansion
When the delayed expansion was already activated a few lines earlier

With this new changes var WIN64 set on line 21 is not used ever since, at least on start-msvc9 and start-msvc9-x64

Comment 5

[Joel Rocha]

Attachment: Eliminate the need for maintenance on a second x64 file

I eliminated everything on start-msvc9-x64 leaving just a reference for backward compatibility:
start-msvc9 x64

This eliminates the hassle to copy the changes from the start-msvc9 to start-msvc9-x64

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ted, can you take a look at this stuff?

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 564580 [details] [diff] [review]
Changes on guess-msvc.bat

Review of attachment 564580 [details] [diff] [review]:
-----------------------------------------------------------------

I'm going to go ahead and say that you know a lot more about batch files than I do. I'll just r+ this, and hoep it doesn't break anything.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 564654 [details] [diff] [review]
Eliminate the need for maintenance on a second x64 file

Review of attachment 564654 [details] [diff] [review]:
-----------------------------------------------------------------

I'm out of my league here, I admit it. Also, I just realized that this patch is an updated version of the previous patch.

Comment 9

[Ryan VanderMeulen (PTO, back July 27)]

Joel, I would very much love to improve the start-msvc* situation. Any chance you'd still be interested in working on this?

Comment 10

[Joel Rocha]

Where do you want to go today?

Comment 11

[Ryan VanderMeulen (PTO, back July 27)]

Ideally, I'd love one batch file to rule them all, but I'd settle for just removing the 32/64-specific versions.

Comment 12

[Joel Rocha]

As far as i can see i've proposed changes to end that, at least on start-msvc9-x64.bat that was changed to a one liner: start-msvc9 x64 for backwards compatibility

Comment 13

[Joel Rocha]

Have my changes been approved? I see that Ted reviewed my code with an r+ do i have to do anything? Or just let someone else approve the code i sent?

Comment 14

[Ryan VanderMeulen (PTO, back July 27)]

The main issue at this point would be the need for them to be rebased.

Comment 15

[Joel Rocha]

Attachment: guess-msvc.bat.patch

The proposed changes shall improve the speed of the build while maintaining functionality.

Comment 16

[Joel Rocha]

Attachment: Eliminate the need for maintenance on a second x64 file

I can't guarantee the correctness of this file because the previous batch file had conflicting execution paths to give priority to Windows 8.x SDK's. I decided to chose the one i think it's correct and hope for the best :)
Please test this before check-in.

Comment 17

[Joel Rocha]

Attachment: Eliminate the need for maintenance on a second x64 file

Changed the previous patch. Where it says:
set VCVarsFilename=vcvarsamd64.bat
should be read:
set VCVarsFilename=vcvars64.bat

Sorry about that.

Comment 18

[Joel Rocha]

Attachment: guess-msvc.bat.patch update after bug 990983 fix

Updated guess-msvc.bat file after https://bugzilla.mozilla.org/show_bug.cgi?id=990983

Comment 19

[Ryan VanderMeulen (PTO, back July 27)]

Comment on attachment 8454417 [details] [diff] [review]
Eliminate the need for maintenance on a second x64 file

This bug is sort of a catch-all for multiple issues that should really be handled in their own bugs. I'm going to land the cleanup attachment here and move the bigger refactoring to another bug. Joel, can you ping me on IRC at some point so we can discuss further? I've got some ideas in mind and I'd like to bounce them off you.

Comment 20

[Ryan VanderMeulen (PTO, back July 27)]

Comment on attachment 8454878 [details] [diff] [review]
guess-msvc.bat.patch update after bug 990983 fix

Review of attachment 8454878 [details] [diff] [review]:
-----------------------------------------------------------------

I've played with this locally and it seems to work well. I think getting this in as a first-step to other refactoring is a good idea.

Comment 21

[Ryan VanderMeulen (PTO, back July 27)]

http://hg.mozilla.org/mozilla-build/rev/587f5ce581e5